Java pipeline part of PR #11155 #11288

andsel · 2019-10-31T11:03:58Z

Abstract
This PR contains the implementation to expose through HTTP API the triple (file, line, column) source's config of every plugin. It contains also shared part with Ruby's one.

Description
This PR introduce the ground part to carry out some metadata from the configuration scripts, these metadata regards source file, line and column in which a specific plugin is defined inside the pipeline and associate it with its runtime generated uuid. These data is exposed inside the metrics of each plugin so that the connection between plugin UUID -> config file reference should be more walkable.

This PR is for java execution pipeline, so interest Java and Ruby plugins executed by the Java pipeline.

Changed PipelineCompiler to accept PipelineConfig instead of SourceWithMetadata array because in PipelineConfig has been introduced a remapping method from global line number (because SourceWithMeta comes with a string that's the join of all pipiline's config files, e.g. inputs in one file, filter in another etc, then before compile that files are merge din one big string) while a single SourceWithMetadata doesn't have visibility on all segments. This happen with help of ConfigSourceSegment.

After this remapping phase, done before the pipeline creation, we have to store the information related to the triple (source, line, column) into the AST Nod so that can be used during the instantiation of plugins, because every instance of a plugin has to carry this information because has to be exposed in the metrics.

Rest of the PR is to propagate and expose this information in the 4 kind of plugins we have, plus the fixing of tests.

andsel · 2019-11-15T13:06:39Z

Jenkins test this please

andsel · 2019-11-22T08:44:53Z

Jenkins test this please

andsel · 2019-11-28T10:24:40Z

Jenkins test this please

logstash-core/lib/logstash/config/pipeline_config.rb

kares · 2019-11-28T15:15:59Z

logstash-core/spec/logstash/compiler/compiler_spec.rb

-          end
-        end
-      end
+#       describe "applying protocol and id metadata" do


what's going on here? 😮

The motivation is that the method LogStash::Compiler::compile_sources before accepted an array sources_with_metadata and now accept a PipelineConfig object that contains the array.
The test before verified that the compile generated a list of tree respecting the order of pipelines string provided in input, but the compile method before was called in production code only with an Array of a single SourceWithMetadata. So test verified a thing that could never happen in production code, and now using a single SourceWithMetadata and not a list the test has no meaning.

If you agree with the unusefulness of this code I remove it

okay I see ... if its not used I vote for removal.

if you really want to keep it (as its potentially to be resurrected later - which I really can not tell) than I would consider adding a comment (like you explained above) with a TODO/NOTE tag on why its commented out.

andsel · 2019-11-28T17:42:25Z

Jenkins test this please

qa/integration/specs/monitoring_api_spec.rb

logstash-core/src/test/java/org/logstash/config/ir/compiler/FakeOutClass.java

logstash-core/src/test/java/org/logstash/config/ir/CompiledPipelineTest.java

logstash-core/src/main/java/org/logstash/plugins/PluginFactoryExt.java

kares · 2019-12-04T13:06:33Z

logstash-core/src/main/java/org/logstash/config/ir/compiler/OutputDelegatorExt.java

+        String configRef = args.op_aref(context, configRefKey).asJavaString();
+        initMetrics(id, metric, configRef);
+
+        args.remove(configRefKey);


maybe instead of args.op_aref() and than remove do a:
IRubyObject configRef = args.delete(context, configRefKey)
nitMetrics(id, metric, configRef.asJavaString());

NOTE: I am assuming the "config-ref" key is allways to be found in the map ...

I don't know, it could be that value is null, in that case if I'm not wrong it compared as "null" string

okay, let's not worry about that - we could still avoid doing a hash lookup twice
for these 3 lines, doing something like:

IRubyObject configRef = args.delete(context, configRefKey) nitMetrics(id, metric, configRef.asJavaString());

... do you feel its more confusing? (fine leaving it as is if you prefer)

sounds good

logstash-core/src/main/java/org/logstash/config/ir/compiler/AbstractOutputDelegatorExt.java

kares · 2019-12-04T13:08:17Z

logstash-core/src/main/java/org/logstash/config/ir/compiler/AbstractFilterDelegatorExt.java

@@ -39,7 +39,7 @@ public AbstractFilterDelegatorExt(final Ruby runtime, final RubyClass metaClass)
        super(runtime, metaClass);
    }

