From a26ae7d80a6e116f197d0b1cb02bd02ac65d363f Mon Sep 17 00:00:00 2001 From: Chris Sinjakli Date: Tue, 8 Mar 2022 23:04:47 +0000 Subject: [PATCH] Remove framework-specific route detection from collector middleware Sadly, with the benefit of hindsight, this wasn't a good idea. There are two reasons we're dropping this: - It doesn't play nicely with libraries like `Rack::Builder`, which dispatches requests to different Rack apps based on a path prefix in a way that isn't visible to middleware. For example, when using `Rack::Builder`, `sinatra.route` only contains the parts of the path after the prefix that `Rack::Builder` used to dispatch to the specific app, and doesn't leave any information in the request environment to indicate which prefix it dispatched to. - It turns out framework-specific route info isn't always formatted in a way that looks good in a Prometheus label. For example, when using regex-based route-matching in Sinatra, you can end up with labels that look like `\\/vapes\\/([0-9]+)\\/?`. For a really detailed dive into those two issues, see this GitHub comment: https://github.com/prometheus/client_ruby/issues/249#issuecomment-1061317511 Signed-off-by: Chris Sinjakli --- lib/prometheus/middleware/collector.rb | 52 ++------------------ spec/prometheus/middleware/collector_spec.rb | 31 ------------ 2 files changed, 5 insertions(+), 78 deletions(-) diff --git a/lib/prometheus/middleware/collector.rb b/lib/prometheus/middleware/collector.rb index 26893176..c785d61f 100644 --- a/lib/prometheus/middleware/collector.rb +++ b/lib/prometheus/middleware/collector.rb @@ -72,12 +72,12 @@ def record(env, code, duration) counter_labels = { code: code, method: env['REQUEST_METHOD'].downcase, - path: strip_ids_from_path(path), + path: path, } duration_labels = { method: env['REQUEST_METHOD'].downcase, - path: strip_ids_from_path(path), + path: path, } @requests.increment(labels: counter_labels) @@ -87,52 +87,10 @@ def record(env, code, duration) nil end - # While `PATH_INFO` is framework agnostic, and works for any Rack app, some Ruby web - # frameworks pass a more useful piece of information into the request env - the - # route that the request matched. - # - # This means that rather than using our generic `:id` and `:uuid` replacements in - # the `path` label for any path segments that look like dynamic IDs, we can put the - # actual route that matched in there, with correctly named parameters. For example, - # if a Sinatra app defined a route like: - # - # get "/foo/:bar" do - # ... - # end - # - # instead of containing `/foo/:id`, the `path` label would contain `/foo/:bar`. - # - # Sadly, Rails is a notable exception, and (as far as I can tell at the time of - # writing) doesn't provide this info in the request env. def generate_path(env) - if env['sinatra.route'] - route = env['sinatra.route'].partition(' ').last - elsif env['grape.routing_args'] - # We are deep in the weeds of an object that Grape passes into the request env, - # but don't document any explicit guarantees about. Let's have a fallback in - # case they change it down the line. - # - # This code would be neater with the safe navigation operator (`&.`) here rather - # than the much more verbose `respond_to?` calls, but unlike Rails' `try` - # method, it still raises an error if the object is non-nil, but doesn't respond - # to the method being called on it. - route = nil - - route_info = env.dig('grape.routing_args', :route_info) - if route_info.respond_to?(:pattern) - pattern = route_info.pattern - if pattern.respond_to?(:origin) - route = pattern.origin - end - end - - # Fall back to PATH_INFO if Grape change the structure of `grape.routing_args` - route ||= env['PATH_INFO'] - else - route = env['PATH_INFO'] - end - - [env['SCRIPT_NAME'], route].join + full_path = [env['SCRIPT_NAME'], env['PATH_INFO']].join + + strip_ids_from_path(full_path) end def strip_ids_from_path(path) diff --git a/spec/prometheus/middleware/collector_spec.rb b/spec/prometheus/middleware/collector_spec.rb index 06034289..626fc146 100644 --- a/spec/prometheus/middleware/collector_spec.rb +++ b/spec/prometheus/middleware/collector_spec.rb @@ -123,37 +123,6 @@ expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.5" => 1) end - it 'prefers sinatra.route to PATH_INFO' do - metric = :http_server_requests_total - - env('sinatra.route', 'GET /foo/:named_param') - get '/foo/7' - env('sinatra.route', nil) - expect(registry.get(metric).values.keys.last[:path]).to eql("/foo/:named_param") - end - - it 'prefers grape.routing_args to PATH_INFO' do - metric = :http_server_requests_total - - # request.env["grape.routing_args"][:route_info].pattern.origin - # - # Yes, this is the object you have to traverse to get the path. - # - # No, I'm not happy about it either. - grape_routing_args = { - route_info: double(:route_info, - pattern: double(:pattern, - origin: '/foo/:named_param' - ) - ) - } - - env('grape.routing_args', grape_routing_args) - get '/foo/7' - env('grape.routing_args', nil) - expect(registry.get(metric).values.keys.last[:path]).to eql("/foo/:named_param") - end - it "falls back to PATH_INFO if the structure of grape.routing_args changes" do metric = :http_server_requests_total