From 1eb1c9ac6196b3bbb8f07277edcf32b7b32f9e59 Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sun, 21 Nov 2021 00:33:03 +0000 Subject: [PATCH 1/7] Remove `instance` as a special grouping key label in push client This is in preparation for the introduction of arbitrary labels in the grouping key. We no longer need to support `instance` as a special case, and will instead generate a path that combines the job label with everything passed in a grouping key hash. Signed-off-by: Chris Sinjakli --- lib/prometheus/client/push.rb | 16 +++++----------- spec/prometheus/client/push_spec.rb | 18 +++--------------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index a0b4b8cf..75bdfb88 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -16,20 +16,18 @@ module Client class Push DEFAULT_GATEWAY = 'http://localhost:9091'.freeze PATH = '/metrics/job/%s'.freeze - INSTANCE_PATH = '/metrics/job/%s/instance/%s'.freeze SUPPORTED_SCHEMES = %w(http https).freeze - attr_reader :job, :instance, :gateway, :path + attr_reader :job, :gateway, :path - def initialize(job:, instance: nil, gateway: DEFAULT_GATEWAY, **kwargs) + def initialize(job:, gateway: DEFAULT_GATEWAY, **kwargs) raise ArgumentError, "job cannot be nil" if job.nil? raise ArgumentError, "job cannot be empty" if job.empty? @mutex = Mutex.new @job = job - @instance = instance @gateway = gateway || DEFAULT_GATEWAY - @path = build_path(job, instance) + @path = build_path(job) @uri = parse("#{@gateway}#{@path}") @http = Net::HTTP.new(@uri.host, @uri.port) @@ -70,12 +68,8 @@ def parse(url) raise ArgumentError, "#{url} is not a valid URL: #{e}" end - def build_path(job, instance) - if instance && !instance.empty? - format(INSTANCE_PATH, ERB::Util::url_encode(job), ERB::Util::url_encode(instance)) - else - format(PATH, ERB::Util::url_encode(job)) - end + def build_path(job) + format(PATH, ERB::Util::url_encode(job)) end def request(req_class, registry = nil) diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index c8cd55d4..63818f2a 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -70,28 +70,16 @@ end describe '#path' do - it 'uses the default metrics path if no instance value given' do + it 'uses the default metrics path if no grouping key given' do push = Prometheus::Client::Push.new(job: 'test-job') expect(push.path).to eql('/metrics/job/test-job') end - it 'uses the default metrics path if an empty instance value is given' do - push = Prometheus::Client::Push.new(job: 'bar-job', instance: '') - - expect(push.path).to eql('/metrics/job/bar-job') - end - - it 'uses the full metrics path if an instance value is given' do - push = Prometheus::Client::Push.new(job: 'bar-job', instance: 'foo') - - expect(push.path).to eql('/metrics/job/bar-job/instance/foo') - end - it 'escapes non-URL characters' do - push = Prometheus::Client::Push.new(job: 'bar job', instance: 'foo ') + push = Prometheus::Client::Push.new(job: '') - expected = '/metrics/job/bar%20job/instance/foo%20%3Cmy%20instance%3E' + expected = '/metrics/job/%3Cbar%20job%3E' expect(push.path).to eql(expected) end end From 5be6736a8a5ce823697d04f86e3fac2fe31e84ed Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sun, 21 Nov 2021 17:18:45 +0000 Subject: [PATCH 2/7] Support arbitrary grouping keys in push client This doesn't validate label keys or handle encoding special values in label values yet. That will be added in a later commit. Signed-off-by: Chris Sinjakli --- lib/prometheus/client/push.rb | 15 +++++++++++---- spec/prometheus/client/push_spec.rb | 13 +++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index 75bdfb88..dc17d1c0 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -20,14 +20,15 @@ class Push attr_reader :job, :gateway, :path - def initialize(job:, gateway: DEFAULT_GATEWAY, **kwargs) + def initialize(job:, gateway: DEFAULT_GATEWAY, grouping_key: {}, **kwargs) raise ArgumentError, "job cannot be nil" if job.nil? raise ArgumentError, "job cannot be empty" if job.empty? @mutex = Mutex.new @job = job @gateway = gateway || DEFAULT_GATEWAY - @path = build_path(job) + @grouping_key = grouping_key + @path = build_path(job, grouping_key) @uri = parse("#{@gateway}#{@path}") @http = Net::HTTP.new(@uri.host, @uri.port) @@ -68,8 +69,14 @@ def parse(url) raise ArgumentError, "#{url} is not a valid URL: #{e}" end - def build_path(job) - format(PATH, ERB::Util::url_encode(job)) + def build_path(job, grouping_key) + path = format(PATH, ERB::Util::url_encode(job)) + + grouping_key.each do |label, value| + path += "/#{label}/#{ERB::Util::url_encode(value)}" + end + + path end def request(req_class, registry = nil) diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index 63818f2a..9d3e9ce0 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -76,10 +76,19 @@ expect(push.path).to eql('/metrics/job/test-job') end + it 'appends additional grouping labels to the path if specified' do + push = Prometheus::Client::Push.new( + job: 'test-job', + grouping_key: { foo: "bar", baz: "qux"}, + ) + + expect(push.path).to eql('/metrics/job/test-job/foo/bar/baz/qux') + end + it 'escapes non-URL characters' do - push = Prometheus::Client::Push.new(job: '') + push = Prometheus::Client::Push.new(job: '', grouping_key: { foo_label: '' }) - expected = '/metrics/job/%3Cbar%20job%3E' + expected = '/metrics/job/%3Cbar%20job%3E/foo_label/%3Cbar%20value%3E' expect(push.path).to eql(expected) end end From 2b3310d5b808080a5dd2907b5fbf28beee640a81 Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sat, 1 Jan 2022 17:00:45 +0000 Subject: [PATCH 3/7] Validate grouping key labels in push client This builds on the introduction of `grouping_key` in a previous commit, ensuring that the labels passed in follow the usual naming restrictions on Prometheus labels. Signed-off-by: Chris Sinjakli --- lib/prometheus/client/push.rb | 3 +++ spec/prometheus/client/push_spec.rb | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index dc17d1c0..fba85c35 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -7,6 +7,7 @@ require 'prometheus/client' require 'prometheus/client/formats/text' +require 'prometheus/client/label_set_validator' module Prometheus # Client is a ruby implementation for a Prometheus compatible client. @@ -23,6 +24,8 @@ class Push def initialize(job:, gateway: DEFAULT_GATEWAY, grouping_key: {}, **kwargs) raise ArgumentError, "job cannot be nil" if job.nil? raise ArgumentError, "job cannot be empty" if job.empty? + @validator = LabelSetValidator.new(expected_labels: grouping_key.keys) + @validator.validate_symbols!(grouping_key) @mutex = Mutex.new @job = job diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index 9d3e9ce0..ccb9fbc8 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -43,6 +43,12 @@ end.to raise_error ArgumentError end end + + it 'raises InvalidLabelError if a grouping key label has an invalid name' do + expect do + Prometheus::Client::Push.new(job: "test-job", grouping_key: { "not_a_symbol" => "foo" }) + end.to raise_error Prometheus::Client::LabelSetValidator::InvalidLabelError + end end describe '#add' do From 6e5b14ffd89191658b77093e52684fe87b60559e Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sat, 1 Jan 2022 22:14:48 +0000 Subject: [PATCH 4/7] Use base64 encoding when necessary in grouping key in push client Certain label values (empty string, and anything containing a `/`) cannot be used in the grouping key without extra work to encode them properly. Labels are represented as pairs of path segments in the URL, so an unencoded `/` would be treated as a path separator. The empty string would result in two consecturive path separators (`//`), which HTTP libraries, proxies, and web servers are liable to normalise away. This commit uses the base64 encoding method supported by the pushgateway server to encode such values. See: https://github.com/prometheus/pushgateway/blob/6393a901f56d4dda62cd0f6ab1f1f07c495b6354/README.md#url Signed-off-by: Chris Sinjakli --- lib/prometheus/client/push.rb | 22 +++++++++++++++++++++- spec/prometheus/client/push_spec.rb | 18 ++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index fba85c35..686b336c 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -1,5 +1,6 @@ # encoding: UTF-8 +require 'base64' require 'thread' require 'net/http' require 'uri' @@ -76,7 +77,26 @@ def build_path(job, grouping_key) path = format(PATH, ERB::Util::url_encode(job)) grouping_key.each do |label, value| - path += "/#{label}/#{ERB::Util::url_encode(value)}" + if value.include?('/') + encoded_value = Base64.urlsafe_encode64(value) + path += "/#{label}@base64/#{encoded_value}" + # While it's valid for the urlsafe_encode64 function to return an + # empty string when the input string is empty, it doesn't work for + # our specific use case as we're putting the result into a URL path + # segment. A double slash (`//`) can be normalised away by HTTP + # libraries, proxies, and web servers. + # + # For empty strings, we use a single padding character (`=`) as the + # value. + # + # See the pushgateway docs for more details: + # + # https://github.com/prometheus/pushgateway/blob/6393a901f56d4dda62cd0f6ab1f1f07c495b6354/README.md#url + elsif value.empty? + path += "/#{label}@base64/=" + else + path += "/#{label}/#{ERB::Util::url_encode(value)}" + end end path diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index ccb9fbc8..84418379 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -91,6 +91,24 @@ expect(push.path).to eql('/metrics/job/test-job/foo/bar/baz/qux') end + it 'encodes grouping key label values containing `/` in url-safe base64' do + push = Prometheus::Client::Push.new( + job: 'test-job', + grouping_key: { foo: "bar/baz"}, + ) + + expect(push.path).to eql('/metrics/job/test-job/foo@base64/YmFyL2Jheg==') + end + + it 'encodes empty grouping key label values as a single base64 padding character' do + push = Prometheus::Client::Push.new( + job: 'test-job', + grouping_key: { foo: ""}, + ) + + expect(push.path).to eql('/metrics/job/test-job/foo@base64/=') + end + it 'escapes non-URL characters' do push = Prometheus::Client::Push.new(job: '', grouping_key: { foo_label: '' }) From 84d475586f39b7bd23dc565604be4cdcbac33986 Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sat, 1 Jan 2022 23:24:02 +0000 Subject: [PATCH 5/7] Raise error when grouping key clashes with metric labels in push client The pushgateway will overwrite metric labels if the same label is used as part of the grouping key. Other Promethus clients (at least the Go one) will error if you try to do this, as it can be quite unexpected. This commit makes us follow suit. We may find that some users prefer to let those clashes through (i.e. let the labels from the grouping_key win). If that ends up being the case, we can introduce a config flag for people to opt into that behaviour. Signed-off-by: Chris Sinjakli --- lib/prometheus/client/metric.rb | 2 +- lib/prometheus/client/push.rb | 22 ++++++++++++++++++++++ spec/prometheus/client/push_spec.rb | 26 ++++++++++++++++++++++++-- 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/lib/prometheus/client/metric.rb b/lib/prometheus/client/metric.rb index c492b08d..328686e3 100644 --- a/lib/prometheus/client/metric.rb +++ b/lib/prometheus/client/metric.rb @@ -7,7 +7,7 @@ module Prometheus module Client # Metric class Metric - attr_reader :name, :docstring, :preset_labels + attr_reader :name, :docstring, :labels, :preset_labels def initialize(name, docstring:, diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index 686b336c..3639ebc9 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -5,6 +5,7 @@ require 'net/http' require 'uri' require 'erb' +require 'set' require 'prometheus/client' require 'prometheus/client/formats/text' @@ -103,6 +104,8 @@ def build_path(job, grouping_key) end def request(req_class, registry = nil) + validate_no_label_clashes!(registry) if registry + req = req_class.new(@uri) req.content_type = Formats::Text::CONTENT_TYPE req.basic_auth(@uri.user, @uri.password) if @uri.user @@ -114,6 +117,25 @@ def request(req_class, registry = nil) def synchronize @mutex.synchronize { yield } end + + def validate_no_label_clashes!(registry) + # There's nothing to check if we don't have a grouping key + return if @grouping_key.empty? + + # We could be doing a lot of comparisons, so let's do them against a + # set rather than an array + grouping_key_labels = @grouping_key.keys.to_set + + registry.metrics.each do |metric| + metric.labels.each do |label| + if grouping_key_labels.include?(label) + raise LabelSetValidator::InvalidLabelSetError, + "label :#{label} from grouping key collides with label of the " \ + "same name from metric :#{metric.name} and would overwrite it" + end + end + end + end end end end diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index 84418379..9b833d59 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -1,11 +1,13 @@ # encoding: UTF-8 +require 'prometheus/client/gauge' require 'prometheus/client/push' describe Prometheus::Client::Push do let(:gateway) { 'http://localhost:9091' } - let(:registry) { Prometheus::Client.registry } - let(:push) { Prometheus::Client::Push.new(job: 'test-job', gateway: gateway, open_timeout: 5, read_timeout: 30) } + let(:registry) { Prometheus::Client::Registry.new } + let(:grouping_key) { {} } + let(:push) { Prometheus::Client::Push.new(job: 'test-job', gateway: gateway, grouping_key: grouping_key, open_timeout: 5, read_timeout: 30) } describe '.new' do it 'returns a new push instance' do @@ -193,5 +195,25 @@ push.send(:request, Net::HTTP::Put, registry) end end + + context 'with a grouping key that clashes with a metric label' do + let(:grouping_key) { { foo: "bar"} } + + before do + gauge = Prometheus::Client::Gauge.new( + :test_gauge, + labels: [:foo], + docstring: "test docstring" + ) + registry.register(gauge) + gauge.set(42, labels: { foo: "bar" }) + end + + it 'raises an error when grouping key labels conflict with metric labels' do + expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error( + Prometheus::Client::LabelSetValidator::InvalidLabelSetError + ) + end + end end end From ae419d227e2b7cae6ff19f83a16716912856ac1d Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sun, 2 Jan 2022 01:56:21 +0000 Subject: [PATCH 6/7] Raise errors for non-2xx responses in push client Currently, we don't do anything with the response from `Net::HTTP` in the push client. This means that when we get a response that indicates our metrics didn't make it to the pushgateway, we silently carry on, and the user has no ideas that their metrics weren't recorded. If users decide they don't care about this happening, they can always rescue the base `Prometheus::Client::Push::HttpError` class (or everything that extends `StandardError` if they also want to ignore things like timeouts, connection refused, DNS resolution failure, etc). Signed-off-by: Chris Sinjakli --- lib/prometheus/client/push.rb | 24 ++++++- spec/prometheus/client/push_spec.rb | 103 ++++++++++++++++++++++++++-- 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb index 3639ebc9..04f4d7ae 100644 --- a/lib/prometheus/client/push.rb +++ b/lib/prometheus/client/push.rb @@ -17,6 +17,11 @@ module Client # Push implements a simple way to transmit a given registry to a given # Pushgateway. class Push + class HttpError < StandardError; end + class HttpRedirectError < HttpError; end + class HttpClientError < HttpError; end + class HttpServerError < HttpError; end + DEFAULT_GATEWAY = 'http://localhost:9091'.freeze PATH = '/metrics/job/%s'.freeze SUPPORTED_SCHEMES = %w(http https).freeze @@ -111,7 +116,10 @@ def request(req_class, registry = nil) req.basic_auth(@uri.user, @uri.password) if @uri.user req.body = Formats::Text.marshal(registry) if registry - @http.request(req) + response = @http.request(req) + validate_response!(response) + + response end def synchronize @@ -136,6 +144,20 @@ def validate_no_label_clashes!(registry) end end end + + def validate_response!(response) + status = Integer(response.code) + if status >= 300 + message = "status: #{response.code}, message: #{response.message}, body: #{response.body}" + if status <= 399 + raise HttpRedirectError, message + elsif status <= 499 + raise HttpClientError, message + else + raise HttpServerError, message + end + end + end end end end diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index 9b833d59..95429a13 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -123,6 +123,14 @@ let(:content_type) { Prometheus::Client::Formats::Text::CONTENT_TYPE } let(:data) { Prometheus::Client::Formats::Text.marshal(registry) } let(:uri) { URI.parse("#{gateway}/metrics/job/test-job") } + let(:response) do + double( + :response, + code: '200', + message: 'OK', + body: 'Everything worked' + ) + end it 'sends marshalled registry to the specified gateway' do request = double(:request) @@ -134,12 +142,99 @@ expect(http).to receive(:use_ssl=).with(false) expect(http).to receive(:open_timeout=).with(5) expect(http).to receive(:read_timeout=).with(30) - expect(http).to receive(:request).with(request) + expect(http).to receive(:request).with(request).and_return(response) expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) push.send(:request, Net::HTTP::Post, registry) end + context 'for a 3xx response' do + let(:response) do + double( + :response, + code: '301', + message: 'Moved Permanently', + body: 'Probably no body, but technically you can return one' + ) + end + + it 'raises a redirect error' do + request = double(:request) + allow(request).to receive(:content_type=) + allow(request).to receive(:body=) + allow(Net::HTTP::Post).to receive(:new).with(uri).and_return(request) + + http = double(:http) + allow(http).to receive(:use_ssl=) + allow(http).to receive(:open_timeout=) + allow(http).to receive(:read_timeout=) + allow(http).to receive(:request).with(request).and_return(response) + allow(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) + + expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error( + Prometheus::Client::Push::HttpRedirectError + ) + end + end + + context 'for a 4xx response' do + let(:response) do + double( + :response, + code: '400', + message: 'Bad Request', + body: 'Info on why the request was bad' + ) + end + + it 'raises a client error' do + request = double(:request) + allow(request).to receive(:content_type=) + allow(request).to receive(:body=) + allow(Net::HTTP::Post).to receive(:new).with(uri).and_return(request) + + http = double(:http) + allow(http).to receive(:use_ssl=) + allow(http).to receive(:open_timeout=) + allow(http).to receive(:read_timeout=) + allow(http).to receive(:request).with(request).and_return(response) + allow(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) + + expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error( + Prometheus::Client::Push::HttpClientError + ) + end + end + + context 'for a 5xx response' do + let(:response) do + double( + :response, + code: '500', + message: 'Internal Server Error', + body: 'Apology for the server code being broken' + ) + end + + it 'raises a server error' do + request = double(:request) + allow(request).to receive(:content_type=) + allow(request).to receive(:body=) + allow(Net::HTTP::Post).to receive(:new).with(uri).and_return(request) + + http = double(:http) + allow(http).to receive(:use_ssl=) + allow(http).to receive(:open_timeout=) + allow(http).to receive(:read_timeout=) + allow(http).to receive(:request).with(request).and_return(response) + allow(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) + + expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error( + Prometheus::Client::Push::HttpServerError + ) + end + end + it 'deletes data from the registry' do request = double(:request) expect(request).to receive(:content_type=).with(content_type) @@ -149,7 +244,7 @@ expect(http).to receive(:use_ssl=).with(false) expect(http).to receive(:open_timeout=).with(5) expect(http).to receive(:read_timeout=).with(30) - expect(http).to receive(:request).with(request) + expect(http).to receive(:request).with(request).and_return(response) expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) push.send(:request, Net::HTTP::Delete) @@ -168,7 +263,7 @@ expect(http).to receive(:use_ssl=).with(true) expect(http).to receive(:open_timeout=).with(5) expect(http).to receive(:read_timeout=).with(30) - expect(http).to receive(:request).with(request) + expect(http).to receive(:request).with(request).and_return(response) expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) push.send(:request, Net::HTTP::Post, registry) @@ -189,7 +284,7 @@ expect(http).to receive(:use_ssl=).with(true) expect(http).to receive(:open_timeout=).with(5) expect(http).to receive(:read_timeout=).with(30) - expect(http).to receive(:request).with(request) + expect(http).to receive(:request).with(request).and_return(response) expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) push.send(:request, Net::HTTP::Put, registry) From 2105319d119a7b195c2d75e97fc6327775483a75 Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Sun, 9 Jan 2022 02:08:28 +0000 Subject: [PATCH 7/7] Tweak wording of spec for clarity Signed-off-by: Chris Sinjakli --- spec/prometheus/client/push_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb index 95429a13..372094ca 100644 --- a/spec/prometheus/client/push_spec.rb +++ b/spec/prometheus/client/push_spec.rb @@ -111,7 +111,7 @@ expect(push.path).to eql('/metrics/job/test-job/foo@base64/=') end - it 'escapes non-URL characters' do + it 'URL-encodes all other non-URL-safe characters' do push = Prometheus::Client::Push.new(job: '', grouping_key: { foo_label: '' }) expected = '/metrics/job/%3Cbar%20job%3E/foo_label/%3Cbar%20value%3E'