Use integration metadata to create ES actions (part 2) #1158

andsel · 2023-11-03T14:29:04Z

Release notes

Added support for propagating event processing metadata when this output is downstream of an Elastic Integration Filter and configured without explicit version, version_type, or routing directives

What does this PR do?

Change the creation of actions that are passed down to Elasticsearch to use also the metadata fields set by an integration.
The interested fields are version, version_type and routing, the field values are taken verbatim without placeholders resolution.
The version, version_type and routing that are configured in the plugin settings have precedence on the integration ones because manifest an explicit choice made by the user.

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

This PR complete the fix related to interoperability issue with Agent's integrations (started with #1155), where some metadata valued by integration has to be used down to Elasticsearch.

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

use the plugin with an integration that configures the document id, and in case of same id no new documents are indexed.

How to test this PR locally

Please refer to the same flow used in #1155

Related issues

Closes Use integration's metadata fields (_routing, _version, _version_type) when present #1157

…etadata

…proper context

…rams method

spec/unit/outputs/elasticsearch_spec.rb

yaauie · 2023-11-14T22:42:06Z

lib/logstash/outputs/elasticsearch.rb

@@ -557,9 +554,30 @@ def common_event_params(event)
    #      }
    params[:pipeline] = target_pipeline unless (target_pipeline.nil? || target_pipeline.empty?)

+    params[:version] = resolve_version(event, event_version)
+    params[:version_type] = resolve_version_type(event, event_version_type)
+
    params


The issues raised elsewhere could likely be resolved by leaning on Hash#compact, which is available since Ruby 2.4 (and therefore on all Logstash 7+):

Suggested change

params

params.compact

spec/unit/outputs/elasticsearch_spec.rb

yaauie · 2023-11-14T22:53:37Z

lib/logstash/outputs/elasticsearch.rb

@@ -557,9 +554,30 @@ def common_event_params(event)
    #      }
    params[:pipeline] = target_pipeline unless (target_pipeline.nil? || target_pipeline.empty?)

+    params[:version] = resolve_version(event, event_version)
+    params[:version_type] = resolve_version_type(event, event_version_type)


alternate approaches to Hash#compact to avoid storing nil values:

use a local variable:

Suggested change

params[:version_type] = resolve_version_type(event, event_version_type)

resolved_version_type = resolve_version_type(event, event_version_type)

params[:version_type] = resolved_version_type unless resolved_version_type.nil?

rely on safe-descent &. with Object#tap:

Suggested change

params[:version_type] = resolve_version_type(event, event_version_type)

resolve_version_type(event, event_version_type)&.tap { |resolved| params.store(:version_type, resolved) }

Thank's @yaauie to have provided also this alternative, I've opted for the one-liner compact

routing => nil is expected in some tests:
https://app.travis-ci.com/github/logstash-plugins/logstash-output-elasticsearch/jobs/613384063#L4748

This comes from the fixture

logstash-output-elasticsearch/spec/integration/outputs/retry_spec.rb

Lines 9 to 19 in 47a5169

ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17", :_type=> doc_type }, event1.to_hash])

else

ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17" }, event1.to_hash])

end

end

let(:event2) { LogStash::Event.new("geoip" => { "location" => [ 0.0, 0.0] }, "@timestamp" => "2014-11-17T20:37:17.223Z", "@metadata" => {"retry_count" => 0}) }

let(:action2) do

if ESHelper.es_version_satisfies?("< 7")

ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17", :_type=> doc_type }, event2.to_hash])

else

ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17" }, event2.to_hash])

I've reverted the behaviour only for routing with commit e91210c

lib/logstash/outputs/elasticsearch.rb

Adding `version => nil` is a change in behaviour because, actually it's never added, so this commit respect the existing logic, do not add event's keys that are empty Co-authored-by: Ry Biesemeyer <[email protected]>

… commit rollback the changes to satisfy that condition

yaauie

LGTM 👍🏼

andsel self-assigned this Nov 3, 2023

andsel force-pushed the feature/use_integration_metadata_to_create_es_actions_part2 branch 3 times, most recently from 8714d88 to 64a6ef5 Compare November 8, 2023 11:26

roaksoax added the status:work-in-progress label Nov 8, 2023

andsel added 12 commits November 10, 2023 10:23

Use pipeline name provided from integration execution if present in m…

