Merge pull request #251 from prometheus/sinjo-remove-framework-specific-routing

Remove framework-specific route detection from collector middleware

Author: Sinjo

examples/rack/README.md

@@ -54,22 +54,42 @@ example, if you want to [change the way IDs are stripped from the
 path](https://github.com/prometheus/client_ruby/blob/982fe2e3c37e2940d281573c7689224152dd791f/lib/prometheus/middleware/collector.rb#L97-L101)
 you could override the appropriate method:
 
-```Ruby
+```ruby
 require 'prometheus/middleware/collector'
-module Prometheus
-  module Middleware
-    class MyCollector < Collector
-      def strip_ids_from_path(path)
-        super(path)
-          .gsub(/8675309/, ':jenny\\1')
-      end
-    end
+
+class MyCollector < Prometheus::Middleware::Collector
+  def strip_ids_from_path(path)
+    super(path)
+      .gsub(/8675309/, ':jenny\\1')
   end
 end
 ```
 
 and use your class in `config.ru` instead.
 
+If you want to completely customise how the `path` label is generated, you can
+override `generate_path`. For example, to use
+[Sinatra](https://github.com/sinatra/sinatra)'s framework-specific route info
+from the request environment:
+
+```ruby
+require 'prometheus/middleware/collector'
+
+class MyCollector < Prometheus::Middleware::Collector
+  def generate_path(env)
+    # `sinatra.route` contains both the request method and the route, separated
+    # by a space (e.g. "GET /payments/:id"). To get just the request path, you
+    # can partition the string on " ".
+    env['sinatra.route'].partition(' ').last
+  end
+end
+```
+
+Just make sure that your custom path generation logic strips IDs from the path
+it returns, or gets the path from a source that would never contain them in the
+first place (such as `sinatra.route`), otherwise you'll generate a huge number
+of label values!
+
 **Note:** `Prometheus::Middleware::Collector` isn't explicitly designed to be
 subclassed, so the internals are liable to change at any time, including in
 patch releases. Overriding its methods is done at your own risk!

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|