Change patching of journey router for rails > 7.1

We don't want to patch #find_routes method instead of #serve method for rails > 7.1,
to make sure that we are setting the http.route tag as early as
possible.

lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb

@@ -11,77 +11,51 @@ module ActionDispatch
           module Instrumentation
             module_function
 
-            def format_http_route(http_route)
-              http_route.gsub(/\(.:format\)\z/, '')
+            def set_http_route_tags(route_spec, script_name)
+              return unless Tracing.enabled?
+
+              return unless route_spec
+
+              request_trace = Tracing.active_trace
+              return unless request_trace
+
+              request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, route_spec.to_s.gsub(/\(.:format\)\z/, ''))
+              request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, script_name) if script_name
+            rescue StandardError => e
+              Datadog.logger.error(e.message)
             end
 
             # Instrumentation for ActionDispatch::Journey components
             module Journey
-              # Instrumentation for ActionDispatch::Journey::Router
-              # for Rails versions older than 7.1
+              # Instrumentation for ActionDispatch::Journey::Router for Rails versions older than 7.1
               module Router
                 def find_routes(req)
                   result = super
 
-                  return result unless Tracing.enabled?
-
-                  active_span = Tracing.active_span
-                  return result unless active_span
-
-                  begin
-                    # Journey::Router#find_routes retuns an array for each matching route.
-                    # This array is [match_data, path_parameters, route].
-                    # We need the route object, since it has a path with route specification.
-                    current_route = result.last&.last&.path&.spec
-                    return result unless current_route
-
-                    # When Rails is serving requests to Rails Engine routes, this function is called
-                    # twice: first time for the route on which the engine is mounted, and second
-                    # time for the internal engine route.
-                    last_route = active_span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE)
+                  # result is an array of [match, parameters, route] tuples
+                  routes = result.map(&:last)
 
-                    active_span.set_tag(
-                      Tracing::Metadata::Ext::HTTP::TAG_ROUTE,
-                      Instrumentation.format_http_route(last_route.to_s + current_route.to_s)
-                    )
-                  rescue StandardError => e
-                    Datadog.logger.error(e.message)
+                  routes.each do |route|
+                    # non-dispatcher routes are not end routes,
+                    # this could be a route prefix for a rails engine for example
+                    Instrumentation.set_http_route_tags(route.path.spec, req.env['SCRIPT_NAME']) if route&.dispatcher?
                   end
 
                   result
                 end
               end
 
-              # Since Rails 7.1 `Router#serve` adds `#route_uri_pattern` attribute to the request,
-              # and the `Router#find_routes` now takes a block as an argument to make the route computation lazy
+              # Since Rails 7.1 `Router#find_routes` makes the route computation lazy
               # https://github.com/rails/rails/commit/35b280fcc2d5d474f9f2be3aca3ae7aa6bba66eb
               module LazyRouter
-                def serve(req)
-                  response = super
-
-                  return response unless Tracing.enabled?
-
-                  active_span = Tracing.active_span
-                  return response unless active_span
-
-                  begin
-                    return response if req.route_uri_pattern.nil?
-
-                    # For normal Rails routes `#route_uri_pattern` is the full route and `#script_name` is nil.
-                    #
-                    # For Rails Engine routes `#route_uri_pattern` is the route as defined in the engine,
-                    # and `#script_name` is the route prefix at which the engine is mounted.
-                    http_route = req.script_name.to_s + req.route_uri_pattern
+                def find_routes(req)
+                  super do |match, parameters, route|
+                    # non-dispatcher routes are not end routes,
+                    # this could be a route prefix for a rails engine for example
+                    Instrumentation.set_http_route_tags(route.path.spec, req.env['SCRIPT_NAME']) if route&.dispatcher?
 
-                    active_span.set_tag(
-                      Tracing::Metadata::Ext::HTTP::TAG_ROUTE,
-                      Instrumentation.format_http_route(http_route)
-                    )
-                  rescue StandardError => e
-                    Datadog.logger.error(e.message)
+                    yield [match, parameters, route]
                   end
-
-                  response
                 end
               end
             end

lib/datadog/tracing/metadata/ext.rb

@@ -88,6 +88,7 @@ module HTTP
           TAG_USER_AGENT = 'http.useragent'
           TAG_URL = 'http.url'
           TAG_ROUTE = 'http.route'
+          TAG_ROUTE_PATH = 'http.route.path'
           TYPE_INBOUND = AppTypes::TYPE_WEB.freeze
           TYPE_OUTBOUND = 'http'
           TYPE_PROXY = 'proxy'

spec/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation_spec.rb

