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|