From 60ceee416e553c935f3c2cd44a09a2ee336fc878 Mon Sep 17 00:00:00 2001 From: Yury Lebedev Date: Thu, 29 Aug 2024 11:50:48 +0200 Subject: [PATCH] Add handling of ambiguous rails routes with constraints --- .../action_dispatch/instrumentation.rb | 18 ++- .../action_dispatch/journey/router_spec.rb | 109 +++++++++++++++++- 2 files changed, 117 insertions(+), 10 deletions(-) diff --git a/lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb b/lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb index 83313a630f1..71f36645f56 100644 --- a/lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb +++ b/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] diff --git a/spec/datadog/tracing/contrib/action_pack/action_dispatch/journey/router_spec.rb b/spec/datadog/tracing/contrib/action_pack/action_dispatch/journey/router_spec.rb index c514b4f9d62..6988c9778b8 100644 --- a/spec/datadog/tracing/contrib/action_pack/action_dispatch/journey/router_spec.rb +++ b/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