Skip to content

Commit

Permalink
Include SCRIPT_NAME when determining path in Collector
Browse files Browse the repository at this point in the history
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 <i.kerseymer@gmail.com>
Co-authored-by: Ruslan Kornev <oganer@gmail.com>
Signed-off-by: Daniel Magliola <dmagliola@crystalgears.com>
  • Loading branch information
3 people committed Oct 14, 2020
1 parent 726536c commit 0761f59
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 2 deletions.
6 changes: 4 additions & 2 deletions lib/prometheus/middleware/collector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 12 additions & 0 deletions spec/prometheus/middleware/collector_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down

0 comments on commit 0761f59

Please sign in to comment.