-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use full URL in http.url
tag
#2265
Conversation
353e405
to
952447d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wait on this pending internal discussion. Thanks!
952447d
to
7b7e4d4
Compare
- 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
7b7e4d4
to
3b2c316
Compare
This is now made optional via a quantization option.
This is now only dropped when
This fixes a discrepancy between the happy path and the fallback.
This is now removed from the PR. |
Save for one test case which was adjusted for an inconsistency that is now resolved, the behaviour does not change, thus 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.
84810ac
to
ec68ba1
Compare
@@ -74,7 +74,7 @@ | |||
spans.each do |span| | |||
if span.name == Datadog::Tracing::Contrib::Rack::Ext::SPAN_REQUEST | |||
expect(span.resource).to eq('GET /endpoint') | |||
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::HTTP::TAG_URL)).to eq('/endpoint') | |||
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::HTTP::TAG_URL)).to eq('/one/endpoint') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why these Sinatra tests have changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
**Configuring URL quantization behavior** | ||
|
||
```ruby | ||
Datadog.configure do |c| | ||
# Default behavior: all values are quantized, fragment is removed. | ||
# http://example.com/path?category_id=1&sort_by=asc#featured --> http://example.com/path?category_id&sort_by | ||
# http://example.com/path?categories[]=1&categories[]=2 --> http://example.com/path?categories[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironically, the doc here was consistent with the internal Quantization::HTTP.url
but not with the user-facing Rack integration http.url
tagging behaviour!
@@ -23,6 +23,8 @@ module Rack | |||
# in the Rack environment so that it can be retrieved by the underlying | |||
# application. If request tags are not set by the app, they will be set using | |||
# information available at the Rack level. | |||
# | |||
# rubocop:disable Metrics/ClassLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there are not too many of them, so it can reasonably fit into this PR. Will do.
options = configuration[:quantize] || {} | ||
|
||
# Quantization::HTTP.url base defaults to :show, but we are transitioning | ||
options[:base] ||= :exclude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, I suggest adding the comment in this line with the prefix # DEV-2.0:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points! Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll do that in another PR, along with this suggestion: #2283 (comment)
@@ -47,12 +47,24 @@ | |||
it { is_expected.to eq('http://example.com/path') } | |||
end | |||
|
|||
context 'with show: :all' do | |||
context 'with fragment: :show' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hmm there's flakiness still:
Had one like that before, seems like EDIT: confirmed leakage!
|
e5b57f2
to
a5ff117
Compare
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks 👍 after the latest changes.
In practice this cop seems to be disabled as soon as we reach the limit.
All good, merging 🎉 |
Hi @lloeki. Thanks for the fix. I'm testing dd-trace 1.5.1 and still see the same issue unfortunately.
|
Thanks @gingerlime, it seems the line triggering this comes from the other PR #2310
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. |
@lloeki yeah, sorry, I wasn't sure where to post this. I already linked to this comment on the other PR though. |
What does this PR do?
Adjust HTTP URL tags to comply with span tag unification RFC
complete request URL
Enable query string quantization by default
This change is mandated by the unifying tag RFC.
Motivation
Compliance with internal RFC document.