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 <[email protected]>

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|