From 6d0f1186cd067a19bdebe5eabe660e50f97f94aa Mon Sep 17 00:00:00 2001 From: Zarir Hamza Date: Thu, 8 Feb 2024 13:37:22 -0500 Subject: [PATCH] rewrites as trace tag --- Rakefile | 2 +- lib/datadog/tracing/contrib/grape/endpoint.rb | 7 ++----- lib/datadog/tracing/contrib/rack/middlewares.rb | 15 +++++++++------ lib/datadog/tracing/contrib/rails/patcher.rb | 6 +++--- lib/datadog/tracing/contrib/sinatra/tracer.rb | 6 +++--- lib/datadog/tracing/metadata/ext.rb | 1 + sig/datadog/tracing/metadata/ext.rbs | 1 + spec/datadog/tracing/contrib/http_route_spec.rb | 6 +++--- 8 files changed, 23 insertions(+), 21 deletions(-) diff --git a/Rakefile b/Rakefile index 11afc6126a8..a29cb33859e 100644 --- a/Rakefile +++ b/Rakefile @@ -383,7 +383,7 @@ namespace :spec do desc '' # "Explicitly hiding from `rake -T`" RSpec::Core::RakeTask.new(:routetest) do |t, args| - t.pattern = 'spec/datadog/tracing/contrib/rack/http_route_spec.rb' + t.pattern = 'spec/datadog/tracing/contrib/http_route_spec.rb' t.rspec_opts = args.to_a.join(' ') end diff --git a/lib/datadog/tracing/contrib/grape/endpoint.rb b/lib/datadog/tracing/contrib/grape/endpoint.rb index bc88ea2118e..3081660ccfb 100644 --- a/lib/datadog/tracing/contrib/grape/endpoint.rb +++ b/lib/datadog/tracing/contrib/grape/endpoint.rb @@ -64,7 +64,6 @@ def endpoint_start_process(_name, _start, _finish, _id, payload) Datadog.logger.error(e.message) end - # rubocop:disable Metrics/AbcSize def endpoint_run(name, start, finish, id, payload) return unless Thread.current[KEY_RUN] @@ -105,9 +104,8 @@ def endpoint_run(name, start, finish, id, payload) span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_METHOD, request_method) span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_URL, path) - if Thread.current.key?(:datadog_http_routing) && Thread.current[:datadog_http_routing].is_a?(Array) - Thread.current[:datadog_http_routing] << [:grape, endpoint.env['SCRIPT_NAME'], integration_route] - end + trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, integration_route) + trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, endpoint.env['SCRIPT_NAME']) ensure span.start(start) span.finish(finish) @@ -115,7 +113,6 @@ def endpoint_run(name, start, finish, id, payload) rescue StandardError => e Datadog.logger.error(e.message) end - # rubocop:enable Metrics/AbcSize def endpoint_start_render(*) return if Thread.current[KEY_RENDER] diff --git a/lib/datadog/tracing/contrib/rack/middlewares.rb b/lib/datadog/tracing/contrib/rack/middlewares.rb index a316780442a..d2811a6517a 100644 --- a/lib/datadog/tracing/contrib/rack/middlewares.rb +++ b/lib/datadog/tracing/contrib/rack/middlewares.rb @@ -102,7 +102,6 @@ def call(env) request_trace = Tracing.active_trace || TraceOperation.new env[Ext::RACK_ENV_REQUEST_SPAN] = request_span - Thread.current[:datadog_http_routing] = [] Datadog::Core::Remote::Tie::Tracing.tag(boot, request_span) @@ -113,9 +112,8 @@ def call(env) # call the rest of the stack status, headers, response = @app.call(env) - if status != 404 && (routed = Thread.current[:datadog_http_routing].last) - last_script_name = routed[1] - last_route = routed[2] + if status != 404 && (last_route = request_trace.get_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE)) + last_script_name = request_trace.get_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH) # If the last_script_name is empty but the env['SCRIPT_NAME'] is NOT empty # then the current rack request was not routed and must be accounted for @@ -128,8 +126,13 @@ def call(env) last_route = env['PATH_INFO'] end - request_span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, last_script_name + last_route) if last_route - Thread.current[:datadog_http_routing] = [] + # Clear the route and route path tags from the request trace to avoid possibility of misplacement + request_trace.clear_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE) + request_trace.clear_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH) + + # Ensure tags are placed in rack.request span as desired + request_span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, last_script_name + last_route) + request_span.clear_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH) end [status, headers, response] diff --git a/lib/datadog/tracing/contrib/rails/patcher.rb b/lib/datadog/tracing/contrib/rails/patcher.rb index b87ee263fbe..e2d147cd0cb 100644 --- a/lib/datadog/tracing/contrib/rails/patcher.rb +++ b/lib/datadog/tracing/contrib/rails/patcher.rb @@ -16,9 +16,9 @@ module JourneyRouterPatch def find_routes(*args) result = super integration_route = result.first[2].path.spec.to_s.gsub(/\(\.:format\)/, '') if result.any? - if Thread.current.key?(:datadog_http_routing) && Thread.current[:datadog_http_routing].is_a?(Array) - Thread.current[:datadog_http_routing] << [:rails, args.first.env['SCRIPT_NAME'], integration_route] - end + request_trace = Tracing.active_trace || TraceOperation.new + request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, integration_route) + request_trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, args.first.env['SCRIPT_NAME']) result end end diff --git a/lib/datadog/tracing/contrib/sinatra/tracer.rb b/lib/datadog/tracing/contrib/sinatra/tracer.rb index 5116b6566aa..e85f3ea6830 100644 --- a/lib/datadog/tracing/contrib/sinatra/tracer.rb +++ b/lib/datadog/tracing/contrib/sinatra/tracer.rb @@ -74,9 +74,9 @@ def route_eval Contrib::Analytics.set_measured(span) _, path = env['sinatra.route'].split(' ', 2) - if Thread.current.key?(:datadog_http_routing) && Thread.current[:datadog_http_routing].is_a?(Array) - Thread.current[:datadog_http_routing] << [:sinatra, env['SCRIPT_NAME'], path] - end + trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, path) + trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, env['SCRIPT_NAME']) + # binding.pry super end end diff --git a/lib/datadog/tracing/metadata/ext.rb b/lib/datadog/tracing/metadata/ext.rb index 3ea239a2c87..a2c944bd1b9 100644 --- a/lib/datadog/tracing/metadata/ext.rb +++ b/lib/datadog/tracing/metadata/ext.rb @@ -87,6 +87,7 @@ module HTTP TAG_CLIENT_IP = 'http.client_ip' HEADER_USER_AGENT = 'User-Agent' TAG_ROUTE = 'http.route' + TAG_ROUTE_PATH = 'http.route.path' # General header functionality module Headers diff --git a/sig/datadog/tracing/metadata/ext.rbs b/sig/datadog/tracing/metadata/ext.rbs index ed4d78e2d2d..f5a236abffc 100644 --- a/sig/datadog/tracing/metadata/ext.rbs +++ b/sig/datadog/tracing/metadata/ext.rbs @@ -43,6 +43,7 @@ module Datadog TAG_BASE_URL: ::String TAG_METHOD: ::String TAG_ROUTE: String + TAG_ROUTE_PATH: String TAG_STATUS_CODE: ::String TAG_USER_AGENT: ::String TAG_URL: ::String diff --git a/spec/datadog/tracing/contrib/http_route_spec.rb b/spec/datadog/tracing/contrib/http_route_spec.rb index 8a48e0cc780..924db954d96 100644 --- a/spec/datadog/tracing/contrib/http_route_spec.rb +++ b/spec/datadog/tracing/contrib/http_route_spec.rb @@ -34,8 +34,8 @@ apps_to_build = apps Rack::Builder.new do - apps_to_build.each do |root, app| - map root do + apps_to_build.each do |route, app| + map route do run app end end @@ -155,7 +155,7 @@ def show expect(span.get_tag('http.route')).to eq('/sinatra/hello/world') - next + break end end end