Add more testing around the NullMetric implementation

The current test where only checking if the `NullMetric` Interface was
matching the original metric interface, this PR add more test around the
actual methods and fix an issue with `decrement` using too much
arguments in the context of a namespaced metric.

The test were also move into a shared example to be used in both the `NullMetric`
and the `NamedspacedMetric`.

Also the NullMetric class will no use the same validation for keys as the namespaced class and will
reject empty or nil keys.

Fixes #5545

logstash-core/lib/logstash/instrument/metric.rb

@@ -18,22 +18,22 @@ module LogStash module Instrument
     end
 
     def increment(namespace, key, value = 1)
-      validate_key!(key)
+      self.class.validate_key!(key)
       collector.push(namespace, key, :counter, :increment, value)
     end
 
     def decrement(namespace, key, value = 1)
-      validate_key!(key)
+      self.class.validate_key!(key)
       collector.push(namespace, key, :counter, :decrement, value)
     end
 
     def gauge(namespace, key, value)
-      validate_key!(key)
+      self.class.validate_key!(key)
       collector.push(namespace, key, :gauge, :set, value)
     end
 
     def time(namespace, key)
-      validate_key!(key)
+      self.class.validate_key!(key)
 
       if block_given?
         timer = TimedExecution.new(self, namespace, key)
@@ -46,6 +46,7 @@ module LogStash module Instrument
     end
 
     def report_time(namespace, key, duration)
+      self.class.validate_key!(key)
       collector.push(namespace, key, :counter, :increment, duration)
     end
 
@@ -69,11 +70,11 @@ module LogStash module Instrument
       NamespacedMetric.new(self, name)
     end
 
-    private
+    def self.validate_key!(key)
-    def validate_key!(key)
       raise MetricNoKeyProvided if key.nil? || key.empty?
     end
 
+    private
     # Allow to calculate the execution of a block of code.
     # This class support 2 differents syntax a block or the return of
     # the object itself, but in the later case the metric wont be recorded

logstash-core/lib/logstash/instrument/namespaced_metric.rb

@@ -24,7 +24,7 @@ module LogStash module Instrument
       @metric.increment(namespace_name, key, value)
     end
 
-    def decrement(namespace, key, value = 1)
+    def decrement(key, value = 1)
       @metric.decrement(namespace_name, key, value)
     end
 

logstash-core/lib/logstash/instrument/null_metric.rb

@@ -9,20 +9,25 @@ module LogStash module Instrument
    attr_reader :namespace_name, :collector
 
    def increment(key, value = 1)
+     Metric.validate_key!(key)
    end
 
-   def decrement(namespace, key, value = 1)
+   def decrement(key, value = 1)
+     Metric.validate_key!(key)
    end
 
    def gauge(key, value)
+     Metric.validate_key!(key)
    end
 
    def report_time(key, duration)
+     Metric.validate_key!(key)
    end
 
    # We have to manually redefine this method since it can return an
    # object this object also has to be implemented as a NullObject
    def time(key)
+     Metric.validate_key!(key)
      if block_given?
        yield
      else

logstash-core/spec/logstash/instrument/namespaced_metric_spec.rb

@@ -2,10 +2,11 @@
 require "logstash/instrument/namespaced_metric"
 require "logstash/instrument/metric"
 require_relative "../../support/matchers"
+require_relative "../../support/shared_examples"
 require "spec_helper"
 
 describe LogStash::Instrument::NamespacedMetric do
-  let(:namespace) { :stats }
+  let(:namespace) { :root }
   let(:collector) { [] }
   let(:metric) { LogStash::Instrument::Metric.new(collector) }
 
@@ -27,6 +28,64 @@ describe LogStash::Instrument::NamespacedMetric do
     new_namespace = subject.namespace(:wally)
 
     expect(subject.namespace_name).to eq([namespace])
-    expect(new_namespace.namespace_name).to eq([:stats, :wally])
+    expect(new_namespace.namespace_name).to eq([:root, :wally])
   end