@@ -4,13 +4,65 @@
 require 'datadog/tracing/contrib/action_pack/action_dispatch/instrumentation'
 
 RSpec.describe Datadog::Tracing::Contrib::ActionPack::ActionDispatch::Instrumentation do
-  describe '::format_http_route' do
-    it 'removes (.:format) part of the route' do
-      expect(described_class.format_http_route('/api/users/:id(.:format)')).to eq('/api/users/:id')
+  describe '::set_http_route_tags' do
+    let(:tracing_enabled) { true }
+
+    before do
+      expect(Datadog::Tracing).to receive(:enabled?).and_return(tracing_enabled)
+    end
+
+    context 'when tracing is disabled' do
+      let(:tracing_enabled) { false }
+
+      it 'sets no tags' do
+        Datadog::Tracing.trace('rack.request') do |_span, trace|
+          described_class.set_http_route_tags('/users/:id', '/auth')
+
+          expect(trace.send(:meta)).not_to have_key('http.route')
+          expect(trace.send(:meta)).not_to have_key('http.route.path')
+        end
+      end
     end
 
-    it 'does not remove optional params from the route' do
-      expect(described_class.format_http_route('/api/users/(:id)')).to eq('/api/users/(:id)')
+    it 'sets http.route and http.route.path tags on existing trace' do
+      Datadog::Tracing.trace('rack.request') do |_span, trace|
+        described_class.set_http_route_tags('/users/:id(.:format)', '/auth')
+
+        expect(trace.send(:meta).fetch('http.route')).to eq('/users/:id')
+        expect(trace.send(:meta).fetch('http.route.path')).to eq('/auth')
+      end
+    end
+
+    it 'sets no http.route.path when script name is nil' do
+      Datadog::Tracing.trace('rack.request') do |_span, trace|
+        described_class.set_http_route_tags('/users/:id(.:format)', nil)
+
+        expect(trace.send(:meta).fetch('http.route')).to eq('/users/:id')
+        expect(trace.send(:meta)).not_to have_key('http.route.path')
+      end
+    end
+
+    it 'sets no tags when route spec is nil' do
+      Datadog::Tracing.trace('rack.request') do |_span, trace|
+        described_class.set_http_route_tags(nil, '/auth')
+
+        expect(trace.send(:meta)).not_to have_key('http.route')
+        expect(trace.send(:meta)).not_to have_key('http.route.path')
+      end
+    end
+
+    it 'does not create new traces when no active trace is present' do
+      described_class.set_http_route_tags('/users/:id', '/auth')
+
+      expect(traces).to be_empty
+    end
+
+    it 'rescues exceptions' do
+      expect(Datadog::Tracing).to receive(:active_trace).and_raise('boom')
+
+      expect(Datadog.logger).to receive(:error).with('boom')
+
+      described_class.set_http_route_tags('/users/:id', '/auth')
     end
   end
 end

spec/datadog/tracing/contrib/action_pack/action_dispatch/journey/router_spec.rb

@@ -20,7 +20,7 @@
     Datadog.registry[:rack].reset_configuration!
   end
 
-  describe '#serve' do
+  describe '#find_routes' do
     before do
       rails_test_application.instance.routes.append do
         namespace :api, defaults: { format: :json } do
@@ -55,23 +55,21 @@ def show
       it 'sets http.route when requesting a known route' do
         get '/api/users/1'
 
-        rack_span = spans.first
+        rack_trace = traces.first
 
-        expect(rack_span).to be_root_span
-        expect(rack_span.name).to eq('rack.request')
-
-        expect(rack_span.get_tag('http.route')).to eq('/api/users/:id')
+        expect(rack_trace.name).to eq('rack.request')
+        expect(rack_trace.send(:meta).fetch('http.route')).to eq('/api/users/:id')
+        expect(rack_trace.send(:meta).fetch('http.route.path')).to be_empty
       end
 
       it 'sets no http.route when requesting an unknown route' do
         get '/nope'
 
-        rack_span = spans.first
-
-        expect(rack_span).to be_root_span
-        expect(rack_span.name).to eq('rack.request')
+        rack_trace = traces.first
 
-        expect(rack_span.tags).not_to have_key('http.route')
+        expect(rack_trace.name).to eq('rack.request')
+        expect(rack_trace.send(:meta)).not_to have_key('http.route')
+        expect(rack_trace.send(:meta)).not_to have_key('http.route.path')
       end
     end
 
@@ -87,7 +85,7 @@ def show
       it 'does not set http.route' do
         get '/api/users/1'
 
-        expect(spans).to be_empty
+        expect(traces).to be_empty
       end
     end
   end