Add http.route tag to rails, grape, and sinatra integrations #3849

y9v · 2024-08-19T09:12:01Z

What does this PR do?
This PR adds http.route tag to rack spans for Rails (including Rails Engines), Grape, and Sinatra applications.

Motivation:
Having http.route tag on spans helps grouping requests.

Additional Notes:
In Rails version 7.1 internal ActionDispatch::Journey::Router#find_routes method was changed, which was the reason why the previous implementation had to be rolled back.

This PR fixes this issue by checking action_pack version, and either patching ActionDispatch::Journey::Router#find_routes for Rails versions prior to 7.1, or patching ActionDispatch::Journey::Router#serve and using #route_uri_pattern that gets set on the request object.

How to test the change?
Screenshots from local testing:

pr-commenter · 2024-08-19T09:47:17Z

Benchmarks

Benchmark execution time: 2024-09-18 08:13:40

Comparing candidate commit deaba02 in PR branch add-http-route with baseline commit 8f9f58c in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

🟥 throughput [-0.838op/s; -0.791op/s] or [-11.579%; -10.925%]

codecov-commenter · 2024-08-19T13:16:09Z

Codecov Report

Attention: Patch coverage is 93.79015% with 29 lines in your changes missing coverage. Please review.

Project coverage is 97.83%. Comparing base (8f9f58c) to head (deaba02).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
...ec/datadog/tracing/contrib/rails/support/rails7.rb	81.25%	18 Missing ⚠️
...ec/datadog/tracing/contrib/rack/http_route_spec.rb	90.47%	4 Missing ⚠️
...action_pack/action_dispatch/journey/router_spec.rb	97.82%	3 Missing ⚠️
...ec/datadog/tracing/contrib/rails/support/rails6.rb	84.61%	2 Missing ⚠️
...rib/action_pack/action_dispatch/instrumentation.rb	97.22%	1 Missing ⚠️
lib/datadog/tracing/contrib/grape/endpoint.rb	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3849      +/-   ##
==========================================
- Coverage   97.85%   97.83%   -0.03%     
==========================================
  Files        1285     1291       +6     
  Lines       76951    77394     +443     
  Branches     3789     3845      +56     
==========================================
+ Hits        75302    75716     +414     
- Misses       1649     1678      +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

y9v · 2024-08-19T14:58:48Z

deleted bunch of duplicated comments from the static code analyzer.

vpellan

LGTM !

spec/datadog/tracing/contrib/rails/support/rails7.rb

ivoanjo

Left a few notes!

It wasn't quite clear to me -- do we already have testing for both Rails 7.0 and Rails 7.1?

Also, given their previous work, it may be worth pinging @zarirhamza or @lloeki for a review?

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

y9v · 2024-08-20T11:48:49Z

@ivoanjo thank you for the review!

Regarding Rails 7 tests: we don't have them yet (except system tests for Rails 7.1). The reason for adding rails 7 spec helper was that we are testing action_pack with ActionPack version 7.0.8 and I needed a test rails application to test the router within a rack span.

The fact that we are not testing against version 7.1 and 7.2 yet is the reason why the test coverage is low for this patch.

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

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

lib/datadog/tracing/contrib/sinatra/tracer_middleware.rb

ivoanjo · 2024-08-21T14:33:06Z

Matrixfile

  'action_pack' => {
-    'activesupport' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby',
+    'actionpack-5.0' => '✅ 2.5 / ✅ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ 3.3 / ❌ 3.4 / ✅ jruby',
+    'actionpack-6.0' => '❌ 2.5 / ❌ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ 3.3 / ❌ 3.4 / ✅ jruby',
+    'actionpack-7.0' => '❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ❌ jruby',
+    'actionpack-7.1' => '❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ❌ jruby',


As you might've noticed, each separate appraise that gets added results in an explosion of files to maintain/update.

Since the main dependency needed for these tests is rails, have you considered reusing some of the existing appraisal groups when possible?

E.g. For instance using rails5-mysql2 / rails6-mysql2 and then adding rails7 and rails71?

(Our whole setup for appraisals does help us get coverage but it has a lot of sharp edges and most of us are dreaming of something better lol)

yes, adding appraisals for action_pack was far from being a perfect solution :)

As for reusing existing appraisals - I was not sure if I could just add rails7 or something similar, since all of our existing rails appraisals are bundled together with a database adapter. How do you think, does it makes sense to add appraisals for bare-bones rails?

