Convert label values to strings in base Metric class

Currently, you get an inconsistent experience when using different store
implementations (e.g. `SingleThreaded` vs `DirectFileStore`).

Because `DirectFileStore` has to write your metrics into a file, it
has to convert label values into strings. That means that when you
access them they're strings. Other stores don't do this, so you can
access them as symbols if you provided them as symbols.

To give users a consistent experience when swapping between stores, this
commit changes the Metric base class to convert the label values to
strings before they get anywhere near the underlying store.

Signed-off-by: Chris Sinjakli <chris@gocardless.com>

Author: Chris Sinjakli

lib/prometheus/client/metric.rb

@@ -27,7 +27,7 @@ def initialize(name,
 
         @name = name
         @docstring = docstring
-        @preset_labels = preset_labels
+        @preset_labels = stringify_values(preset_labels)
 
         @store = Prometheus::Client.config.data_store.for_metric(
           name,
@@ -85,8 +85,13 @@ def validate_docstring(docstring)
       def label_set_for(labels)
         # We've already validated, and there's nothing to merge. Save some cycles
         return preset_labels if @all_labels_preset && labels.empty?
+        labels = stringify_values(labels)
         @validator.validate_labelset!(preset_labels.merge(labels))
       end
+
+      def stringify_values(labels)
+        labels.map { |k,v| [k, v.to_s] }.to_h
+      end
     end
   end
 end

spec/examples/metric_example.rb

@@ -1,5 +1,31 @@
 # encoding: UTF-8
 
+# TODO: Convert these tests to use a fake metric class rather than shared examples
+#
+# Right now, we're using shared examples that we include in every metric type's tests
+# to validate the behaviour of the base metric class.
+#
+# This makes it difficult to test certain behaviour, as the interfaces of those metric
+# types differ and these tests can end up needing to know about them.
+#
+# You can see that in the tests for #get, which depend on `type` which isn't defined in
+# this file. The test files that include these shared examples have to do so with a block
+# that provides the `type` variable.
+#
+# This cropped up in a much worse way when trying to test the code that makes sure label
+# values are all strings. Writing a test here that gets included in all the real metric
+# implementations is near impossible. You need your test to call a different method to
+# alter a metric value (e.g. `set`, `increment` or `observe` depending on the metric type)
+# which means having each concrete metric type's tests passing us a lambda that we can
+# call agnostically of the metric type.
+#
+# The resultant code is confusing to follow, so we opted to duplicate those tests in each
+# metric type's test file.
+#
+# Changing this file to implement a fake metric class (e.g. `FakeTestCounter`) would let
+# us easily test the functionality of the base `Prometheus::Client::Metric` without
+# getting caught up in the specifics of the real metric types.
+
 shared_examples_for Prometheus::Client::Metric do
   subject { described_class.new(:foo, docstring: 'foo description') }
 

spec/prometheus/client/counter_spec.rb

@@ -78,5 +78,28 @@
         end.each(&:join)
       end.to change { counter.get }.by(100.0)
     end
+
+    context "with non-string label values" do
+      subject { described_class.new(:foo, docstring: 'Labels', labels: [:foo]) }
+
+      it "converts labels to strings for consistent storage" do
+        subject.increment(labels: { foo: :label })
+        expect(subject.get(labels: { foo: 'label' })).to eq(1.0)
+      end
+
+      context "and some labels preset" do
+        subject do
+          described_class.new(:foo,
+                              docstring: 'Labels',
+                              labels: [:foo, :bar],
+                              preset_labels: { foo: :label })
+        end
+
+        it "converts labels to strings for consistent storage" do
+          subject.increment(labels: { bar: :label })
+          expect(subject.get(labels: { foo: 'label', bar: 'label' })).to eq(1.0)
+        end
+      end
+    end
   end
 end

spec/prometheus/client/gauge_spec.rb

@@ -110,6 +110,29 @@
         end.each(&:join)
       end.to change { gauge.get }.by(100.0)
     end
+
+    context "with non-string label values" do
+      subject { described_class.new(:foo, docstring: 'Labels', labels: [:foo]) }
+
+      it "converts labels to strings for consistent storage" do
+        subject.increment(labels: { foo: :label })
+        expect(subject.get(labels: { foo: 'label' })).to eq(1.0)
+      end
+
+      context "and some labels preset" do
+        subject do
+          described_class.new(:foo,
+                              docstring: 'Labels',
+                              labels: [:foo, :bar],
+                              preset_labels: { foo: :label })
+        end
+
+        it "converts labels to strings for consistent storage" do
+          subject.increment(labels: { bar: :label })
+          expect(subject.get(labels: { foo: 'label', bar: 'label' })).to eq(1.0)
+        end
+      end
+    end
   end
 
   describe '#decrement' do

spec/prometheus/client/histogram_spec.rb

@@ -73,6 +73,35 @@
         expect { histogram.with_labels(test: 'value').observe(2) }.not_to raise_error
       end
     end
+
+    context "with non-string label values" do
+      let(:histogram) do
+        described_class.new(:foo,
+                            docstring: 'foo description',
+                            labels: [:foo],
+                            buckets: [2.5, 5, 10])
+      end
+
+      it "converts labels to strings for consistent storage" do
+        histogram.observe(5, labels: { foo: :label })
+        expect(histogram.get(labels: { foo: 'label' })["10"]).to eq(1.0)
+      end
+
+      context "and some labels preset" do
+        let(:histogram) do
+          described_class.new(:foo,
+                              docstring: 'foo description',
+                              labels: [:foo, :bar],
+                              preset_labels: { foo: :label },
+                              buckets: [2.5, 5, 10])
+        end
+
+        it "converts labels to strings for consistent storage" do
+          histogram.observe(5, labels: { bar: :label })
+          expect(histogram.get(labels: { foo: 'label', bar: 'label' })["10"]).to eq(1.0)
+        end
+      end
+    end
   end
 
   describe '#get' do

spec/prometheus/client/summary_spec.rb

@@ -68,6 +68,33 @@
         expect { summary.with_labels(test: 'value').observe(2) }.not_to raise_error
       end
     end
+
+    context "with non-string label values" do
+      let(:summary) do
+        described_class.new(:foo,
+                            docstring: 'foo description',
+                            labels: [:foo])
+      end
+
+      it "converts labels to strings for consistent storage" do
+        summary.observe(5, labels: { foo: :label })
+        expect(summary.get(labels: { foo: 'label' })["count"]).to eq(1.0)
+      end
+
+      context "and some labels preset" do
+        let(:summary) do
+          described_class.new(:foo,
+                              docstring: 'foo description',
+                              labels: [:foo, :bar],
+                              preset_labels: { foo: :label })
+        end
+
+        it "converts labels to strings for consistent storage" do
+          summary.observe(5, labels: { bar: :label })
+          expect(summary.get(labels: { foo: 'label', bar: 'label' })["count"]).to eq(1.0)
+        end
+      end
+    end
   end
 
   describe '#get' do