Logstash: command line -p broken

[colinsurprenant]

when launching logstash using -p option to specify an alternate path.plugins it crashes. I noticed this on master, not sure how far back this has been broken.

The workaround is to use the path.plugins: option in logstash.yml.

$ bin/logstash -p tmp/ -f tmp/syslog.conf
NameError: `@path.plugins' is not allowable as an instance variable name
         instance_variable_get at org/jruby/RubyKernel.java:2023
                           get at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/attribute/instance.rb:21
                       _append at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/attribute/instance.rb:41
  block in define_appender_for at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/attribute/declaration.rb:41
                          take at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/attribute/instance.rb:57
                 parse_options at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/option/parsing.rb:32
                         parse at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/command.rb:52
                           run at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/command.rb:66
                           run at /Users/colin/dev/src/elasticsearch/logstash/logstash-core/lib/logstash/runner.rb:214
                           run at /Users/colin/dev/src/elasticsearch/logstash/vendor/bundle/jruby/2.3.0/gems/clamp-0.6.5/lib/clamp/command.rb:132
                        <main> at /Users/colin/dev/src/elasticsearch/logstash/lib/bootstrap/environment.rb:67

[yaauie]

I was able to replicate on 5.6.4

[tsg]

Just to link the issues together:

• The issue was reported before: https://github.com/elastic/logstash/issues/6074
• And here: https://github.com/elastic/logstash/issues/7267
• And a fix was proposed: https://github.com/elastic/logstash/pull/7276
• There was a decision, however, (in https://github.com/elastic/logstash/issues/7267#issuecomment-305243978) to deprecate the feature, and a ticket was created for it: https://github.com/elastic/logstash/issues/6200
• There is even a PR to deprecate it, but seems that it got lost at some point https://github.com/elastic/logstash/pull/6439 and didn't happen in 6.0.

We should discuss if removing the flag is still what we want, and if yes do it and close these threads.

[colinsurprenant]

@tsg thanks for the investigative work on this! 👁

Personally I find that option useful, it allows creating a local plugin without requiring the gem boilerplate. Clearly we should fix it or remove it but since (I believe) it was never marked as deprecated we should probably at least fix the issue now and if we (re)decide to remove that option then initiate the deprecation notice on it.

[jsvd]

@tsg to add a bit more confusion, PR https://github.com/elastic/logstash/pull/8887 was an attempt to fix the issue, but it breaks another part of the setting handling because of the way we use the cli option parser library (clamp).

overall I think we should deprecate this feature (the use case is now handled by file base ruby scripts in the ruby filter).
I also agree that since the feature is there until 6.x it should work, we just haven't put enough effort to fix it.

[colinsurprenant]

overall I think we should deprecate this feature (the use case is now handled by file base ruby scripts in the ruby filter).

Yes and no - Ruby filter only addresses the filters use-cases, not the inputs and outputs.

I'd like to understand what the problem is with keeping this feature around? (other than fixing the issue, which we agree we need to do anyway) - I will re-read the linked issues to see why we "decided" to remove/deprecate it ... I am a bit confused.

[jsvd]

the fix in https://github.com/elastic/logstash/pull/8887 was merged to master and 6.x
@andrewvc should we also include this in 6.3/6.2/5.9?