Added origins of pipeline configurations #11123

andsel · 2019-09-06T11:51:23Z

Added origins of pipeline configurations, for example config string when configuration is passed with -e on command line or the paths of config files used.

robbavey

Nice work @andsel - just missing a few config. sources and I have a question on whether you think we might be outputting this information too regularly (particularly for customers who are using complex configurations with many config files)

robbavey · 2019-09-06T17:54:49Z

logstash-core/lib/logstash/java_pipeline.rb

+          "config string"
+        when "file"
+          config_part.id
+      end


There are some sources missing here, which lead to this value being set to nil - monitoring pipeline, central pipeline management and modules

robbavey · 2019-09-06T17:58:47Z

logstash-core/lib/logstash/java_pipeline.rb

+
+    pipeline_sources = []
+    pipeline_config.config_parts.each do |config_part|
+      pipeline_sources <<


This is missing some source types - managed pipelines, monitoring pipelines and modules

robbavey · 2019-09-06T18:04:25Z

logstash-core/lib/logstash/java_pipeline.rb

@@ -472,6 +472,20 @@ def maybe_setup_out_plugins
  def default_logging_keys(other_keys = {})


I wonder if default_logging_keys is necessarily the right place for this - this is logged in 16 different places in this file. Maybe we could use it more selectively (on startup, on failure?)

andsel · 2019-09-09T15:30:01Z

@robbavey fixed your suggestions. I log the pipeline source only at the startup of a pipeline, moved the code to extract the pipeline source from PipelineConfig into pipelines' root AbstractPipelineExt.

robbavey

Getting very close! If we can get some tests in too, that would be awesome

robbavey · 2019-09-09T18:27:44Z

logstash-core/lib/logstash/java_pipeline.rb

        "pipeline.workers" => pipeline_workers,
        "pipeline.batch.size" => batch_size,
        "pipeline.batch.delay" => batch_delay,
-        "pipeline.max_inflight" => max_inflight))
+        "pipeline.max_inflight" => max_inflight)
+      pipeline_log_params["pipeline.sources"] = pipeline_source_details


Can you also add this to

logstash/logstash-core/lib/logstash/java_pipeline.rb

Line 112 in 5578310

logger.error("Pipeline aborted due to error", default_logging_keys(:exception => e, :backtrace => e.backtrace))

robbavey · 2019-09-09T18:28:13Z

logstash-core/lib/logstash/pipeline.rb

@@ -636,6 +639,20 @@ def maybe_setup_out_plugins
  def default_logging_keys(other_keys = {})
    keys = super
    keys[:thread] ||= thread.inspect if thread
+


Can be removed

robbavey · 2019-09-09T18:28:57Z

logstash-core/lib/logstash/pipeline.rb

+        "pipeline.batch.size" => settings.get("pipeline.batch.size"),
+        "pipeline.batch.delay" => settings.get("pipeline.batch.delay"))
+
+    pipeline_log_params["pipeline.sources"] = pipeline_source_details


Can you also add this to

logstash/logstash-core/lib/logstash/pipeline.rb

Line 186 in 5578310

@logger.error("Pipeline aborted due to error", default_logging_keys(:exception => e, :backtrace => e.backtrace))

robbavey · 2019-09-09T18:32:52Z

logstash-core/src/main/java/org/logstash/execution/AbstractPipelineExt.java

+                case "x-pack-config-management":
+                    pipelineSources.add(RubyString.newString(context.runtime, "central pipeline management"));
+                    break;
+                case "module":


Ideally I think we might want the name of the module here, but it might not be too important as we should be able to glean it from elsewhere

robbavey

Really close to getting this through! A couple of nitpicks around unnecessary code, and an ask for an extra test and we should be good to go!

robbavey · 2019-09-10T14:52:26Z

logstash-core/lib/logstash/java_pipeline.rb

@@ -225,11 +227,14 @@ def start_workers
      config_metric.gauge(:graph, ::LogStash::Config::LIRSerializer.serialize(lir))
      config_metric.gauge(:cluster_uuids, resolve_cluster_uuids)

-      @logger.info("Starting pipeline", default_logging_keys(
+      pipeline_log_params = default_logging_keys(


Nit: No need for the extra assignment:

Suggested change

pipeline_log_params = default_logging_keys(

pipeline_log_params = default_logging_keys(

"pipeline.workers" => pipeline_workers,

"pipeline.batch.size" => batch_size,

"pipeline.batch.delay" => batch_delay,

"pipeline.max_inflight" => max_inflight))

"pipeline.max_inflight" => max_inflight,

"pipeline.sources" => pipeline_source_details)

@logger.info("Starting pipeline", default_logging_keys)

robbavey · 2019-09-10T14:52:55Z

logstash-core/lib/logstash/pipeline.rb

-      "pipeline.workers" => settings.get("pipeline.workers"),
-      "pipeline.batch.size" => settings.get("pipeline.batch.size"),
-      "pipeline.batch.delay" => settings.get("pipeline.batch.delay")))
+    pipeline_log_params = default_logging_keys(


Nit: Again no need for separate assignment

robbavey · 2019-09-10T14:53:28Z

logstash-core/lib/logstash/pipeline.rb

@@ -180,7 +182,9 @@ def start
        @finished_run.make_true
      rescue => e
        close
-        @logger.error("Pipeline aborted due to error", default_logging_keys(:exception => e, :backtrace => e.backtrace))
+        pipeline_log_params = default_logging_keys(:exception => e, :backtrace => e.backtrace)


Nit again, no need for extra assignment, add the extra field to the default_logging_keys method call

robbavey · 2019-09-10T14:54:21Z

qa/integration/specs/pipeline_log_spec.rb

+    plainlog_file = "#{temp_dir}/logstash-plain.log"
+    expect(File.exists?(plainlog_file)).to be true
+    expect(IO.read(plainlog_file) =~ /Starting pipeline.*"pipeline.sources"=>\["config string"\]/).to be > 0
+  end


Can we also add a test for logstash starting via a config file as well as config string? I think that is probably where this functionality has the most utility.

andsel · 2019-09-10T15:53:30Z

thanks @robbavey for precious suggestions, now it should be fine

robbavey

Couple more things - I think we should be able to improve the test flakiness and a nitpick on a leftover puts statement in the test, and then happy to see this merged

robbavey · 2019-09-10T18:53:07Z

qa/integration/specs/pipeline_log_spec.rb

+      "pipeline.id" => pipeline_name
+    }
+    IO.write(@ls.application_settings_file, settings.to_yaml)
+    puts "initial_config_file: #{initial_config_file}"


No need for puts statement

robbavey · 2019-09-10T18:55:11Z

qa/integration/specs/pipeline_log_spec.rb

+    IO.write(@ls.application_settings_file, settings.to_yaml)
+    puts "initial_config_file: #{initial_config_file}"
+    @ls.spawn_logstash("-w", "1", "-f", "#{initial_config_file}")
+    @ls.wait_for_logstash


I think this line might be causing the build flakiness. as Logstash comes up and down before the call to the API endpoint can be completed. Removing this line from this (and the other logging tests) might help

@robbavey fixed this bringing code to check LS process termination

robbavey

LGTM

…s of config files used, module). closes 9630

jsvd · 2019-09-25T13:39:32Z

@andsel @robbavey should we also get this into 7.x (7.5.0)?

robbavey · 2019-09-25T13:57:21Z

@jsvd - yes we should

andsel · 2019-09-30T08:18:29Z

this is already present in branch 7.x (1cbaeeb) has to be ported also to 7.5?

robbavey mentioned this pull request Sep 6, 2019

[Meta Issue] Logging Improvements #11074

Closed

8 tasks

elasticsearch-bot self-assigned this Sep 6, 2019

robbavey unassigned elasticsearch-bot Sep 6, 2019

robbavey self-requested a review September 6, 2019 16:50

robbavey requested changes Sep 6, 2019

View reviewed changes

robbavey requested changes Sep 9, 2019

View reviewed changes

andsel requested a review from robbavey September 10, 2019 14:31

robbavey requested changes Sep 10, 2019

View reviewed changes

robbavey reviewed Sep 10, 2019

View reviewed changes

robbavey approved these changes Sep 11, 2019

View reviewed changes

Added origins of pipeline's configuration (es config string, the path…

0e622cc

…s of config files used, module). closes 9630

andsel force-pushed the feature/add_pipeline_configuration_origins branch from 34b5880 to 0e622cc Compare September 12, 2019 14:04

andsel merged commit 0e622cc into elastic:master Sep 12, 2019

jsvd added v7.5.0 v8.0.0 labels Sep 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added origins of pipeline configurations #11123

Added origins of pipeline configurations #11123

andsel commented Sep 6, 2019

robbavey left a comment

robbavey Sep 6, 2019

robbavey Sep 6, 2019

robbavey Sep 6, 2019

andsel commented Sep 9, 2019

robbavey left a comment

robbavey Sep 9, 2019

robbavey Sep 9, 2019

robbavey Sep 9, 2019

robbavey Sep 9, 2019

robbavey left a comment

robbavey Sep 10, 2019

robbavey Sep 10, 2019

robbavey Sep 10, 2019

robbavey Sep 10, 2019

andsel commented Sep 10, 2019

robbavey left a comment

robbavey Sep 10, 2019

robbavey Sep 10, 2019

andsel Sep 11, 2019

robbavey left a comment

jsvd commented Sep 25, 2019

robbavey commented Sep 25, 2019

andsel commented Sep 30, 2019

		@@ -472,6 +472,20 @@ def maybe_setup_out_plugins
		def default_logging_keys(other_keys = {})

Added origins of pipeline configurations #11123

Added origins of pipeline configurations #11123

Conversation

andsel commented Sep 6, 2019

robbavey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andsel commented Sep 9, 2019

robbavey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbavey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andsel commented Sep 10, 2019

robbavey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbavey left a comment

Choose a reason for hiding this comment

jsvd commented Sep 25, 2019

robbavey commented Sep 25, 2019

andsel commented Sep 30, 2019