+
+  context "#increment" do
+    it "a counter by 1" do
+      metric = subject.increment(:error_rate)
+      expect(collector).to be_a_metric_event([:root, :error_rate], :counter, :increment, 1)
+    end
+
+    it "a counter by a provided value" do
+      metric = subject.increment(:error_rate, 20)
+      expect(collector).to be_a_metric_event([:root, :error_rate], :counter, :increment, 20)
+    end
+  end
+
+  context "#decrement" do
+    it "a counter by 1" do
+      metric = subject.decrement(:error_rate)
+      expect(collector).to be_a_metric_event([:root, :error_rate], :counter, :decrement, 1)
+    end
+
+    it "a counter by a provided value" do
+      metric = subject.decrement(:error_rate, 20)
+      expect(collector).to be_a_metric_event([:root, :error_rate], :counter, :decrement, 20)
+    end
+  end
+
+  context "#gauge" do
+    it "set the value of a key" do
+      metric = subject.gauge(:size_queue, 20)
+      expect(collector).to be_a_metric_event([:root, :size_queue], :gauge, :set, 20)
+    end
+  end
+
+  context "#time" do
+    let(:sleep_time) { 2 }
+    let(:sleep_time_ms) { sleep_time * 1_000 }
+
+    it "records the duration" do
+      subject.time(:duration_ms) { sleep(sleep_time) }
+
+      expect(collector.last).to be_within(sleep_time_ms).of(sleep_time_ms + 5)
+      expect(collector[0]).to match([:root])
+      expect(collector[1]).to be(:duration_ms)
+      expect(collector[2]).to be(:counter)
+    end
+
+    it "return a TimedExecution" do
+      execution = subject.time(:duration_ms)
+      sleep(sleep_time)
+      execution.stop
+
+      expect(collector.last).to be_within(sleep_time_ms).of(sleep_time_ms + 0.1)
+      expect(collector[0]).to match([:root])
+      expect(collector[1]).to be(:duration_ms)
+      expect(collector[2]).to be(:counter)
+    end
+  end
+
+  include_examples "metrics commons operations"
 end

logstash-core/spec/logstash/instrument/null_metric_spec.rb

@@ -1,21 +1,19 @@
 # encoding: utf-8
 require "logstash/instrument/null_metric"
 require "logstash/instrument/namespaced_metric"
-require_relative "../../support/matchers"
+require_relative "../../support/shared_examples"
 
 describe LogStash::Instrument::NullMetric do
+  # This is defined in the `namespaced_metric_spec`
+  include_examples "metrics commons operations"
+
   it "defines the same interface as `Metric`" do
     expect(described_class).to implement_interface_of(LogStash::Instrument::NamespacedMetric)
   end
 
-  describe "#time" do
+  describe "#namespace" do
-    it "returns the value of the block without recording any metrics" do
+    it "return a NullMetric" do
-      expect(subject.time(:execution_time) { "hello" }).to eq("hello")
+      expect(subject.namespace(key)).to be_kind_of LogStash::Instrument::NullMetric
-    end
-
-    it "return a TimedExecution" do
-      execution = subject.time(:do_something)
-      expect { execution.stop }.not_to raise_error
     end
   end
 end

logstash-core/spec/support/shared_examples.rb

@@ -0,0 +1,98 @@
+# encoding: utf-8
+# Define the common operation that both the `NullMetric` class and the Namespaced class should answer.
+shared_examples "metrics commons operations" do
+  let(:key) { "galaxy" }
+
+  describe "#increment" do
+    it "allows to increment a key with no amount" do
+      expect { subject.increment(key, 100) }.not_to raise_error
+    end
+
+    it "allow to increment a key" do
+      expect { subject.increment(key) }.not_to raise_error
+    end
+
+    it "raises an exception if the key is an empty string" do
+      expect { subject.increment("", 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+
+    it "raise an exception if the key is nil" do
+      expect { subject.increment(nil, 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+  end
+
+  describe "#decrement" do
+    it "allows to decrement a key with no amount" do
+      expect { subject.decrement(key, 100) }.not_to raise_error
+    end
+
+    it "allow to decrement a key" do
+      expect { subject.decrement(key) }.not_to raise_error
+    end
+
+    it "raises an exception if the key is an empty string" do
+      expect { subject.decrement("", 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+
+    it "raise an exception if the key is nil" do
+      expect { subject.decrement(nil, 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+  end
+
+  describe "#gauge" do
+    it "allows to set a value" do
+      expect { subject.gauge(key, "pluto") }.not_to raise_error
+    end
+
+
+    it "raises an exception if the key is an empty string" do
+      expect { subject.gauge("", 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+
+    it "raise an exception if the key is nil" do
+      expect { subject.gauge(nil, 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+  end
+
+  describe "#report_time" do
+    it "allow to record time" do
+      expect { subject.report_time(key, 1000) }.not_to raise_error
+    end
+
+    it "raises an exception if the key is an empty string" do
+      expect { subject.report_time("", 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+
+    it "raise an exception if the key is nil" do
+      expect { subject.report_time(nil, 20) }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+  end
+
+  describe "#time" do
+    it "allow to record time with a block given" do
+      expect do
+        subject.time(key) { 1+1 }
+      end.not_to raise_error
+    end
+
+    it "returns the value of the block without recording any metrics" do
+      expect(subject.time(:execution_time) { "hello" }).to eq("hello")
+    end
+
+    it "return a TimedExecution" do
+      execution = subject.time(:do_something)
+      expect { execution.stop }.not_to raise_error
+    end
+
+
+    it "raises an exception if the key is an empty string" do
+      expect { subject.time("") {} }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+
+    it "raise an exception if the key is nil" do
+      expect { subject.time(nil) {} }.to raise_error(LogStash::Instrument::MetricNoKeyProvided)
+    end
+  end
+end
+
+