Route badly interpolated dynamic index to DLQ #1084

andsel · 2022-09-07T14:13:41Z

Release notes

Modified the resolution of dynamic index name so that in case of interpolation errors the event is sent to the DLQ. This feature could be explicitly disabled configuring properly the dlq_on_failed_indexname_interpolation.

What does this PR do?

Update the the pre-indexing steps, so to that in case of an IndexInterpolationError is raised during the index name resolution, the event is sent to DLQ. It leverarges the refactored #handle_dlq_status from PR #1080 to reuse the enqueuing logic.

Why is it important/What is the impact to the user?

Let the user to handle specifically the events which once indexed would create an ugly formatted name like %{[not_existing_field]}_index.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~[ ] I have made corresponding change to the default configuration files (and/or docker env variables)~~
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

run with

.ci/docker-setup.sh && INTEGRATION=true .ci/docker-run.sh

adds feature flag to disable this behavior. Track telemetry data to know how many uses this explicit disabling.

How to test this PR locally

enable the DLQ, set dead_letter_queue.enable: true in config/logstash.yml.
launch an Elasticsearch instance
execute a pipeline which wouldn't resolve the index name like:

input {
	generator {
		message => '{"name": "John", "surname": "Doe"}'
		count => 1
		codec => json
	}
}

output {
	elasticsearch {
		index => "%{[not_existing_field]}"
		hosts => "http://localhost:9200"
		user => "elastic"
		password => "changeme"
	}
}

then drain the DQL with another pipeline and verify it contains the previous failed event:

input {
  dead_letter_queue {
  	path => "/path_to/data/dead_letter_queue/"
  }
}

