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: #249 (comment) Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>

Author: Sinjo

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)

spec/prometheus/middleware/collector_spec.rb

@@ -123,54 +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
-
- grape_routing_args = {
- route_info: double(:route_info,
- pattern: double(:pattern,
- origin_but_different: '/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/:id")
- end
-
  context 'when the app raises an exception' do
  let(:original_app) do
  lambda do |env|