Fixing the Registry threading/design issues

lib/prometheus/client.rb

@@ -8,7 +8,7 @@ module Prometheus
   module Client
     # Returns a default registry object
     def self.registry
-      @registry ||= Registry.new
+      Registry.instance
     end
 
     def self.config

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,38 +1,39 @@
 # encoding: UTF-8
 
-require 'thread'
-
 require 'prometheus/client/counter'
 require 'prometheus/client/summary'
 require 'prometheus/client/gauge'
 require 'prometheus/client/histogram'
+require 'concurrent'
+require 'singleton'
 
 module Prometheus
   module Client
     # Registry
     class Registry
       class AlreadyRegisteredError < StandardError; end
 
+      include Singleton
+
       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 +74,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/formats/text_spec.rb

@@ -10,7 +10,7 @@
     Prometheus::Client.config.data_store = Prometheus::Client::DataStores::Synchronized.new
   end
 
-  let(:registry) { Prometheus::Client::Registry.new }
+  let(:registry) { Prometheus::Client::Registry.instance }
 
   before do
     foo = registry.counter(:foo,

spec/prometheus/client/registry_spec.rb

@@ -4,7 +4,7 @@
 require 'prometheus/client/registry'
 
 describe Prometheus::Client::Registry do
-  let(:registry) { Prometheus::Client::Registry.new }
+  let(:registry) { Prometheus::Client::Registry.instance }
 
   describe '.new' do
     it 'returns a new registry instance' do
@@ -13,42 +13,34 @@
   end
 
   describe '#register' do
+    let(:registry) { Prometheus::Client::Registry.clone.instance }
+
     it 'registers a new metric container and returns it' do
       metric = double(name: :test)
 
       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
 
@@ -99,6 +91,8 @@ def registry.exist?(*args)
   end
 
   describe '#exist?' do
+    let(:registry) { Prometheus::Client::Registry.clone.instance }
+
     it 'returns true if a metric name has been registered' do
       registry.register(double(name: :test))
 
@@ -111,6 +105,8 @@ def registry.exist?(*args)
   end
 
   describe '#get' do
+    let(:registry) { Prometheus::Client::Registry.clone.instance }
+
     it 'returns a previously registered metric container' do
       registry.register(double(name: :test))
 

spec/prometheus/integration_spec.rb

@@ -0,0 +1,22 @@
+# encoding: UTF-8
+
+require 'rack/test'
+require 'rack'
+require 'prometheus/middleware/collector'
+require 'prometheus/middleware/exporter'
+
+require_relative "../support/api"
+
+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/middleware/collector_spec.rb

@@ -12,7 +12,7 @@
   end
 
   let(:registry) do
-    Prometheus::Client::Registry.new
+    Prometheus::Client::Registry.instance
   end
 
   let(:original_app) do
@@ -41,18 +41,22 @@
     expect(last_response).to be_ok
   end
 
-  it 'traces request information' do
-    expect(Benchmark).to receive(:realtime).and_yield.and_return(0.2)
+  context 'trace' do
+    let(:registry) { Prometheus::Client::Registry.clone.instance }
 
-    get '/foo'
+    it 'traces request information' do
+      expect(Benchmark).to receive(:realtime).and_yield.and_return(0.2)
 
-    metric = :http_server_requests_total
-    labels = { method: 'get', path: '/foo', code: '200' }
-    expect(registry.get(metric).get(labels: labels)).to eql(1.0)
+      get '/foo'
 
-    metric = :http_server_request_duration_seconds
-    labels = { method: 'get', path: '/foo' }
-    expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.25" => 1)
+      metric = :http_server_requests_total
+      labels = { method: 'get', path: '/foo', code: '200' }
+      expect(registry.get(metric).get(labels: labels)).to eql(1.0)
+
+      metric = :http_server_request_duration_seconds
+      labels = { method: 'get', path: '/foo' }
+      expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.25" => 1)
+    end
   end
 
   it 'normalizes paths containing numeric IDs by default' do
@@ -113,6 +117,7 @@
         metrics_prefix: 'lolrus',
       )
     end
+    let(:registry) { Prometheus::Client::Registry.clone.instance }
 
     it 'provides alternate metric names' do
       expect(

spec/prometheus/middleware/exporter_spec.rb

@@ -7,7 +7,7 @@
   include Rack::Test::Methods
 
   let(:registry) do
-    Prometheus::Client::Registry.new
+    Prometheus::Client::Registry.instance
   end
 
   let(:app) do

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"
+
+require_relative "../support/api"
+
+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
+        res = get '/metrics'
+
+        expect(res.body).not_to be_empty
+      end
+
+      t2 = Thread.new do
+        latch.wait
+        res = get '/metrics'
+
+        expect(res.body).not_to be_empty
+      end
+
+      t3 = Thread.new do
+        latch.wait
+        res = get '/metrics'
+        expect(res.body).not_to be_empty
+      end
+
+      t4 = Thread.new do
+        latch.wait
+        res = get '/metrics'
+
+        expect(res.body).not_to be_empty
+      end
+
+      latch.count_down
+
+      [t1, t2, t3, t4].each(&:join)
+    end
+  end
+end

spec/support/api.rb

@@ -0,0 +1,13 @@
+require 'rack'
+
+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
+
+