Merge pull request #4688 from DataDog/appsec-57809-make-http-route-available-within-request

[NO-TICKET] Improve tracing Rails route computation

Author: TonyCTHsu

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

@@ -9,20 +9,23 @@ module ActionPack
         module ActionDispatch
           # Instrumentation for ActionDispatch components
           module Instrumentation
+            SCRIPT_NAME_KEY = 'SCRIPT_NAME'
+            FORMAT_SUFFIX = '(.:format)'
+
             module_function
 
-            def set_http_route_tags(route_spec, script_name)
+            def set_http_route_tags(route_spec, route_path)
               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, route_spec)
 
-              if script_name && !script_name.empty?
-                request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, script_name)
+              if route_path && !route_path.empty?
+                request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, route_path)
               end
             end
 
@@ -40,16 +43,17 @@ module Journey
               # Instrumentation for ActionDispatch::Journey::Router for Rails versions older than 7.1
               module Router
                 def find_routes(req)
+                  # result is an array of [match, parameters, route] tuples
                   result = super
+                  result.each do |_, _, route|
+                    next unless Instrumentation.dispatcher_route?(route)
 
-                  # result is an array of [match, parameters, route] tuples
-                  routes = result.map(&:last)
+                    http_route = route.path.spec.to_s
+                    http_route.delete_suffix!(FORMAT_SUFFIX)
 
-                  routes.each do |route|
-                    if Instrumentation.dispatcher_route?(route)
-                      Instrumentation.set_http_route_tags(route.path.spec, req.env['SCRIPT_NAME'])
-                      break
-                    end
+                    Instrumentation.set_http_route_tags(http_route, req.env[SCRIPT_NAME_KEY])
+
+                    break
                   end
 
                   result
@@ -62,7 +66,10 @@ module LazyRouter
                 def find_routes(req)
                   super do |match, parameters, route|
                     if Instrumentation.dispatcher_route?(route)
-                      Instrumentation.set_http_route_tags(route.path.spec, req.env['SCRIPT_NAME'])
+                      http_route = route.path.spec.to_s
+                      http_route.delete_suffix!(FORMAT_SUFFIX)
+
+                      Instrumentation.set_http_route_tags(http_route, req.env[SCRIPT_NAME_KEY])
                     end
 
                     yield [match, parameters, route]

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

@@ -4,15 +4,9 @@
 require 'datadog/tracing/contrib/action_pack/action_dispatch/instrumentation'
 
 RSpec.describe Datadog::Tracing::Contrib::ActionPack::ActionDispatch::Instrumentation do
-  describe '::set_http_route_tags' do
-    let(:tracing_enabled) { true }
-
-    before do
-      expect(Datadog::Tracing).to receive(:enabled?).and_return(tracing_enabled)
-    end
-
+  describe '.set_http_route_tags' do
     context 'when tracing is disabled' do
-      let(:tracing_enabled) { false }
+      before { expect(Datadog::Tracing).to receive(:enabled?).and_return(false) }
 
       it 'sets no tags' do
         Datadog::Tracing.trace('rack.request') do |_span, trace|
@@ -24,37 +18,41 @@
       end
     end
 
-    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')
+    context 'when tracing is enabled' do
+      before { expect(Datadog::Tracing).to receive(:enabled?).and_return(true) }
 
-        expect(trace.send(:meta).fetch('http.route')).to eq('/users/:id')
-        expect(trace.send(:meta).fetch('http.route.path')).to eq('/auth')
+      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', '/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
-    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)
+      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', nil)
 
-        expect(trace.send(:meta).fetch('http.route')).to eq('/users/:id')
-        expect(trace.send(:meta)).not_to have_key('http.route.path')
+          expect(trace.send(:meta).fetch('http.route')).to eq('/users/:id')
+          expect(trace.send(:meta)).not_to have_key('http.route.path')
+        end
       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')
+      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')
+          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 create new traces when no active trace is present' do
-      described_class.set_http_route_tags('/users/:id', '/auth')
+      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
+        expect(traces).to be_empty
+      end
     end
   end
 end