removed actionpack appraisal and added appraisals for rails7 and rails71 here 5ba0fef

Strech

From my point of view (still lacking context) it's good (with a few questions)

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

lib/datadog/tracing/contrib/rack/middlewares.rb

marcotc · 2024-09-16T19:55:13Z

lib/datadog/tracing/contrib/grape/endpoint.rb

@@ -62,6 +63,15 @@ def endpoint_start_process(_name, _start, _finish, _id, payload)
              span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT)
              span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_ENDPOINT_RUN)

+              if (grape_route = env['grape.routing_args'] && env['grape.routing_args'][:route_info])


Can you add parenthesis here to improve reading clarity in this if condition?

Suggested change

if (grape_route = env['grape.routing_args'] && env['grape.routing_args'][:route_info])

if ((grape_route = env['grape.routing_args']) && grape_route[:route_info])

fixed: 7317018

marcotc · 2024-09-16T19:55:46Z

lib/datadog/tracing/contrib/grape/endpoint.rb

+              if (grape_route = env['grape.routing_args'] && env['grape.routing_args'][:route_info])
+                trace.set_tag(
+                  Tracing::Metadata::Ext::HTTP::TAG_ROUTE,
+                  grape_route.path&.gsub(/\(\.:?\w+\)\z/, '')


Can you add a comment explaining at a high level what we are matching here with /\(\.:?\w+\)\z/?

added c62c24d

marcotc · 2024-09-16T19:58:07Z

lib/datadog/tracing/contrib/rack/middlewares.rb

+              #
+              # To account for the unaccounted nested rack requests of /rack/hello/world,
+              # we use 'PATH_INFO knowing that rack cannot have named parameters
+              if last_script_name == '' && env['SCRIPT_NAME'] != ''


Can env['SCRIPT_NAME'] be nil here, and thus triggering env['SCRIPT_NAME'] != '' to be true?

yeah, I think it actually can. I will add handling for nil here too

marcotc · 2024-09-16T20:00:04Z

spec/datadog/tracing/contrib/rails/support/rails7.rb

@@ -0,0 +1,213 @@
+# Loaded by the `bin/rails` script in a real Rails application


Thank you so much for adding Rails 7 to the test matrix!

marcotc

I left a few minor comments, but it looks great overall!

marcotc · 2024-09-17T18:42:44Z

lib/datadog/tracing/contrib/grape/endpoint.rb

+                puts '=' * 80
+                puts grape_route[:route_info].path
+                puts '=' * 80


oops, thanks! deaba02

y9v requested review from a team as code owners August 19, 2024 09:12

github-actions bot added integrations Involves tracing integrations tracing labels Aug 19, 2024

y9v self-assigned this Aug 19, 2024

y9v changed the title ~~Add http route~~ Add http.route tack tag to rails, grape, and sinatra integrations Aug 19, 2024

y9v force-pushed the add-http-route branch 2 times, most recently from 108627a to 830bd67 Compare August 19, 2024 12:59

DataDog deleted a comment from datadog-datadog-staging-us1-all bot Aug 19, 2024

DataDog deleted a comment from datadog-datadog-prod-us1 bot Aug 19, 2024

DataDog deleted a comment from datadog-datadog-staging-us1-all bot Aug 19, 2024

DataDog deleted a comment from datadog-staging-us1-crawler-test bot Aug 19, 2024

vpellan approved these changes Aug 20, 2024

View reviewed changes

spec/datadog/tracing/contrib/rails/support/rails7.rb Show resolved Hide resolved

ivoanjo reviewed Aug 20, 2024

View reviewed changes

lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb Outdated Show resolved Hide resolved

lib/datadog/tracing/contrib/action_pack/action_dispatch/instrumentation.rb Outdated Show resolved Hide resolved

y9v force-pushed the add-http-route branch from 2e8bae3 to 234cfe3 Compare August 20, 2024 13:34

p-datadog reviewed Aug 20, 2024

View reviewed changes

y9v force-pushed the add-http-route branch 2 times, most recently from a053c22 to 59b5290 Compare August 21, 2024 09:56

y9v requested review from zarirhamza, lloeki, p-datadog and ivoanjo August 21, 2024 11:58

y9v mentioned this pull request Aug 21, 2024

Add specs that were not executed to test:contrib task #3855

Merged

ivoanjo reviewed Aug 21, 2024

View reviewed changes

y9v requested review from a team as code owners September 13, 2024 19:00

github-actions bot added appsec Application Security monitoring product core Involves Datadog core libraries profiling Involves Datadog profiling labels Sep 13, 2024

y9v force-pushed the add-http-route branch from b060bd4 to 3207b16 Compare September 13, 2024 19:03

github-actions bot removed appsec Application Security monitoring product core Involves Datadog core libraries profiling Involves Datadog profiling labels Sep 13, 2024

y9v requested a review from p-datadog September 13, 2024 19:07

y9v and others added 2 commits September 13, 2024 19:34

Update gemfiles/*

81217b9

Change around(:example) to around(:suite) for rack http route test

fedd3e3

Strech approved these changes Sep 16, 2024

View reviewed changes

spec/datadog/tracing/contrib/action_pack/action_dispatch/journey/router_spec.rb Show resolved Hide resolved

p-datadog reviewed Sep 16, 2024

View reviewed changes

lib/datadog/tracing/contrib/rack/middlewares.rb Outdated Show resolved Hide resolved

p-datadog approved these changes Sep 16, 2024

View reviewed changes

marcotc reviewed Sep 16, 2024

View reviewed changes

marcotc approved these changes Sep 16, 2024

View reviewed changes

y9v added 4 commits September 17, 2024 13:26

Fix route path tag in rack, improve grape route instrumentation code

7317018

Add a comment to grape endpoint explaining route format removal

c62c24d

Add handling for possible nil value of env['SCRIPT_NAME']

b72916c

Merge branch 'master' into add-http-route

704eb19

marcotc reviewed Sep 17, 2024

View reviewed changes

Remove debugging code

deaba02

lloeki approved these changes Sep 18, 2024

View reviewed changes

y9v merged commit a9e360b into master Sep 19, 2024
193 checks passed

y9v deleted the add-http-route branch September 19, 2024 06:40

github-actions bot added this to the 2.4.0 milestone Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add http.route tag to rails, grape, and sinatra integrations #3849

Add http.route tag to rails, grape, and sinatra integrations #3849

y9v commented Aug 19, 2024 •

edited

Loading

pr-commenter bot commented Aug 19, 2024 •

edited

Loading

codecov-commenter commented Aug 19, 2024 •

edited

Loading

y9v commented Aug 19, 2024 •

edited

Loading

vpellan left a comment

ivoanjo left a comment

y9v commented Aug 20, 2024 •

edited

Loading

ivoanjo Aug 21, 2024

y9v Aug 21, 2024

y9v Aug 22, 2024 •

edited

Loading

Strech left a comment

marcotc Sep 16, 2024

y9v Sep 17, 2024

marcotc Sep 16, 2024

y9v Sep 17, 2024

marcotc Sep 16, 2024 •

edited

Loading

y9v Sep 17, 2024 •

edited

Loading

marcotc Sep 16, 2024

marcotc left a comment

marcotc Sep 17, 2024

y9v Sep 18, 2024

	if (grape_route = env['grape.routing_args'] && env['grape.routing_args'][:route_info])
	if ((grape_route = env['grape.routing_args']) && grape_route[:route_info])

		@@ -0,0 +1,213 @@
		# Loaded by the `bin/rails` script in a real Rails application

Add http.route tag to rails, grape, and sinatra integrations #3849

Add http.route tag to rails, grape, and sinatra integrations #3849

Conversation

y9v commented Aug 19, 2024 • edited Loading

pr-commenter bot commented Aug 19, 2024 • edited Loading

Benchmarks

scenario:profiler - sample timeline=false

codecov-commenter commented Aug 19, 2024 • edited Loading

Codecov Report

y9v commented Aug 19, 2024 • edited Loading

vpellan left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

y9v commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y9v Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

y9v Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y9v commented Aug 19, 2024 •

edited

Loading

pr-commenter bot commented Aug 19, 2024 •

edited

Loading

codecov-commenter commented Aug 19, 2024 •

edited

Loading

y9v commented Aug 19, 2024 •

edited

Loading

y9v commented Aug 20, 2024 •

edited

Loading

y9v Aug 22, 2024 •

edited

Loading

marcotc Sep 16, 2024 •

edited

Loading

y9v Sep 17, 2024 •

edited

Loading