From 0761f5924173bd273028b5143e44321eb23d6f8a Mon Sep 17 00:00:00 2001 From: Daniel Magliola Date: Wed, 14 Oct 2020 11:09:55 +0100 Subject: [PATCH] Include SCRIPT_NAME when determining path in Collector When determining the path for a request, `Rack::Request` prefixes the `SCRIPT_NAME`, [as seen here][1]. This is a problem with our current code when using mountable engines, where the engine part of the path gets lost. This patch fixes that to include `SCRIPT_NAME` as part of the path. NOTE: This is not backwards compatible. Labels will change in existing metrics. We will cut a new major version once we ship this. [1]: https://github.com/rack/rack/blob/294fd239a71aab805877790f0a92ee3c72e67d79/lib/rack/request.rb#L512 Co-authored-by: Ian Ker-Seymer Co-authored-by: Ruslan Kornev Signed-off-by: Daniel Magliola --- lib/prometheus/middleware/collector.rb | 6 ++++-- spec/prometheus/middleware/collector_spec.rb | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/prometheus/middleware/collector.rb b/lib/prometheus/middleware/collector.rb index 65de3d17..48e3ddd1 100644 --- a/lib/prometheus/middleware/collector.rb +++ b/lib/prometheus/middleware/collector.rb @@ -67,15 +67,17 @@ def trace(env) end def record(env, code, duration) + path = [env["SCRIPT_NAME"], env['PATH_INFO']].join + counter_labels = { code: code, method: env['REQUEST_METHOD'].downcase, - path: strip_ids_from_path(env['PATH_INFO']), + path: strip_ids_from_path(path), } duration_labels = { method: env['REQUEST_METHOD'].downcase, - path: strip_ids_from_path(env['PATH_INFO']), + path: strip_ids_from_path(path), } @requests.increment(labels: counter_labels) diff --git a/spec/prometheus/middleware/collector_spec.rb b/spec/prometheus/middleware/collector_spec.rb index e2d53164..7809bcf9 100644 --- a/spec/prometheus/middleware/collector_spec.rb +++ b/spec/prometheus/middleware/collector_spec.rb @@ -55,6 +55,18 @@ expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.25" => 1) end + it 'includes SCRIPT_NAME in the path if provided' do + metric = :http_server_requests_total + + get '/foo' + expect(registry.get(metric).values.keys.last[:path]).to eql("/foo") + + env('SCRIPT_NAME', '/engine') + get '/foo' + env('SCRIPT_NAME', nil) + expect(registry.get(metric).values.keys.last[:path]).to eql("/engine/foo") + end + it 'normalizes paths containing numeric IDs by default' do expect(Benchmark).to receive(:realtime).and_yield.and_return(0.3)