Raise logging level for 'invalid_index_name_exception' ES exceptions to ERROR

[webmat]

As discussed in #759, an invalid index exception should log at the ERROR level.
Presumably, most of these will happen just after an update to a user's Logstash
configuration.

Still missing in this PR

• Add tests
• Version bump

Questions

• Does dlq_writer.write already support (event, reason, params_hash)?
• When we get an ES exception, are response['index'] and response['index']['error']
  guaranteed to be set, or should I guard against anything in there being nil?
  • Got nil exceptions in the integration tests. Now guarding.

I would posit that making the list of exceptions that are elevated to ERROR is
out of scope for this. We could release this as is, and if there's interest we
make it configurable later on.

Generate an invalid_index_name_exception with

bin/logstash -e "input { generator { count => 1 } }
  output { elasticsearch { user => 'elastic' password => 'elastic'
    index => 'bad,index'
  }}"

Generate another ES exception (mapping) with

bin/logstash -e "input { generator { count => 2 } }
  filter { ruby { code => \" event.set('mapping_fail', (event.get('sequence') == 0 ? {'foo' => 'bar'} : 'foobar' ) ) \"} }
  output { elasticsearch { user => 'elastic' password => 'elastic' }}"

closes #759

[webmat]

This new if is deep in the submit method. This will not be easy to get to for testing purposes. Perhaps I should extract the error logging to a small helper method?

[webmat]

Test failures on 3a848c8 with older versions of ES answered my question about guarding for response['index']['error']. Got nil exceptions there. I'll guard against this and move this code to a small method that'll be easier to test.

[webmat]

I could not repro the nil exceptions as they happened in Travis. Tried with ES 1.7, 2.4, 5.6 and 6.2.4 LOL (all those that failed).

This small method is pretty easy to test and doesn't require much setup.

@jsvd Quick question. There's a TODO in there about sending "status", "action" and "response" as a params hash to dlq_writer.write instead of baking it into the message string. But looking at DeadLetterQueueWriter or DLQEntry, it's not obvious to me whether it supports params args. It looks like it doesn't. Am I missing something?

[jsvd]

@jsvd Quick question. There's a TODO in there about sending "status", "action" and "response" as a params hash to dlq_writer.write instead of baking it into the message string. But looking at DeadLetterQueueWriter or DLQEntry, it's not obvious to me whether it supports params args. It looks like it doesn't. Am I missing something?

I don't think you are, we can see in the java code for the DLQ writer that we expect two objects as arguments, but quickly those get coerced into a LogStash::Event and a string

[jsvd]

I'm not a huge fan of this "report_doc_level_error" to be writing data to the DLQ. Maybe we can have this method only take care of the else case in this if, where the DLQ isn't available.
Another alternative is to call this method something like handle_doc_level_error. the word "handle" works for everything, yay

[webmat]

Yes, handle_ is a better name

[webmat]

Actually this method is specifically for statuses that are candidates for the DLQ. I'll go even further and rename it to handle_dlq_status. It logs if DLQ is not enabled, and it sends to DLQ if it's enabled.

[webmat]

Ah, I had missed the PluginDeadLetterQueueWriter class sitting in the middle. Now it makes sense 😂

[webmat]

We should be good to go for PR, as of commit 5c074c1. Version bump is done, invalid_index_name_exception logs an error instead of a warning, and I've started adding significant tests around handling of events getting statuses that are candidates for the DLQ (whether or not it's enabled/available).

I've noticed a few things while working on this, and I've created follow-up issue #772 to handle them.

[jsvd]

We can do response.fetch('index', {}).fetch('error', {}).get('type') instead.

[webmat]

Ok @jsvd All tests are passing now. I'd like to merge tomorrow if there's nothing else.

Thanks for the terser syntax for digging into nested hashes :-)

[jsvd]

LGTM (please squash the commits)

[elasticsearch-bot]

Mathieu Martin merged this into the following branches!

Branch	Commits
master	b48c0bf, 9e10ad7