From 1a780ab1472032ccad2c2ca60e237657d4fe0431 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Tue, 19 Jan 2016 14:21:06 -0500 Subject: [PATCH] Add a `Plugin#id` that return an ID for a plugin This method return an ID for the plugins and be configured by the users in the configuration like this: ``` elasticsearch { id => "ABC" ... } ``` This information will be used when collecting metrics for a specific plugin. Allowing the user to change it allow to stick between restart. Fixes #3892 Fixes #4525 --- .../lib/logstash/filter_delegator.rb | 2 +- .../lib/logstash/output_delegator.rb | 6 +- logstash-core/lib/logstash/plugin.rb | 39 +++++++------ .../spec/logstash/output_delegator_spec.rb | 6 +- logstash-core/spec/logstash/plugin_spec.rb | 56 +++++++++++++++++++ 5 files changed, 86 insertions(+), 23 deletions(-) diff --git a/logstash-core/lib/logstash/filter_delegator.rb b/logstash-core/lib/logstash/filter_delegator.rb index ac07499da0f..ab6c3afa0bd 100644 --- a/logstash-core/lib/logstash/filter_delegator.rb +++ b/logstash-core/lib/logstash/filter_delegator.rb @@ -20,7 +20,7 @@ def initialize(logger, klass, metric, *args) @filter = klass.new(options) # Scope the metrics to the plugin - @metric = metric.namespace(@filter.identifier_name) + @metric = metric.namespace(@filter.id.to_sym) @filter.metric = @metric define_flush_method if @filter.respond_to?(:flush) diff --git a/logstash-core/lib/logstash/output_delegator.rb b/logstash-core/lib/logstash/output_delegator.rb index 9ad00e78389..7ac962dfeb7 100644 --- a/logstash-core/lib/logstash/output_delegator.rb +++ b/logstash-core/lib/logstash/output_delegator.rb @@ -17,12 +17,12 @@ def initialize(logger, klass, default_worker_count, metric, *args) @threadsafe = klass.threadsafe? @config = args.reduce({}, :merge) @klass = klass - + # Create an instance of the input so we can fetch the identifier output = @klass.new(*args) - + # Scope the metrics to the plugin - @metric = metric.namespace(output.identifier_name) + @metric = metric.namespace(output.id.to_sym) # We define this as an array regardless of threadsafety # to make reporting simpler, even though a threadsafe plugin will just have diff --git a/logstash-core/lib/logstash/plugin.rb b/logstash-core/lib/logstash/plugin.rb index fa5f78148cd..b62eaefc387 100644 --- a/logstash-core/lib/logstash/plugin.rb +++ b/logstash-core/lib/logstash/plugin.rb @@ -5,7 +5,7 @@ require "logstash/instrument/null_metric" require "cabin" require "concurrent" -require "digest/md5" +require "securerandom" class LogStash::Plugin @@ -21,13 +21,19 @@ class LogStash::Plugin # for a specific plugin. config :enable_metric, :validate => :boolean, :default => true - # Under which name you want to collect metric for this plugin? - # This will allow you to compare the performance of the configuration change, this - # name need to be unique per plugin configuration. + # Add a unique `ID` to the plugin instance, this `ID` is used for tracking + # information for a specific configuration of the plugin. + # + # ``` + # output { + # stdout { + # id => "ABC" + # } + # } + # ``` # # If you don't explicitely set this variable Logstash will generate a unique name. - # This name will be valid until the configuration change. - config :metric_identifier, :validate => :string, :default => "" + config :id, :validate => :string, :default => "" public def hash @@ -46,6 +52,16 @@ def initialize(params=nil) @logger = Cabin::Channel.get(LogStash) end + # Return a uniq ID for this plugin configuration, by default + # we will generate a UUID + # + # If the user defines a `id => 'ABC'` in the configuration we will return + # + # @return [String] A plugin ID + def id + (@params["id"].nil? || @params["id"].empty?) ? SecureRandom.uuid : @params["id"] + end + # close is called during shutdown, after the plugin worker # main task terminates public @@ -83,21 +99,13 @@ def debug_info end def metric=(new_metric) - @metric = new_metric.namespace(identifier_name) + @metric = new_metric.namespace(@id) end def metric @metric_plugin ||= enable_metric ? @metric : LogStash::Instrument::NullMetric.new end - def identifier_name - @identifier_name ||= (@metric_identifier.nil? || @metric_identifier.empty?) ? "#{self.class.config_name}-#{params_hash_code}".to_sym : @identifier.to_sym - end - - def params_hash_code - Digest::MD5.hexdigest(params.to_s) - end - # Look up a plugin by type and name. public def self.lookup(type, name) @@ -120,7 +128,6 @@ def self.lookup(type, name) end private - # lookup a plugin by type and name in the existing LogStash module namespace # ex.: namespace_lookup("filter", "grok") looks for LogStash::Filters::Grok # @param type [String] plugin type, "input", "ouput", "filter" diff --git a/logstash-core/spec/logstash/output_delegator_spec.rb b/logstash-core/spec/logstash/output_delegator_spec.rb index 5d7b6186307..a4fe2a8a253 100644 --- a/logstash-core/spec/logstash/output_delegator_spec.rb +++ b/logstash-core/spec/logstash/output_delegator_spec.rb @@ -20,7 +20,7 @@ allow(out_inst).to receive(:register) allow(out_inst).to receive(:multi_receive) allow(out_inst).to receive(:metric=).with(any_args) - allow(out_inst).to receive(:identifier_name).and_return("a-simple-plugin") + allow(out_inst).to receive(:id).and_return("a-simple-plugin") allow(logger).to receive(:debug).with(any_args) end @@ -60,7 +60,7 @@ allow(out_klass).to receive(:threadsafe?).and_return(false) allow(out_klass).to receive(:workers_not_supported?).and_return(false) allow(out_inst).to receive(:metric=).with(any_args) - allow(out_inst).to receive(:identifier_name).and_return("a-simple-plugin") + allow(out_inst).to receive(:id).and_return("a-simple-plugin") end it "should instantiate multiple workers" do @@ -77,7 +77,7 @@ before do allow(out_klass).to receive(:threadsafe?).and_return(true) allow(out_inst).to receive(:metric=).with(any_args) - allow(out_inst).to receive(:identifier_name).and_return("a-simple-plugin") + allow(out_inst).to receive(:id).and_return("a-simple-plugin") allow(out_klass).to receive(:workers_not_supported?).and_return(false) end diff --git a/logstash-core/spec/logstash/plugin_spec.rb b/logstash-core/spec/logstash/plugin_spec.rb index 781a57aefe0..b6d72e803bb 100644 --- a/logstash-core/spec/logstash/plugin_spec.rb +++ b/logstash-core/spec/logstash/plugin_spec.rb @@ -1,6 +1,10 @@ # encoding: utf-8 require "spec_helper" require "logstash/plugin" +require "logstash/outputs/base" +require "logstash/codecs/base" +require "logstash/inputs/base" +require "logstash/filters/base" describe LogStash::Plugin do it "should fail lookup on inexisting type" do @@ -149,4 +153,56 @@ class LogStash::Filters::Stromae < LogStash::Filters::Base end end end + + describe "#id" do + plugin_types = [ + LogStash::Filters::Base, + LogStash::Codecs::Base, + LogStash::Outputs::Base, + LogStash::Inputs::Base + ] + + plugin_types.each do |plugin_type| + let(:plugin) do + Class.new(plugin_type) do + config_name "simple_plugin" + + config :host, :validate => :string + config :export, :validte => :boolean + + def register; end + end + end + + let(:config) do + { + "host" => "127.0.0.1", + "export" => true + } + end + + subject { plugin.new(config) } + + context "plugin type is #{plugin_type}" do + context "when there is not ID configured for the output" do + it "it uses a UUID to identify this plugins" do + expect(subject.id).not_to eq(nil) + end + + it "will be different between instance of plugins" do + expect(subject.id).not_to eq(plugin.new(config).id) + end + end + + context "When a user provide an ID for the plugin" do + let(:id) { "ABC" } + let(:config) { super.merge("id" => id) } + + it "uses the user provided ID" do + expect(subject.id).to eq(id) + end + end + end + end + end end