From ad4dcc24be1c251d4717b7709dfc81d52bfad643 Mon Sep 17 00:00:00 2001 From: Yury Lebedev Date: Wed, 28 Aug 2024 18:01:35 +0200 Subject: [PATCH] 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. --- .../action_dispatch/instrumentation.rb | 78 +++++++------------ lib/datadog/tracing/metadata/ext.rb | 1 + .../action_dispatch/instrumentation_spec.rb | 62 +++++++++++++-- .../action_dispatch/journey/router_spec.rb | 22 +++--- 4 files changed, 94 insertions(+), 69 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 0faf9c124ae..83313a630f1 100644 --- a/lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb +++ b/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 diff --git a/lib/datadog/tracing/metadata/ext.rb b/lib/datadog/tracing/metadata/ext.rb index c32f4f41d72..95cf3d4e7f0 100644 --- a/lib/datadog/tracing/metadata/ext.rb +++ b/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' diff --git a/spec/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation_spec.rb b/spec/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation_spec.rb index bfd10f52aae..47d55570eb2 100644 --- a/spec/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation_spec.rb +++ b/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 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 95a7d95f8eb..c514b4f9d62 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 @@ -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