Add handling of ambiguous rails routes with constraints

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

@@ -20,7 +20,10 @@ def set_http_route_tags(route_spec, script_name)
               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
+
+              if script_name && !script_name.empty?
+                request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, script_name)
+              end
             rescue StandardError => e
               Datadog.logger.error(e.message)
             end
@@ -36,9 +39,12 @@ def find_routes(req)
                   routes = result.map(&:last)
 
                   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?
+                    # we only want to set route tags for dispatcher routes,
+                    # since they instantiate controller that processes the request
+                    if route&.dispatcher?
+                      Instrumentation.set_http_route_tags(route.path.spec, req.env['SCRIPT_NAME'])
+                      break
+                    end
                   end
 
                   result
@@ -50,8 +56,8 @@ def find_routes(req)
               module LazyRouter
                 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
+                    # we only want to set route tags for dispatcher routes,
+                    # since they instantiate controller that processes the request
                     Instrumentation.set_http_route_tags(route.path.spec, req.env['SCRIPT_NAME']) if route&.dispatcher?
 
                     yield [match, parameters, route]

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

@@ -22,16 +22,29 @@
 
   describe '#find_routes' do
     before do
+      engine.routes.append do
+        post '/sign-in/(:expires_in)' => 'tokens#create'
+      end
+
+      auth_engine = engine
+
       rails_test_application.instance.routes.append do
         namespace :api, defaults: { format: :json } do
           resources :users, only: %i[show]
+
+          mount auth_engine => '/auth'
         end
+
+        get '/items/:id', to: 'items#by_id', id: /\d+/
+        get '/items/:slug', to: 'items#by_slug', id: /(\w-)+/
+
+        get 'books/*section/:title', to: 'books#show'
       end
     end
 
-    let(:controllers) { [controller] }
+    let(:controllers) { [users_controller, items_controller, books_controller] }
 
-    let(:controller) do
+    let(:users_controller) do
       stub_const(
         'Api::UsersController',
         Class.new(ActionController::Base) do
@@ -42,6 +55,54 @@ def show
       )
     end
 
+    let(:items_controller) do
+      stub_const(
+        'ItemsController',
+        Class.new(ActionController::Base) do
+          def by_id
+            head :ok
+          end
+
+          def by_slug
+            head :ok
+          end
+        end
+      )
+    end
+
+    let(:books_controller) do
+      stub_const(
+        'BooksController',
+        Class.new(ActionController::Base) do
+          def show
+            head :ok
+          end
+        end
+      )
+    end
+
+    let(:status_controller) do
+      stub_const(
+        'StatusesController',
+        Class.new(ActionController::Base) do
+          def show
+            head :ok
+          end
+        end
+      )
+    end
+
+    let(:engine) do
+      stub_const('AuthEngine', Module.new)
+
+      stub_const(
+        'AuthEngine::Engine',
+        Class.new(::Rails::Engine) do
+          isolate_namespace AuthEngine
+        end
+      )
+    end
+
     context 'with default configuration' do
       before do
         Datadog.configure do |c|
@@ -59,10 +120,50 @@ def show
 
         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
+        expect(rack_trace.send(:meta)).not_to have_key('http.route.path')
+      end
+
+      it 'sets http.route correctly for ambiguous route with constraints' do
+        get '/items/1'
+
+        rack_trace = traces.first
+
+        expect(rack_trace.name).to eq('rack.request')
+        expect(rack_trace.send(:meta).fetch('http.route')).to eq('/items/:id')
+        expect(rack_trace.send(:meta)).not_to have_key('http.route.path')
+      end
+
+      it 'sets http.route correctly for ambiguous route with constraints, case two' do
+        get '/items/something'
+
+        rack_trace = traces.first
+
+        expect(rack_trace.name).to eq('rack.request')
+        expect(rack_trace.send(:meta).fetch('http.route')).to eq('/items/:slug')
+        expect(rack_trace.send(:meta)).not_to have_key('http.route.path')
+      end
+
+      it 'sets http.route correctly for routes with globbing' do
+        get 'books/some/section/title'
+
+        rack_trace = traces.first
+
+        expect(rack_trace.name).to eq('rack.request')
+        expect(rack_trace.send(:meta).fetch('http.route')).to eq('/books/*section/:title')
+        expect(rack_trace.send(:meta)).not_to have_key('http.route.path')
+      end
+
+      it 'sets http.route and http.route.path for rails engine routes' do
+        post '/api/auth/sign-in'
+
+        rack_trace = traces.first
+
+        expect(rack_trace.name).to eq('rack.request')
+        expect(rack_trace.send(:meta).fetch('http.route')).to eq('/sign-in(/:expires_in)')
+        expect(rack_trace.send(:meta).fetch('http.route.path')).to eq('/api/auth')
       end
 
-      it 'sets no http.route when requesting an unknown route' do
+      it 'does not set http.route when requesting an unknown route' do
         get '/nope'
 
         rack_trace = traces.first