-    protected void initMetrics(final String id, final AbstractNamespacedMetricExt namespacedMetric) {
+    protected void initMetrics(final String id, final AbstractNamespacedMetricExt namespacedMetric, String codeRef) {


hmm, seems like you will always convert this arg to a RubyString why not pass down a RubyString ?

No in JavaOutputDeletegatorExt.create receive Java string created https://github.com/elastic/logstash/pull/11288/files/c347a1dd45728d08618ebf40f3b62f6fbfa15124#diff-f99a4d603d2230e54ef9d84b92f5d36dR239-R244

yes but this PR is in charge of that change, I meant:
why not change String configRef -> RubyString configRef ?
and than initMetrics could also receive a RubyString what we're doing here on a few places is :
get a RubyString convert to a String (using asJavaString()) and than back RubyString.newString(...)

(might be missing some parts here - please let me know if its not feasible to do so)

kares · 2019-12-04T13:11:27Z

logstash-core/src/main/java/org/logstash/config/ir/CompiledPipeline.java

            res.put(v.getId(), pluginFactory.buildOutput(
                    RubyUtil.RUBY.newString(def.getName()), RubyUtil.RUBY.newFixnum(source.getLine()),
-                    RubyUtil.RUBY.newFixnum(source.getColumn()), convertArgs(def), convertJavaArgs(def, cve)
+                    RubyUtil.RUBY.newFixnum(source.getColumn()), sourceFile, sourceLine, convertArgs(def), convertJavaArgs(def, cve)


isn't source.getLine() == sourceLine in which case we could be passing down less args?
maybe just Java args (we're changing the signature anyways) and do the Ruby wrapping in buildOutput
what do you think?

The idea to move string with the source, line and column already formatted into a more meaningful object was an improvement to propose later, maybe after the move of some code from Ruby to Java part (regarding the compilation of pipeline)

logstash-core/lib/logstash/compiler/lscl.rb

…emap global line numbers to source/line

…class that need it

…stance instead or more complicated invokation of

kares · 2019-12-09T09:14:35Z

logstash-core/src/main/java/org/logstash/config/ir/compiler/OutputStrategyExt.java

            initOutputCallsite(outputClass);
+
+            final RubyClass codecDelegatorClass = (RubyClass) RubyUtil.RUBY.executeScript(
+                    "require 'logstash/codecs/delegator'\nLogStash::Codecs::Delegator",


regarding this pattern - its fine in tests but ideally should not be used in production code.
... there's seems to be a pattern of keeping RubyClass instances under RubyUtil // this is preferred, right?
2nd option would be to have the require happen once (elsewhere) and than just retrieve the constant
e.g. RUBY.getClassFromPath("LogStash::Codecs::Delegator")

kares · 2019-12-09T09:19:13Z

logstash-core/lib/logstash/codecs/delegator.rb

@@ -1,7 +1,9 @@
 module LogStash::Codecs
  class Delegator < SimpleDelegator
-    def initialize(obj)
+    def initialize(obj, parent_plugin_name)


LS' Delegator seems to be gaining some more responsibilities ...
personally do not like it but do not want to block the PR due my preference
// someone please chime in e.g. @colinsurprenant was looking at this recently

kares · 2019-12-09T10:00:21Z

logstash-core/lib/logstash/pipeline.rb

@@ -70,7 +70,7 @@ def non_reloadable_plugins


  def plugin(plugin_type, name, line, column, *args)
-    @plugin_factory.plugin(plugin_type, name, line, column, *args)
+    @plugin_factory.plugin(plugin_type, name, line, column, "", -1, *args)


looks really ugly and it feels like these should be optional params e.g. PluginFactory signature would look like:
plugin(plugin_type, name, line, column, *args, **opts) (will still get a bit ugly at the Java ext end)

but than from the def plugin signature this method was meant to be simply delegating down the line ...

what I am not sure about, can we really change the underlying signature non backwards-compatibly?

jsvd · 2019-12-10T17:00:25Z

Hi @andsel, I haven't gone deeply into the code, but here are few remarks/questions:

the PR description should have more context about the change, to help the reviewer. maybe some summary of what changed in what areas, and major reasons for it. I know this is a spin off PR, but if someone is coming into it fresh they’ll get discouraged at seeing 40 ish files changed and no explanations.
the PR mentions java part and some ruby part too, could we narrow this down to just the minimum change to support this metadata for the java execution? Maybe a bit more explanation on what the “part” here means (i.e. “ruby part” means file/line support in ruby execution? or for ruby plugins?)
the plugin adds two new properties to the Plugin < Node class for the line and file, was there no way to leverage the existing SourceWithMetadata class for this? or was this change because of the ruby part?

andsel · 2019-12-11T09:58:06Z

@jsvd I've tried to describe better the PR, let me know if it's more meaningful and can help the PR reviwers

robbavey · 2019-12-11T15:33:39Z

@andsel Before I dive too deeply into this, I have a couple of questions, what is the current state of #11155? There is a lot of code shared between this PR and that one. I see this comment from @yaauie - did anything come out of that?

andsel · 2019-12-11T15:38:12Z

@robbavey this PR is composed of the code from last snapshot of #11155 where tried to organize the commits in a more understandable way (I hope) regarding only the common parts for Ruby and Java pipeline and the Java pipeline and changes that made the things tick for the Java pipeline itself.
Regarding the comment from @yaauie no actions was taken

andsel · 2020-01-15T10:54:59Z

Close for same reason of #11497 (comment)

andsel mentioned this pull request Oct 31, 2019

Added line and columns to plugin name #11155

Closed

andsel force-pushed the feature/exposing_config_ref_java_part branch from 9b15258 to 71728c9 Compare October 31, 2019 15:42

andsel force-pushed the feature/exposing_config_ref_java_part branch from 71728c9 to 818eab4 Compare November 21, 2019 16:13

andsel force-pushed the feature/exposing_config_ref_java_part branch from 818eab4 to 7d5b0c5 Compare November 27, 2019 12:46

elasticsearch-bot self-assigned this Nov 28, 2019

kares reviewed Nov 28, 2019

View reviewed changes

logstash-core/lib/logstash/config/pipeline_config.rb Outdated Show resolved Hide resolved

kares reviewed Nov 28, 2019

View reviewed changes

colinsurprenant unassigned elasticsearch-bot Nov 28, 2019

andsel requested a review from kares December 3, 2019 13:28

kares suggested changes Dec 4, 2019

View reviewed changes

andsel added 9 commits December 6, 2019 14:25

Added logic for absolute config reference remapping

9a46a8d

Changed Java pipeline config compiling and PipelineConfiguration to r…

f731217

…emap global line numbers to source/line

Exposed config reference for input plugins in Java pipeline

5e6df93

Exposed config reference for codec plugins in Java pipeline

850b402

Exposed config reference for filter plugins in Java pipeline

7c10b91

Exposed config reference for output plugins in Java pipeline

22ba7c5

Added QA test and fixed x-pack test

d145533

Removed the import of jar from Ruby's global scope to the one of the …

973ed9a

…class that need it

Fixed some suggestions from @kares relating to use of RubyClass.newIn…

dca60d5

…stance instead or more complicated invokation of

andsel force-pushed the feature/exposing_config_ref_java_part branch from c347a1d to dca60d5 Compare December 6, 2019 15:00

kares reviewed Dec 9, 2019

View reviewed changes

elasticsearch-bot self-assigned this Dec 10, 2019

jsvd unassigned elasticsearch-bot Dec 10, 2019

robbavey self-assigned this Dec 10, 2019

jsvd mentioned this pull request Dec 11, 2019

remove line+column passing. avoid one big sourceWithMetadata #11429

Closed

andsel mentioned this pull request Dec 19, 2019

Fix/refact use source with metadata instead of plain text #11456

Closed

andsel closed this Jan 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java pipeline part of PR #11155 #11288

Java pipeline part of PR #11155 #11288

andsel commented Oct 31, 2019 •

edited

Loading

andsel commented Nov 15, 2019

andsel commented Nov 22, 2019

andsel commented Nov 28, 2019

kares Nov 28, 2019

andsel Nov 28, 2019

andsel Nov 28, 2019

kares Nov 28, 2019

andsel commented Nov 28, 2019

kares Dec 4, 2019

andsel Dec 6, 2019

kares Dec 9, 2019

andsel Dec 9, 2019

kares Dec 4, 2019

andsel Dec 6, 2019

kares Dec 9, 2019 •

edited

Loading

kares Dec 9, 2019

kares Dec 4, 2019

andsel Dec 6, 2019

kares Dec 9, 2019

kares Dec 9, 2019

kares Dec 9, 2019

jsvd commented Dec 10, 2019

andsel commented Dec 11, 2019

robbavey commented Dec 11, 2019

andsel commented Dec 11, 2019

andsel commented Jan 15, 2020

Java pipeline part of PR #11155 #11288

Java pipeline part of PR #11155 #11288

Conversation

andsel commented Oct 31, 2019 • edited Loading

andsel commented Nov 15, 2019

andsel commented Nov 22, 2019

andsel commented Nov 28, 2019

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 Nov 28, 2019

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

Choose a reason for hiding this comment

kares Dec 9, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsvd commented Dec 10, 2019

andsel commented Dec 11, 2019

robbavey commented Dec 11, 2019

andsel commented Dec 11, 2019

andsel commented Jan 15, 2020

andsel commented Oct 31, 2019 •

edited

Loading

kares Dec 9, 2019 •

edited

Loading