Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Isolate datastream vs normal indexing decision into test fixture #1162

Open
andsel opened this issue Nov 21, 2023 · 0 comments
Open

Isolate datastream vs normal indexing decision into test fixture #1162

andsel opened this issue Nov 21, 2023 · 0 comments

Comments

@andsel
Copy link
Contributor

andsel commented Nov 21, 2023

In fixing issue #1160 the PR #1161 added some testing to cover the case of integration metadata in combination with datastreams.
This led to the the creation of two tests which switched method under-test:

In these tests the method under test switched from event_action_tuple to data_stream_event_action_tuple to test the datastream scenario.
It would mean that abstraction level tested by test isn't right one, ideally the fixture's setup would trigger one path of execution or the other. There shouldn't be knowledge in the test about which method to call, in this context.

This issue is proposing to introduce a change so that the tests haven't knowldege to call event_action_tuple or data_stream_event_action_tuple to proper test the use case, should be exposed a more abstract method which isolate from this decision, and decision is made only based on the data in the fixture.

Possibile solution could be the introduction o action_tuple(event) that uses the @event_mapper (which presently wraps either event_action_tuple or data_stream_event_action_tuple depending on other configuration):

  def action_tuple(event)
    @event_mapper.call(event)
  end

And then changing the current only call-site of @event_mapper.call to go through our new unified boundary:

diff --git a/lib/logstash/outputs/elasticsearch.rb b/lib/logstash/outputs/elasticsearch.rb
index 598265d..55e9a87 100644
--- a/lib/logstash/outputs/elasticsearch.rb
+++ b/lib/logstash/outputs/elasticsearch.rb
@@ -424,7 +424,7 @@ class LogStash::Outputs::ElasticSearch < LogStash::Outputs::Base
     event_mapping_errors = [] # list of FailedEventMapping
     events.each do |event|
       begin
-        successful_events << @event_mapper.call(event)
+        successful_events << action_tuple(event)
       rescue EventMappingError => ie
         event_mapping_errors << FailedEventMapping.new(event, ie.message)
       end

We could then test action_tuple directly for a given plugin config without needing to know whether we should be testing event_action_tuple or data_stream_event_action_tuple.

Another possible way would be to pull the datastream-specific and legacy-specific behaviour out into a self-contained Modules so that setup_mapper_and_target wouldn't need to store procs-that-call-specific-methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant