Creating a plugin that detects and records the matched params

[EduardoSimon]

Hi @jeremyevans , I am trying to create a plugin to instrument an application following the Open Telemetry spec. The plugin must be automatic, like every other instrumentation, requiring should start generating telemetry data according to the spec. My initial approach was to have a middleware like this one at the rack app level that could read the env and obtain the information needed to build the attributes described in the otel spec. That middleware would be autoloaded using an approach similar like the following:

require "roda"
require "opentelemetry-instrumentation-rack"
require "opentelemetry/instrumentation/rack/middlewares/tracer_middleware"

OpenTelemetry::SDK::configure do |c|
  c.use "OpenTelemetry::Instrumentation::Rack"
end

class CustomMiddleware
   def initialize(app)
      @app = app
   end
   
   def call(env)
      @app.call(env)
      ### Add the telemetry data needed here
   end
end

class Roda
  module RodaPlugins
    module OtelInstrumenter
      def self.configure(app)
        app.use OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware
        app.use CustomMiddleware
      end
    end
  end
end

Roda.plugin Roda::RodaPlugins::OtelInstrumenter

Just by requiring this file we could start using our roda instrumentation. Most of the basic http attributes from the spec are covered by the rack instrumentation however there is one that I haven't figured out how it could possibly be implemented: the http.route. That attribute allows to group traces (units of work in your system) using the parameter described in an on/is block, for example: /users/:userID?. This attribute is extremely useful as you could group every request of fetching a specific user and find anomalies very quickly.

My first approach was to fetch that information from the env hash in rack, however, after digging in the roda source code I've realized that there's no track of those matched params, they are just consumed like any other part of the path. So my question is: is there a way from a plugin perspective to identify them and if not, would a PR with the proposed changed by considered? Thanks for the time considering this.

Regards,
Eduardo

[jeremyevans]

You probably want to use the match_hook plugin: https://roda.jeremyevans.net/rdoc/classes/Roda/RodaPlugins/MatchHook.html

[EduardoSimon]

Hi there,
I have solved the problem described above, unfortunately I wasn't able to use the matchHook plugin as I did not have access to the param name of the block. I have solved it by monkey-patching the if_match method and extracting the args + the parameters of the block. Something along these lines. Its a POC so the performance could be greatly improved, I have to measure it first tho.

def if_match(args, &block)
  @otel_matched_path ||= []
  segment = args.first
  next_token = remaining_path.split("/").reject { |val| val.empty? }.first

  if !@otel_matched_path.include?(segment)
    # When there is a parameter in the matcher. Ex: sessions/:session_id
    if !segment.is_a?(String) && block.arity > 0
      @otel_matched_path << ":#{block.parameters.flatten[1]}"
    # when its a symbol match. r.on :session_id
    elsif segment.is_a?(Symbol)
      @otel_matched_path << segment
    # when its a normal match. r.on "form"
    elsif segment == next_token
      @otel_matched_path << segment
    end
  end

  env["http.route"] = "#{env["SCRIPT_NAME"]}/#{@otel_matched_path.join("/")}"

  super(args, &block)
end

I had to follow a similar approach for the always method, except in that case having no args makes the logic a bit simpler.

Having the http.route calculated I can then access the request's env and read it from a rack middleware. As a result, I can view the traces with low cardinality such as:

POST /v1/sessions/:session_id/cancel

Likewise, I was able to instrument exception within a trace monkeypatching the _hanlde_error internal method.

module InstanceMethods
  # Override the internal handle_error method 
  # https://github.com/jeremyevans/roda/blob/master/lib/roda/plugins/error_handler.rb#L93
  # so the exception could be captured before sending it to the defined handler in the roda apps
  def _handle_error(e)
    span = OpenTelemetry::Instrumentation::Rack.current_span
    return unless span.recording?

    span.record_exception(e)
    super(e)
  end
end

I wanted to have a hook before dispatching the exception to the error_handler from nested roda apps. Is there a better way of doing it?

Nonetheless, I am bit worried that we are modifying the internal api of the framework, and I don't feel really comfortable doing so. Hence, I was thinking on adding the information of the args and the parameters of the block to the match hook so that a custom plugin like mine could use it.

What's your take on it?

Regards,
Eduardo

[jeremyevans]

Sorry for the delay. I just pushed up 44a7e0e, which includes a match_hook_args plugin that should do what you want. Can you check if it will meet your needs? I'm planning the next release for August 12.

[EduardoSimon]

Thanks for the time taken to add that new roda plugin. I've checked it out and the plugin does mostly everything I need. The only thing that I am missing is the name of the parameter in the matched block. For example, in the test file added in the commit there is a test case with and endpoint looking like: /baz/1. In order to add the http.route otel attribute we need to group every call to /baz/ under a common route so it has low cardinality. This would allow to do aggregations on request data for the route. Thats why in my initial implementation I was inspecting the name of the args passed to the block. In this specific case: :id. Would it be possible that you add it to the plugin?

On top of that, it's true that in older applications instead of matching by type like Integer a symbol could be use instead. In those cases, that symbol should take precedence.

[jeremyevans]

For grouping, you should be able to use the matchers argument for that, instead of inspecting the block parameter names. I don't think it's a good idea to add the block parameter names, as it requires significant work to do that, as opposed to passing the matchers and block args (which are both local variables).

If you don't think using the matchers will work, can you explain why?

  r.get 'baz', Integer do |id|
    # shouldn't matter if you group by Integer or :id here, right?
  end

[EduardoSimon]

Yep, that's true, it shouldn't matter, however, when inspecting the routes of your app in a telemetry viewer app like honeycomb or new relic, we would find:

POST /baz/Integer
POST /orders/String

instead of:

POST /baz/:id
POST /orders/:order_id

As far as I'm concerned, inspecting the name of the argument the block is by no means a good idea, but as other rack-compliant web frameworks force you to use a symbol when defining the route like Sinatra, it would be nice to have a consistent shape of the telemetry data.

I can provide richer examples to better illustrate the use case if needed.

Regards

[jeremyevans]

It seems like you could handle class matchers by taking their name, downcasing, and sticking a : in front. For example:

POST /baz/:integer
POST /orders/:string

If you don't think that's good enough, you should probably use :id/:order_id instead of Integer/String as the matcher (no functionality lost now that symbol_matchers supports typecasting).