f0606c5

…etadata

[Test] Rephrased test descriptions

2677982

Reshaped unit test for index replacement, to cover all cases

47181d7

Moved test about target_ingest_pipeline and ingest_document into the …

56b359c

…proper context

Extracted routing param resolution in separate method

e50c208

Use routing setting from ingest_document metadata, if present

3e5924d

Moved the assignment of version and version_type into common_event_pa…

e6eed9f

…rams method

Use version setting from ingest_document metadata, if present

7560c51

Use version_type setting from ingest_document metadata, if present

1d137a5

Removed badly resolved merge commit

e947f06

Extended test suite to cover all the cases as in originating PR

17dfff7

Removed duplication of tests

ed367e4

andsel force-pushed the feature/use_integration_metadata_to_create_es_actions_part2 branch from 64a6ef5 to ed367e4 Compare November 10, 2023 09:33

andsel added the enhancement label Nov 10, 2023

[Skip CI] Bumped version

6508c0e

andsel changed the title ~~Feature/use integration metadata to create es actions part2~~ Use integration metadata to create ES actions (part 2) Nov 10, 2023

andsel marked this pull request as ready for review November 10, 2023 09:48

andsel requested a review from yaauie November 10, 2023 09:48

roaksoax added status:needs-review and removed status:work-in-progress labels Nov 10, 2023

yaauie reviewed Nov 14, 2023

View reviewed changes

andsel commented Nov 16, 2023

View reviewed changes

lib/logstash/outputs/elasticsearch.rb Show resolved Hide resolved

andsel and others added 2 commits November 16, 2023 09:12

Avoid to store nil values for version_type, version and routing

cc45a0a

Adding `version => nil` is a change in behaviour because, actually it's never added, so this commit respect the existing logic, do not add event's keys that are empty Co-authored-by: Ry Biesemeyer <[email protected]>

The existing behavior is to keep routing => nil in event fields. This…

e91210c

… commit rollback the changes to satisfy that condition

andsel requested a review from yaauie November 16, 2023 13:38

yaauie approved these changes Nov 16, 2023

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels Nov 16, 2023

andsel merged commit cfd82fa into logstash-plugins:main Nov 16, 2023
1 check passed

roaksoax removed the status:approved label Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use integration metadata to create ES actions (part 2) #1158

Use integration metadata to create ES actions (part 2) #1158

andsel commented Nov 3, 2023 •

edited

Loading

yaauie Nov 14, 2023

yaauie Nov 14, 2023

andsel Nov 16, 2023

andsel Nov 16, 2023

andsel Nov 16, 2023

yaauie left a comment

	params[:version_type] = resolve_version_type(event, event_version_type)
	resolved_version_type = resolve_version_type(event, event_version_type)
	params[:version_type] = resolved_version_type unless resolved_version_type.nil?

	params[:version_type] = resolve_version_type(event, event_version_type)
	resolve_version_type(event, event_version_type)&.tap { \|resolved\| params.store(:version_type, resolved) }

	ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17", :_type=> doc_type }, event1.to_hash])
	else
	ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17" }, event1.to_hash])
	end
	end
	let(:event2) { LogStash::Event.new("geoip" => { "location" => [ 0.0, 0.0] }, "@timestamp" => "2014-11-17T20:37:17.223Z", "@metadata" => {"retry_count" => 0}) }
	let(:action2) do
	if ESHelper.es_version_satisfies?("< 7")
	ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17", :_type=> doc_type }, event2.to_hash])
	else
	ESHelper.action_for_version(["index", {:_id=>nil, routing_field_name =>nil, :_index=>"logstash-2014.11.17" }, event2.to_hash])

Use integration metadata to create ES actions (part 2) #1158

Use integration metadata to create ES actions (part 2) #1158

Conversation

andsel commented Nov 3, 2023 • edited Loading

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

yaauie Nov 14, 2023

Choose a reason for hiding this comment

yaauie Nov 14, 2023

Choose a reason for hiding this comment

andsel Nov 16, 2023

Choose a reason for hiding this comment

andsel Nov 16, 2023

Choose a reason for hiding this comment

andsel Nov 16, 2023

Choose a reason for hiding this comment

yaauie left a comment

Choose a reason for hiding this comment

andsel commented Nov 3, 2023 •

edited

Loading