Use full URL in `http.url` tag by lloeki · Pull Request #2265 · DataDog/dd-trace-rb

lloeki · 2022-09-09T15:17:09Z

What does this PR do?

Adjust HTTP URL tags to comply with span tag unification RFC

align http.url with tag naming RFC (and Rack's #url) to include the
complete request URL
drop http.base_url, now unneeded
preserve REQUEST_URI logic that differs from Rack's logic
add SCRIPT_NAME which was previously unhandled for PATH_INFO
append QUERY_STRING when using PATH_INFO to match REQUEST_URI

Enable query string quantization by default

This change is mandated by the unifying tag RFC.

Motivation

Compliance with internal RFC document.

delner

Please wait on this pending internal discussion. Thanks!

- add a { base: :show } quantization option to enable the behaviour - align http.url with tag naming RFC (and Rack's #url) to include the complete request URL - drop http.base_url when not unneeded - preserve REQUEST_URI logic that differs from Rack's logic - add SCRIPT_NAME which was previously unhandled for PATH_INFO - append QUERY_STRING when using PATH_INFO to match REQUEST_URI

lloeki · 2022-09-13T14:56:44Z

Adjust HTTP URL tags to comply with span tag unification RFC

This is now made optional via a quantization option.

drop http.base_url, now unneeded

This is now only dropped when http.url includes the URL base.

append QUERY_STRING when using PATH_INFO to match REQUEST_URI

This fixes a discrepancy between the happy path and the fallback.

Enable query string quantization by default

This is now removed from the PR.

lloeki · 2022-09-13T14:58:54Z

Save for one test case which was adjusted for an inconsistency that is now resolved, the behaviour does not change, thus rake spec:rack tests pass right away.

I shall now add a couple of test cases to validate the new behaviour when the option is set.

Note: there seems to be leakage between spec cases, which made this test case flaky, with quantization option being set to show foo (example seed: 63620). The test case was previously unaffected by this leakage because it could not handle query strings.

Rack http.url tag now properly takes SCRIPT_NAME into account in the PATH_INFO case, consistently with REQUEST_URI. These expectations did not account for that.

marcotc · 2022-09-15T16:59:57Z

Could you explain why these Sinatra tests have changed?

These tags actually come from Rack, not Sinatra. I believe they're here because at some point Sinatra may have been backfilling this value by itself. In any case they act as an integration test checking for side effects.

Sadly this test would previously pass but not represent what happens when real-world REQUEST_URI is set, indeed Rack::Test does not set REQUEST_URI.

The change here is mainly due to a bug that this PR addresses, that previously made the fallback to PATH_INFO be inconsistent with REQUEST_URI (i.e it did not include SCRIPT_NAME nor QUERY_STRING), which created all kinds of special cases from the lack of query string to adding base_url tag + url tag not resulting in the same value depending on whether REQUEST_URI or PATH_INFO was used, not the least in the latter case base_url tag + url tag is entirely nonsensical for virtualhosted or nested apps since it misses the intermediate SCRIPT_NAME.

lloeki · 2022-09-16T10:46:40Z

Ironically, the doc here was consistent with the internal Quantization::HTTP.url but not with the user-facing Rack integration http.url tagging behaviour!

lloeki · 2022-09-16T10:48:40Z

I can get the argument for method length (with exceptions), but I'm wondering if there's really such a thing as a class with "too much lines".

I don't think I've ever decided to break down a class when Metrics/ClassLength is triggered, only ever added the cop exclusion tag.

If you want to remove the cop, I'd ✅ it.

Seems like there are not too many of them, so it can reasonably fit into this PR. Will do.

lloeki · 2022-09-16T10:51:46Z

This has to be done here, as doing it as a config default would be silently overridden when :quantize is manually set, which would be most surprising to the user.

Probably a general problem with such nested configuration keys.

If I understand it correctly, this will be removed in the next major release, is that right?

If so, I suggest writing them down in the comment, so we can find it later.

Actually, can you create a new file named docs/NextReleaseUpgradeGuide.md (name suggestion only), just add one section:

# Breaking changes To aid with the next release process, please tag future breaking changes the inline code comment `# DEV-2.0:` prefix. ## List of breaking changes * `http.url` will change its default...

We've been meaning to do it, and it seems like your PR wants to have a breaking change in the next release.

With this, I suggest adding the comment in this line with the prefix # DEV-2.0:.

Good points! Will do.

I think I'll do that in another PR, along with this suggestion: #2283 (comment)

lloeki · 2022-09-16T10:53:47Z

This seemed to be a typo/eager copy-pasting.

- restore the previous default behaviour of `Quantization::HTTP.url`. - also, make an effort to keep the Rack integration default as not preserving the URL base, restricting the transition code for the upcoming breaking change to that integration.

lloeki · 2022-09-16T11:11:36Z

Hmm there's flakiness still:

# pass
bundle exec appraisal ruby-3.1.1-contrib rake spec:rack['--seed 39851']
# fail
bundle exec appraisal ruby-3.1.1-contrib rake spec:rack['--seed 17324']

Had one like that before, seems like quantize: config is leaking from one spec example to the next.

EDIT: confirmed leakage!

$ bundle exec appraisal ruby-3.1.1-contrib rake spec:rack['--seed 17324']
[1] pry(#<RSpec::ExampleGroups::RackIntegrationTests::ForAnApplication::WithABasicRoute::POSTRequest::WithoutParameters>)> Datadog.configuration.tracing[:rack][:quantize]
=> {:base=>:show}
[2] pry(#<RSpec::ExampleGroups::RackIntegrationTests::ForAnApplication::WithABasicRoute::POSTRequest::WithoutParameters>)> rack_options
=> {}

codecov-commenter · 2022-09-16T11:51:10Z

Codecov Report

Merging #2265 (a5ff117) into master (0e92706) will increase coverage by 0.00%.
The diff coverage is 99.49%.

@@           Coverage Diff            @@
##           master    #2265    +/-   ##
========================================
  Coverage   97.56%   97.57%            
========================================
  Files        1087     1093     +6     
  Lines       56965    57328   +363     
========================================
+ Hits        55580    55939   +359     
- Misses       1385     1389     +4

Impacted Files	Coverage Δ
.../datadog/tracing/contrib/rack/header_collection.rb	`93.75% <93.75%> (ø)`
lib/datadog/tracing/contrib/rack/middlewares.rb	`99.20% <97.67%> (+0.16%)`	⬆️
lib/datadog/core/configuration/settings.rb	`98.94% <100.00%> (+0.04%)`	⬆️
lib/datadog/core/header_collection.rb	`100.00% <100.00%> (ø)`
lib/datadog/tracing/client_ip.rb	`100.00% <100.00%> (ø)`
lib/datadog/tracing/configuration/ext.rb	`100.00% <100.00%> (ø)`
...datadog/tracing/contrib/utils/quantization/http.rb	`92.06% <100.00%> (+1.49%)`	⬆️
lib/datadog/tracing/metadata/ext.rb	`100.00% <100.00%> (ø)`
spec/datadog/core/header_collection_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/tracing/client_ip_spec.rb	`100.00% <100.00%> (ø)`
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

marcotc

Looks 👍 after the latest changes.

In practice this cop seems to be disabled as soon as we reach the limit.

lloeki · 2022-09-23T11:25:18Z

Alright, all comments addressed (thanks @marcotc!), waiting for CI.

@delner I'll proceed by dismissing your review since the concern has been discussed internally and addressed.

Discussed internally, adressed.

lloeki · 2022-09-23T11:47:31Z

All good, merging 🎉

gingerlime · 2022-10-20T05:46:19Z

Hi @lloeki. Thanks for the fix. I'm testing dd-trace 1.5.1 and still see the same issue unfortunately.

root@host:/var/local/app/app/dependabot/bundler/ddtrace-1.5.1 %)# curl "http://localhost:3000/|"

Puma caught this error: bad URI(is not URI?): "/|" (URI::InvalidURIError)
/usr/local/lib/ruby/3.0.0/uri/rfc3986_parser.rb:67:in `split'
/usr/local/lib/ruby/3.0.0/uri/rfc3986_parser.rb:72:in `parse'
/usr/local/lib/ruby/3.0.0/uri/rfc3986_parser.rb:106:in `convert_to_uri'
/usr/local/lib/ruby/3.0.0/uri/generic.rb:1100:in `merge'
/usr/local/lib/ruby/3.0.0/uri/rfc3986_parser.rb:78:in `inject'
/usr/local/lib/ruby/3.0.0/uri/rfc3986_parser.rb:78:in `join'
/usr/local/lib/ruby/3.0.0/uri/common.rb:208:in `join'
/usr/local/bundle/gems/ddtrace-1.5.1/lib/datadog/tracing/contrib/rack/middlewares.rb:274:in `parse_url'
/usr/local/bundle/gems/ddtrace-1.5.1/lib/datadog/tracing/contrib/rack/middlewares.rb:167:in `set_request_tags!'
/usr/local/bundle/gems/ddtrace-1.5.1/lib/datadog/tracing/contrib/rack/middlewares.rb:110:in `call'
/usr/local/bundle/gems/rack-mini-profiler-3.0.0/lib/mini_profiler/profiler.rb:249:in `call'
/usr/local/bundle/gems/railties-7.0.4/lib/rails/engine.rb:530:in `call'
/usr/local/bundle/gems/puma-5.6.5/lib/puma/configuration.rb:252:in `call'
/usr/local/bundle/gems/puma-5.6.5/lib/puma/request.rb:77:in `block in handle_request'
/usr/local/bundle/gems/puma-5.6.5/lib/puma/thread_pool.rb:340:in `with_force_shutdown'
/usr/local/bundle/gems/puma-5.6.5/lib/puma/request.rb:76:in `handle_request'
/usr/local/bundle/gems/puma-5.6.5/lib/puma/server.rb:443:in `process_client'
/usr/local/bundle/gems/puma-5.6.5/lib/puma/thread_pool.rb:147:in `block in spawn_thread'

lloeki · 2022-10-20T13:59:04Z

Thanks @gingerlime, it seems the line triggering this comes from the other PR #2310

# /usr/local/bundle/gems/ddtrace-1.5.1/lib/datadog/tracing/contrib/rack/middlewares.rb:274:in `parse_url'
::URI.join(base_url, fullpath).to_s

There are specs supposed to cover the triggering case and prevent a regression but it seems they did not trigger (maybe they don't mimic puma+rack enough). I'll look into it.

gingerlime · 2022-10-20T14:50:10Z

@lloeki yeah, sorry, I wasn't sure where to post this. I already linked to this comment on the other PR though.

lloeki force-pushed the use-full-url-in-http-url-tag branch from 353e405 to 952447d Compare September 9, 2022 15:18

delner previously requested changes Sep 9, 2022

View reviewed changes

lloeki force-pushed the use-full-url-in-http-url-tag branch from 952447d to 7b7e4d4 Compare September 13, 2022 14:51

lloeki force-pushed the use-full-url-in-http-url-tag branch from 7b7e4d4 to 3b2c316 Compare September 13, 2022 14:53

lloeki added 2 commits September 13, 2022 22:04

Adjust Sinatra expectations

ec68ba1

Rack http.url tag now properly takes SCRIPT_NAME into account in the PATH_INFO case, consistently with REQUEST_URI. These expectations did not account for that.

lloeki force-pushed the use-full-url-in-http-url-tag branch from 84810ac to ec68ba1 Compare September 13, 2022 20:11

lloeki marked this pull request as ready for review September 15, 2022 10:41

lloeki requested a review from a team September 15, 2022 10:41

marcotc reviewed Sep 15, 2022

View reviewed changes

Comment thread lib/datadog/tracing/contrib/utils/quantization/http.rb Outdated

lloeki commented Sep 16, 2022

View reviewed changes

lloeki added 3 commits September 16, 2022 13:07

Silence Rubocop for this class

7d4c742

Fix incorrect spec example title

337ecb7

lloeki added 2 commits September 16, 2022 13:35

Prevent configuration leakage for Rack spec

5bbbffb

Remove unneeded super calls

a5ff117

lloeki force-pushed the use-full-url-in-http-url-tag branch from e5b57f2 to a5ff117 Compare September 16, 2022 11:36

marcotc reviewed Sep 21, 2022

View reviewed changes

Comment thread docs/GettingStarted.md

marcotc approved these changes Sep 22, 2022

View reviewed changes

lloeki mentioned this pull request Sep 23, 2022

Add query string automatic redaction #2283

Merged

Disable Metrics/ClassLength cop

8c848ec

In practice this cop seems to be disabled as soon as we reach the limit.

lloeki merged commit 6970088 into master Sep 23, 2022

lloeki deleted the use-full-url-in-http-url-tag branch September 23, 2022 11:47

github-actions Bot added this to the 1.5.0 milestone Sep 23, 2022

yujideveloper mentioned this pull request Oct 12, 2022

Fix URI::InvalidURIError #2310

Merged

lloeki mentioned this pull request Oct 14, 2022

Handle URLs with invalid characters #2311

Merged

Uh oh!

Conversation

lloeki commented Sep 9, 2022

Uh oh!

delner left a comment

Choose a reason for hiding this comment

Uh oh!

lloeki commented Sep 13, 2022

Uh oh!

lloeki commented Sep 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lloeki Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lloeki Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lloeki commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 16, 2022

Codecov Report

Uh oh!

Uh oh!

marcotc left a comment

Choose a reason for hiding this comment

Uh oh!

lloeki commented Sep 23, 2022

Uh oh!

lloeki commented Sep 23, 2022

Uh oh!

gingerlime commented Oct 20, 2022

Uh oh!

lloeki commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gingerlime commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lloeki Sep 16, 2022 •

edited

Loading

lloeki Sep 22, 2022 •

edited

Loading

lloeki commented Sep 16, 2022 •

edited

Loading

lloeki commented Oct 20, 2022 •

edited

Loading