output {
  stdout {
    codec => rubydebug
  }

Related issues

Fixes DLQ when sprintf references a missing field #718
Copies a refactoring done in Validate the action before sending the bulk request to ES. #1080

Use cases

As a user I don't want that unresolved index names generates unexpected indexes and I would to reprocess such failed events with another DLQ pipeline.

…t create an Elasticsearch index

…ot fully resolved

… resolution in the index name

…indexnames

mashhurs · 2022-09-19T16:09:59Z

spec/integration/outputs/index_spec.rb

+    let(:dlq_writer) { double('DLQ writer') }
+    before { subject.instance_variable_set('@dlq_writer', dlq_writer) }
+
+    it "should doesn't create an index name with unresolved placeholders" do


Suggested change

it "should doesn't create an index name with unresolved placeholders" do

it "shouldn't create an index name with unresolved placeholders" do

mashhurs · 2022-09-19T21:49:44Z

CHANGELOG.md

@@ -1,3 +1,6 @@
+## 11.9.0
+ - Feature: force unresolved dynamic index names to be sent into DLQ. This feature could be explicitly disabled using `dlq_on_failed_indexname_interpolation` setting [#1084](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1084)


From the explanation, for me dlq_on_unresolved_index_names would be good for me. We interpolate the index name to resolve it. The end goal seems to me resolving the index name.

mashhurs · 2022-09-19T21:50:37Z

docs/index.asciidoc

+* Value type is <<boolean,boolean>>
+* Default value is `true`.
+
+If enabled, failed index name interpolation events go into dead letter queue.


should be rephrased if we align on dlq_on_unresolved_index_names.

mashhurs · 2022-09-19T22:04:54Z

lib/logstash/outputs/elasticsearch.rb

@@ -362,11 +365,43 @@ def config_init(params)
  # Receive an array of events and immediately attempt to index them (no buffering)
  def multi_receive(events)
    wait_for_successful_connection if @after_successful_connection_done
-    retrying_submit map_events(events)
+    events_mapped = safe_interpolation_map_events(events)


How do you think if we reject index name unresolved events and only bring back successful events?
In my unmerged PR, we had a great discussion with Ry and agreed to work with successful/submittable events here, send to DLQ rejected events wen filtering.

mashhurs · 2022-09-19T22:06:43Z

lib/logstash/outputs/elasticsearch.rb

+         failed_events << event_action_tuple
+       end
+    end
+    MapEventsResult.new(successful_events, failed_events)


Suggested change

MapEventsResult.new(successful_events, failed_events)

unless failed_events.empty?

@logger.error("Can't map some events, needs to be handled by DLQ #{failed_events}")

send_failed_resolutions_to_dlq(failed_events)

end

successful_events

mashhurs · 2022-09-19T22:09:09Z

lib/logstash/plugin_mixins/elasticsearch/common.rb

+
+      log_level = dig_value(response, 'index', 'error', 'type') == 'invalid_index_name_exception' ? :error : :warn
+
+      handle_dlq_status(action, log_level, detailed_message)


Special thanks for including handle_dlq_response & handle_dlq_status!

yaauie

Changes LGTM.

I'm not terribly fond of raising exceptions for flow control, but feel that the structure as-proposed here is simpler and much more clear than alternatives.

My only concern is conflicts with the also-in-flight #1080, which similarly pre-filters events whose action cannot be interpolated.

mashhurs · 2022-09-19T23:30:24Z

Changes LGTM.

I'm not terribly fond of raising exceptions for flow control, but feel that the structure as-proposed here is simpler and much more clear than alternatives.

My only concern is conflicts with the also-in-flight #1080, which similarly pre-filters events whose action cannot be interpolated.

Initially I was thinking that seemed to me easily include my #1080 changes but it takes sometime to make an adjustment (especially unit test cases). Feel free to go first @andsel !

yaauie · 2022-09-20T01:50:03Z

Merged and published:

Published logstash-output-elasticsearch-11.9.0-java.gem

andsel self-assigned this Sep 7, 2022

andsel mentioned this pull request Sep 7, 2022

[Meta, 8.4, 8.5] Logstash DLQ Improvements for 8.x elastic/logstash#10493

Open

22 tasks

andsel changed the title ~~Feature/dlq bad dynamic index replacements~~ Route badly interpolated dynamic index to DLQ Sep 7, 2022

roaksoax added the status:work-in-progress label Sep 7, 2022

andsel mentioned this pull request Sep 12, 2022

Validate the action before sending the bulk request to ES. #1080

Merged

andsel added 5 commits September 19, 2022 10:36

Added unit test to verify that bad replaced dynamic index name doesn'…

0fd1044

…t create an Elasticsearch index

Added scaffolding to do not send to ES index dynamic names that are n…

d5282fd

…ot fully resolved

Fixed test to check no index is created in case of missed placeholder…

175ae5a

… resolution in the index name

Minor, removed commented code

35b0a57

Route unresolved index name events to DLQ

05e68bf

andsel force-pushed the feature/dlq_bad_dynamic_index_replacements branch from f8cf7af to 05e68bf Compare September 19, 2022 08:38

andsel added 3 commits September 19, 2022 14:01

Extracted common dlq sending method

f381cff

Minor, removed commented code

b6b8981

Added feature flag to eventually disable the DLQ-ing of not replaced …

6379134

…indexnames

andsel added the enhancement label Sep 19, 2022

andsel marked this pull request as ready for review September 19, 2022 14:03

Bumped version

6a66e14

andsel force-pushed the feature/dlq_bad_dynamic_index_replacements branch from 4f4cbbc to 6a66e14 Compare September 19, 2022 14:34

roaksoax added status:needs-review and removed status:work-in-progress labels Sep 19, 2022

andsel requested review from yaauie and mashhurs September 19, 2022 15:33

mashhurs requested changes Sep 19, 2022

View reviewed changes

yaauie approved these changes Sep 19, 2022

View reviewed changes

roaksoax added status:changes-requested and removed status:needs-review labels Sep 19, 2022

yaauie merged commit b401631 into logstash-plugins:main Sep 20, 2022

roaksoax removed the status:changes-requested label Sep 20, 2022

This was referenced Dec 21, 2022

DLQ error messages squashed in string instead of keeping structure when logging in json #1103

Closed

dlq-logging: reintroduce structured payload where possible #1105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Route badly interpolated dynamic index to DLQ #1084

Route badly interpolated dynamic index to DLQ #1084

Uh oh!

andsel commented Sep 7, 2022 •

edited

Loading

Uh oh!

mashhurs Sep 19, 2022

Uh oh!

mashhurs Sep 19, 2022

Uh oh!

mashhurs Sep 19, 2022

Uh oh!

mashhurs Sep 19, 2022

Uh oh!

mashhurs Sep 19, 2022

Uh oh!

mashhurs Sep 19, 2022

Uh oh!

yaauie left a comment

Uh oh!

mashhurs commented Sep 19, 2022

Uh oh!

yaauie commented Sep 20, 2022

Uh oh!

Uh oh!

	it "should doesn't create an index name with unresolved placeholders" do
	it "shouldn't create an index name with unresolved placeholders" do

		@@ -1,3 +1,6 @@
		## 11.9.0
		- Feature: force unresolved dynamic index names to be sent into DLQ. This feature could be explicitly disabled using `dlq_on_failed_indexname_interpolation` setting [#1084](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1084)

-    MapEventsResult.new(successful_events, failed_events)
+    unless failed_events.empty?
+      @logger.error("Can't map some events, needs to be handled by DLQ #{failed_events}")
+      send_failed_resolutions_to_dlq(failed_events)
+    end
+    successful_events


		log_level = dig_value(response, 'index', 'error', 'type') == 'invalid_index_name_exception' ? :error : :warn

		handle_dlq_status(action, log_level, detailed_message)

Route badly interpolated dynamic index to DLQ #1084

Route badly interpolated dynamic index to DLQ #1084

Uh oh!

Conversation

andsel commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Uh oh!

mashhurs Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mashhurs Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mashhurs Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mashhurs Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mashhurs Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mashhurs Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

yaauie left a comment

Choose a reason for hiding this comment

Uh oh!

mashhurs commented Sep 19, 2022

Uh oh!

yaauie commented Sep 20, 2022

Uh oh!

Uh oh!

andsel commented Sep 7, 2022 •

edited

Loading