From dac375d21a82b548a31b8747046af80b2cbd0e1a Mon Sep 17 00:00:00 2001 From: Matthieu Prat Date: Tue, 25 Aug 2020 14:38:27 +0100 Subject: [PATCH 1/7] Make all registry methods thread safe The registry is backed by a Hash, which is not guaranteed to be thread safe on all interpreters. For peace of mind, this change synchronizes all accesses to the metrics hash. Another option would have been to use a [thread-safe Hash][1] instead of a Hash but this would have meant adding Ruby Concurrent as a dependency, which I'm assuming we don't want. Ref: https://github.com/prometheus/client_ruby/pull/184#discussion_r406038191 [1]: https://github.com/ruby-concurrency/concurrent-ruby/blob/v1.1.7/lib/concurrent-ruby/concurrent/hash.rb Signed-off-by: Matthieu Prat --- lib/prometheus/client/registry.rb | 8 ++++---- spec/prometheus/client/registry_spec.rb | 4 ---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/prometheus/client/registry.rb b/lib/prometheus/client/registry.rb index 4bf63aa4..0b2f6e9a 100644 --- a/lib/prometheus/client/registry.rb +++ b/lib/prometheus/client/registry.rb @@ -22,7 +22,7 @@ def register(metric) name = metric.name @mutex.synchronize do - if exist?(name.to_sym) + if @metrics.key?(name.to_sym) raise AlreadyRegisteredError, "#{name} has already been registered" end @metrics[name.to_sym] = metric @@ -73,15 +73,15 @@ def histogram(name, docstring:, labels: [], preset_labels: {}, end def exist?(name) - @metrics.key?(name) + @mutex.synchronize { @metrics.key?(name) } end def get(name) - @metrics[name.to_sym] + @mutex.synchronize { @metrics[name.to_sym] } end def metrics - @metrics.values + @mutex.synchronize { @metrics.values } end end end diff --git a/spec/prometheus/client/registry_spec.rb b/spec/prometheus/client/registry_spec.rb index 32727d00..3c4da190 100644 --- a/spec/prometheus/client/registry_spec.rb +++ b/spec/prometheus/client/registry_spec.rb @@ -33,10 +33,6 @@ mutex = Mutex.new containers = [] - def registry.exist?(*args) - super.tap { sleep(0.01) } - end - Array.new(5) do Thread.new do result = begin From ec47d6bd6ef09d346c05ec545a2da112fe67f28a Mon Sep 17 00:00:00 2001 From: James Turley Date: Thu, 30 Jul 2020 08:49:45 +0100 Subject: [PATCH 2/7] Add port option to exporter middleware If the port option is set, all requests for /metrics on other ports will be forwarded to the app. If it is unset or nil, or the ports match, export things as usual. Allows separate mounting of metrics and main app to enforce different security setups etc. Signed-off-by: James Turley --- lib/prometheus/middleware/exporter.rb | 7 +++++- spec/prometheus/middleware/exporter_spec.rb | 28 ++++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/prometheus/middleware/exporter.rb b/lib/prometheus/middleware/exporter.rb index 5a74d8e9..640a3985 100644 --- a/lib/prometheus/middleware/exporter.rb +++ b/lib/prometheus/middleware/exporter.rb @@ -21,11 +21,12 @@ def initialize(app, options = {}) @app = app @registry = options[:registry] || Client.registry @path = options[:path] || '/metrics' + @port = options[:port] @acceptable = build_dictionary(FORMATS, FALLBACK) end def call(env) - if env['PATH_INFO'] == @path + if metrics_port?(env['SERVER_PORT']) && env['PATH_INFO'] == @path format = negotiate(env, @acceptable) format ? respond_with(format) : not_acceptable(FORMATS) else @@ -86,6 +87,10 @@ def build_dictionary(formats, fallback) memo[format::MEDIA_TYPE] = format end end + + def metrics_port?(request_port) + @port.nil? || @port.to_s == request_port + end end end end diff --git a/spec/prometheus/middleware/exporter_spec.rb b/spec/prometheus/middleware/exporter_spec.rb index 8916513b..5299fc1e 100644 --- a/spec/prometheus/middleware/exporter_spec.rb +++ b/spec/prometheus/middleware/exporter_spec.rb @@ -6,13 +6,14 @@ describe Prometheus::Middleware::Exporter do include Rack::Test::Methods + let(:options) { { registry: registry } } let(:registry) do Prometheus::Client::Registry.new end let(:app) do app = ->(_) { [200, { 'Content-Type' => 'text/html' }, ['OK']] } - described_class.new(app, registry: registry) + described_class.new(app, **options) end context 'when requesting app endpoints' do @@ -96,5 +97,30 @@ include_examples 'ok', { 'HTTP_ACCEPT' => accept }, text end + + context 'when a port is specified' do + let(:options) { { registry: registry, port: 9999 } } + + context 'when a request is on the specified port' do + it 'responds with 200 OK' do + registry.counter(:foo, docstring: 'foo counter').increment(by: 9) + + get 'http://example.org:9999/metrics', nil, {} + + expect(last_response.status).to eql(200) + expect(last_response.header['Content-Type']).to eql(text::CONTENT_TYPE) + expect(last_response.body).to eql(text.marshal(registry)) + end + end + + context 'when a request is not on the specified port' do + it 'returns the app response' do + get 'http://example.org:8888/metrics', nil, {} + + expect(last_response).to be_ok + expect(last_response.body).to eql('OK') + end + end + end end end From 5bd6740de4f75c5d9f1bc4e1203e1e7a542cd617 Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Wed, 24 Mar 2021 00:23:34 +0000 Subject: [PATCH 3/7] Add missing 'cgi' require in push client Signed-off-by: Chris Sinjakli --- lib/prometheus/client/push.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index 03efb9f7..0ac99018 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -3,6 +3,7 @@ require 'thread' require 'net/http' require 'uri' +require 'cgi' require 'prometheus/client' require 'prometheus/client/formats/text' From c89710bb55227036864f233b0f99aa88d34572a9 Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sat, 27 Mar 2021 00:31:29 +0000 Subject: [PATCH 4/7] Update email in gemspec Signed-off-by: Chris Sinjakli --- prometheus-client.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prometheus-client.gemspec b/prometheus-client.gemspec index 20ad18bf..21690fab 100644 --- a/prometheus-client.gemspec +++ b/prometheus-client.gemspec @@ -8,7 +8,7 @@ Gem::Specification.new do |s| s.summary = 'A suite of instrumentation metric primitives' \ 'that can be exposed through a web services interface.' s.authors = ['Ben Kochie', 'Chris Sinjakli', 'Daniel Magliola'] - s.email = ['superq@gmail.com', 'chris@gocardless.com', 'dmagliola@crystalgears.com'] + s.email = ['superq@gmail.com', 'chris@sinjakli.co.uk', 'dmagliola@crystalgears.com'] s.homepage = 'https://github.com/prometheus/client_ruby' s.license = 'Apache 2.0' From 467e30fbb3912a01284e6de3bc6bc49139e4b1a8 Mon Sep 17 00:00:00 2001 From: Nick Van Wiggeren Date: Tue, 23 Mar 2021 12:49:48 -0700 Subject: [PATCH 5/7] add open/read timeout kwargs Signed-off-by: Nick Van Wiggeren --- lib/prometheus/client/push.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index 0ac99018..3078811a 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -21,7 +21,7 @@ class Push attr_reader :job, :instance, :gateway, :path - def initialize(job, instance = nil, gateway = nil) + def initialize(job, instance = nil, gateway = nil, **kwargs) @mutex = Mutex.new @job = job @instance = instance @@ -31,6 +31,8 @@ def initialize(job, instance = nil, gateway = nil) @http = Net::HTTP.new(@uri.host, @uri.port) @http.use_ssl = (@uri.scheme == 'https') + @http.open_timeout = kwargs[:open_timeout] if kwargs[:open_timeout] + @http.read_timeout = kwargs[:read_timeout] if kwargs[:read_timeout] end def add(registry) From 621c69d06c2fafe19acc375aafea4477dda66722 Mon Sep 17 00:00:00 2001 From: Daniel Magliola Date: Sat, 12 Jun 2021 18:53:49 +0100 Subject: [PATCH 6/7] Use the original metric's store in `with_labels` clone When calling `with_labels` on a metric object (let's call it "the original"), we instantiate a new metric (the "clone") that is identical except that it has some more pre-set labels, that allow the caller to observe it without having to specify the labels every time. "currying", if you will. The problem with the existing code (as exemplified by issue #225, and by the tests introduced in the previous commit), is that as part of making this new metric, we end up instantiating a new store for this metric. With in-memory stores, the new one will be empty. With file stores, it'll bring over the data from the original metric until the point the clone gets observed once, at which point they fork, while pointing at the same file and keeping separate internal state. An almost sure recipe for file corruption. And when exporting, only the data in the "original" metric's store will be exported, the clone's will be ignored, assuming files didn't get corrupted. The fix is not particularly elegant, but I don't see any way around it: we replace the internal store of the "clone" metric with the one from the "original", through the use of a protected method. The only real alternative is getting rid of `with_labels`, which is a nice-to-have for convenience and performance, but not a necessity. Signed-off-by: Daniel Magliola --- lib/prometheus/client/histogram.rb | 18 ++++++++++++------ lib/prometheus/client/metric.rb | 24 +++++++++++++++++++----- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/lib/prometheus/client/histogram.rb b/lib/prometheus/client/histogram.rb index 12d1ed78..97504543 100644 --- a/lib/prometheus/client/histogram.rb +++ b/lib/prometheus/client/histogram.rb @@ -42,12 +42,18 @@ def self.exponential_buckets(start:, factor: 2, count:) end def with_labels(labels) - self.class.new(name, - docstring: docstring, - labels: @labels, - preset_labels: preset_labels.merge(labels), - buckets: @buckets, - store_settings: @store_settings) + new_metric = self.class.new(name, + docstring: docstring, + labels: @labels, + preset_labels: preset_labels.merge(labels), + buckets: @buckets, + store_settings: @store_settings) + + # The new metric needs to use the same store as the "main" declared one, otherwise + # any observations on that copy with the pre-set labels won't actually be exported. + new_metric.replace_internal_store(@store) + + new_metric end def type diff --git a/lib/prometheus/client/metric.rb b/lib/prometheus/client/metric.rb index 100e455b..899926e6 100644 --- a/lib/prometheus/client/metric.rb +++ b/lib/prometheus/client/metric.rb @@ -40,8 +40,16 @@ def initialize(name, metric_type: type, metric_settings: store_settings ) + + # WARNING: Our internal store can be replaced later by `with_labels` + # Everything we do after this point needs to still work if @store gets replaced + end + + protected def replace_internal_store(new_store) + @store = new_store end + # Returns the value for the given label set def get(labels: {}) label_set = label_set_for(labels) @@ -49,11 +57,17 @@ def get(labels: {}) end def with_labels(labels) - self.class.new(name, - docstring: docstring, - labels: @labels, - preset_labels: preset_labels.merge(labels), - store_settings: @store_settings) + new_metric = self.class.new(name, + docstring: docstring, + labels: @labels, + preset_labels: preset_labels.merge(labels), + store_settings: @store_settings) + + # The new metric needs to use the same store as the "main" declared one, otherwise + # any observations on that copy with the pre-set labels won't actually be exported. + new_metric.replace_internal_store(@store) + + new_metric end def init_label_set(labels) From 2fdc82d270b178af3a9cee81e0b01527ad82b976 Mon Sep 17 00:00:00 2001 From: Daniel Magliola Date: Sun, 20 Jun 2021 00:43:48 +0100 Subject: [PATCH 7/7] Release v2.2 There's new features, so it's a minor version bump. Signed-off-by: Daniel Magliola --- lib/prometheus/client/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/prometheus/client/version.rb b/lib/prometheus/client/version.rb index ec33bff5..0e0f5c54 100644 --- a/lib/prometheus/client/version.rb +++ b/lib/prometheus/client/version.rb @@ -2,6 +2,6 @@ module Prometheus module Client - VERSION = '2.1.0' + VERSION = '2.2.0' end end