undefined method `compact' when tags field is an array

[pmazurek]

There seems to be an issue with the way riemann plugin handles tags.

Given I have the following data forwarded from logstash to riemann output:

{
  "@version": 1,
  "@timestamp": "2015-09-16T13:18:01.995Z",
  "host": "mylocalhost",
  "logger": "python-logstash-logger",
  "type": "logstash",
  "riemann_metric": {
    "metric": 1.1,
    "state": 1,
    "service": "myservice",
    "ttl": 230
  },
  "tags": [],
  "path": "make-logs.py",
  "message": "python-logstash: test",
  "levelname": "INFO"
}

Riemann output is going to receive this data and then silently fail - there is no information about whats happening and why.
If however I use the following data - the only difference is tags:

{
  "tags": "foobar",
  "@timestamp": "2015-09-16T13:18:01.995Z",
  "@version": 1,
  "type": "logstash",
  "host": "mylocalhost",
  "riemann_metric": {
    "metric": 1.1,
    "state": "ok",
    "service": "myservice",
    "ttl": 230
  },
  "path": "make-logs.py",
  "logger": "python-logstash-logger",
  "message": "python-logstash: test extra fields",
  "levelname": "INFO"
}

Everything works like a charm.

My investigation of the ruby gem showed that when I use an array, send_to_riemann is going to throw the following exception:
undefined method compact' for <Java::JavaUtil::ArrayList:1 []>:Java::JavaUtil::ArrayList`

[sonnysideup]

After having encountered this same issue today, I feel inclined to chime in here.

1. The event object passed to the #receive method of this class may have a tags key that points to an instance of Java::JavaUtil::ArrayList. This blows up whenever it passed to the protobuf client.
2. What does this operation even do? It looks like a pointless carryover from logstash-contrib.
3. Blind rescue of Exception errors is bad practice. Reduce the scope of this rescue block to focus on specific errors thrown by Riemann::Client, thereby allowing other exceptions to properly bubble up.
4. Unhandled exceptions should be logged at an error level, not debug.

Is the solution as simple as casting the Java obj whenever we encounter it?

  def build_riemann_formatted_event(event)
    ....

    if @map_fields == true
      r_event.merge! map_fields(nil, event.to_hash)
    end
    # Casting to an Array preserves the items within and removes the `compact' error
    r_event[:tags] = event["tags"].to_a if event["tags"].is_a?(Java::JavaUtil::ArrayList)

    return r_event
  end

[jhitze]

It's an interesting fix, however, it doesn't help if you have other nested fields deeper in your event stack. It also doesn't send any tags to riemann if the tags isn't specifically a Java ArrayList. So I'd hold of on trying to use that.

Check out #9, which is closed now because elastic/logstash#3772 was merged in a bit ago. It looks like it's waiting on another logstash release. (1.5.5? 2.0.0?) Perhaps @guyboertje could weigh in on when it'll be released.

@drywheat, I can answer a few of your comments:

2. It's dumping all the logstash event tags into the top-level riemann event, since tags is a native element that riemann can react to.
3. It's certainly a bad practice in general. In this case we don't want the entire logstash process to blow up if something goes bad with one plugin.
4. An issue has already been filed for this. Exceptions should log to error #8

Otherwise, I believe this issue can be marked as closed, since it is a duplicate of #9.

[guyboertje]

@pmazurek, @drywheat, @jhitze - Thank you for your post and comments. I will try to address your concerns systematically.

A fix for this has been merged but not released. The fix only applies to 1.5.5+, as, for 2.0, we are using a newer version of JrJackson that yields proper Ruby Arrays and Hashes at better performance than the previous version (when yielding Java ArrayList and HashMap). ATM, I can't confirm that the JrJackson change will be applied to 1.5.5.

Any attempts to recursively rubyify the Java objects have a serious negative impact on pipeline throughput. Further, what you think is a ArrayList is actually a JRuby JavaProxy that transparently reports its class as the one it is proxying. This proxy is used to make sure that any Java objects are seen as having the Ruby Object API.

RE: Blind rescue of Exception errors is bad practice: There are times when it is acceptable to do this however this is not one of them. We have an issue 2477 to design/discuss the way forward. This plugin is one of many that do not handle plugin specific error conditions well. We will have to visit every one to refactor the error handling. Some external libraries define their own Error classes which are not always sub-classes of StandardError.
Further, as logstash is very multi-threaded, any exceptions bubbling up to the pipeline thread can stop the whole pipeline therefore we have enabled the abort_on_exception flag on the Thread class, meaning that the input/worker/output thread may die when an error bubbles up and we (in master) make a best effort to log this. But as you will appreciate, attempting to log some fatal errors may create more fatal errors.

As none of the LS dev use Riemann, we are not immediately familiar what errors the transmission to Riemann might raise and that the tests adequately cover normal operations but no abnormal ones - we would welcome efforts by you guys to catalogue and characterise the various ways in which the transmission might fail. Catalogue meaning write them all down, even the JRuby ones (networking etc) and Characterise meaning your opinion on whether they are retryable or fatal.

I agree that any errors logged should be at the error level.