fix!: remove enable_recognize_route and span_naming options

[ahayworth]

recognize_route can end up mutating the request object in the event it
processes a potential match that is not anchored. That is, shall we say,
not ideal. (And causes production applications to break).

I was initially going to just default the options to off
(essentially), but why leave potentially dangerous code lying about?
Also, it ended up being easier to just yank stuff out rather than fix
the test cases to handle the inverse of what the expected, etc. So, this
commit just removes it all.

Step backwards in convenience, sure. Big step forward in safety? Definitely.

For context: https://github.com/open-telemetry/opentelemetry-ruby-contrib/pull/123/files#r1035711863

[fbogsany]

CI failure is relevant.

Failure:
OpenTelemetry::Instrumentation::Rails#test_0001_sets the span name to the format: HTTP_METHOD /rails/route(.:format) [/home/runner/work/opentelemetry-ruby-contrib/opentelemetry-ruby-contrib/instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/patches/action_controller/metal_test.rb:25]:
Expected: "GET /ok(.:format)"
  Actual: "ExampleController#ok"

[ahayworth]

CI failure is relevant.

Ah, I only ran the action_pack specs locally - will fix.

[plantfansam]

LGTM

[plantfansam]

These basic scenarios are basically copy/pasta from the above, which is why it's ok to remove.

[plantfansam]

In days of yore, it looks like we checked for valid context on the rack span. Not sure if we want to bring that back; not a blocker, just a detail I wanted to surface.

[arielvalentin]

@plantfansam I may be incorrect here but I think that checking if it is read/write span i.e. recording?, is better than checking for a valid context.

If I understand this code correctly, it is possible to have a read-only span that has a valid context:

https://github.com/open-telemetry/opentelemetry-ruby/blob/a4efaf3a44b0efc6ca805a6d0a5bdc4f13a01b34/sdk/lib/opentelemetry/sdk/trace/tracer_provider.rb#LL161C81-L161C81

I think that means it would call the no-opt name and add_attributes methods: https://github.com/open-telemetry/opentelemetry-ruby/blob/a4efaf3a44b0efc6ca805a6d0a5bdc4f13a01b34/api/lib/opentelemetry/trace/span.rb#L138

[plantfansam]

I read the code the same way @arielvalentin, and agree with your reasoning!

[stevenharman]

We leverage http.route in our o11y tooling. With this removal we're going to lose that attribute, which is painful. Do y'all have a recommendation for another approach to get that data back into our spans?

[arielvalentin]

The only thing I can think of at the moment is to amend the current span in a ApplicationController#before_action block like this:

      OpenTelemetry::Trace.current_span.set_attribute("http.route", "#{request.request_method} #{params[:controller]}/#{params[:action]}")

[stevenharman]

Hmm… that's better than missing out on the attribute entirely. But it's not the same fidelity as we were getting from route.path.spec.to_s. I wonder if there's a way to bring this back safely by adding it after the request had been dispatched? Or by leveraging AS::Notifications?

Well, either way, this probably belongs as a discussion in a new Issue or Discussion. Thanks for the great work, y'all!

[shelbyspees]

It looks like Rails 7 will be adding a safer way to get the route URI pattern: rails/rails#47129