Merge 80adc30 into b3bbe23

lib/prometheus/client/metric.rb

@@ -1,6 +1,7 @@
 # encoding: UTF-8
 
 require 'thread'
+require 'prometheus/client'
 require 'prometheus/client/label_set_validator'
 
 module Prometheus

lib/prometheus/client/registry.rb

@@ -1,11 +1,10 @@
 # encoding: UTF-8
 
-require 'thread'
-
 require 'prometheus/client/counter'
 require 'prometheus/client/summary'
 require 'prometheus/client/gauge'
 require 'prometheus/client/histogram'
+require 'concurrent'
 
 module Prometheus
   module Client
@@ -15,24 +14,23 @@ class AlreadyRegisteredError < StandardError; end
 
       def initialize
         @metrics = {}
-        @mutex = Mutex.new
+        @lock    = Concurrent::ReentrantReadWriteLock.new
       end
 
       def register(metric)
         name = metric.name
 
-        @mutex.synchronize do
-          if exist?(name.to_sym)
-            raise AlreadyRegisteredError, "#{name} has already been registered"
-          end
+        @lock.with_write_lock do
+          return metric if exist?(name.to_sym)
+
           @metrics[name.to_sym] = metric
         end
 
         metric
       end
 
       def unregister(name)
-        @mutex.synchronize do
+        @lock.with_write_lock do
           @metrics.delete(name.to_sym)
         end
       end
@@ -73,15 +71,21 @@ def histogram(name, docstring:, labels: [], preset_labels: {},
       end
 
       def exist?(name)
-        @metrics.key?(name)
+        @lock.with_read_lock do
+          @metrics.key?(name)
+        end
       end
 
       def get(name)
-        @metrics[name.to_sym]
+        @lock.with_read_lock do
+          @metrics[name.to_sym]
+        end
       end
 
       def metrics
-        @metrics.values
+        @lock.with_read_lock do
+          @metrics.values
+        end
       end
     end
   end

lib/prometheus/middleware/collector.rb

@@ -44,22 +44,27 @@ def call(env) # :nodoc:
       protected
 
       def init_request_metrics
-        @requests = @registry.counter(
-          :"#{@metrics_prefix}_requests_total",
+        counter_name    = "#{@metrics_prefix}_requests_total"
+        histogram_name  = "#{@metrics_prefix}_request_duration_seconds"
+
+        @requests = @registry.get(counter_name) || @registry.counter(
+          counter_name.to_sym,
           docstring:
             'The total number of HTTP requests handled by the Rack application.',
           labels: %i[code method path]
         )
-        @durations = @registry.histogram(
-          :"#{@metrics_prefix}_request_duration_seconds",
+        @durations = @registry.get(histogram_name) || @registry.histogram(
+          histogram_name.to_sym,
           docstring: 'The HTTP response duration of the Rack application.',
           labels: %i[method path]
         )
       end
 
       def init_exception_metrics
-        @exceptions = @registry.counter(
-          :"#{@metrics_prefix}_exceptions_total",
+        exception_counter_name = "#{@metrics_prefix}_exceptions_total"
+
+        @exceptions = @registry.get(exception_counter_name) || @registry.counter(
+          exception_counter_name.to_sym,
           docstring: 'The total number of exceptions raised by the Rack application.',
           labels: [:exception]
         )

spec/prometheus/client/registry_spec.rb

@@ -19,36 +19,26 @@
       expect(registry.register(metric)).to eql(metric)
     end
 
-    it 'raises an exception if a metric name gets registered twice' do
+    it 'returns a metric if a metric name tries to register twice' do
       metric = double(name: :test)
 
       registry.register(metric)
 
-      expect do
-        registry.register(metric)
-      end.to raise_exception described_class::AlreadyRegisteredError
+      expect(registry.metrics.size).to eq 1
     end
 
     it 'is thread safe' do
-      mutex = Mutex.new
-      containers = []
-
       def registry.exist?(*args)
         super.tap { sleep(0.01) }
       end
 
       Array.new(5) do
         Thread.new do
-          result = begin
-            registry.register(double(name: :test))
-          rescue Prometheus::Client::Registry::AlreadyRegisteredError
-            nil
-          end
-          mutex.synchronize { containers << result }
+          registry.register(double(name: :test))
         end
       end.each(&:join)
 
-      expect(containers.compact.size).to eql(1)
+      expect(registry.metrics.size).to eql(1)
     end
   end
 

spec/prometheus/integration_spec.rb

@@ -0,0 +1,30 @@
+# encoding: UTF-8
+
+require 'rack/test'
+require 'rack'
+require 'prometheus/middleware/collector'
+require 'prometheus/middleware/exporter'
+
+API = Rack::Builder.new do
+  use Rack::Deflater
+  use Prometheus::Middleware::Collector
+  use Prometheus::Middleware::Exporter
+
+  map "/" do
+    run ->(_) { [200, {'Content-Type' => 'text/html'}, ['OK']] }
+  end
+end
+
+describe API do
+  include Rack::Test::Methods
+
+  let(:app) { described_class }
+
+  context 'GET /metrics' do
+    it 'fails on the second request' do
+      get '/metrics'
+      expect { last_response }.not_to raise_error
+      expect { get '/metrics' }.not_to raise_error
+    end
+  end
+end

spec/prometheus/threaded_integration_spec.rb

@@ -0,0 +1,52 @@
+# encoding: UTF-8
+
+require 'rack/test'
+require 'rack'
+require 'prometheus/middleware/collector'
+require 'prometheus/middleware/exporter'
+require "concurrent"
+
+API = Rack::Builder.new do
+  use Rack::Deflater
+  use Prometheus::Middleware::Collector
+  use Prometheus::Middleware::Exporter
+
+  map "/" do
+    run ->(_) { [200, {'Content-Type' => 'text/html'}, ['OK']] }
+  end
+end
+
+describe API do
+  include Rack::Test::Methods
+
+  let(:app) { described_class }
+
+  context 'GET /metrics' do
+    it "fails when it's multi threaded request" do
+      latch = Concurrent::CountDownLatch.new(1)
+
+      t1 = Thread.new do
+        latch.wait
+        get '/metrics'
+      end
+
+      t2 = Thread.new do
+        latch.wait
+
+        get '/metrics'
+      end
+
+      t3 = Thread.new do
+        latch.wait
+
+        get '/metrics'
+      end
+
+      latch.count_down
+
+      [t1, t2, t3].each(&:join)
+
+      expect { last_response }.not_to raise_error
+    end
+  end
+end