-
Notifications
You must be signed in to change notification settings - Fork 375
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
Add URL query string quantization to Rack #371
Conversation
a72564b
to
4cfa5d0
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.
couple of things, but I think it's great in the overall!
Configuring URL quantization behavior: | ||
|
||
``` | ||
Datadog.configure do |c| |
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.
nice write up!
lib/ddtrace/contrib/rack/quantize.rb
Outdated
module Datadog | ||
module Contrib | ||
module Rack | ||
# Quantize contains Rack-specic quantization tools. |
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.
specific
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.
facepalm
@@ -104,7 +105,8 @@ def set_request_tags!(request_span, env, status, headers, response, original_env | |||
request_span.set_tag(Datadog::Ext::HTTP::METHOD, env['REQUEST_METHOD']) | |||
end | |||
if request_span.get_tag(Datadog::Ext::HTTP::URL).nil? | |||
request_span.set_tag(Datadog::Ext::HTTP::URL, url) | |||
options = Datadog.configuration[:rack][:quantize] | |||
request_span.set_tag(Datadog::Ext::HTTP::URL, Quantize.format_url(url, options)) |
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.
can we handle any kind of exception here? since it's the first middleware, in case of an issue (i.e. encoding/decoding/filtering problem) would be great to avoid propagating our internal exception. I think it's enough to handle the exception somewhere, and log it as debug()
.
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.
@palazzem are you talking about this tagging function in general? Or specifically the use of quantization here?
it { is_expected.to eq('foo[]=bar&foo[]=baz') } | ||
|
||
context 'that contains encoded braces' do | ||
let(:query_string) { 'foo[]=%5Bbar%5D&foo[]=%5Bbaz%5D' } |
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.
since it's a free function and can be used somewhere else, can we add a test without using URL encoding? so a query string such as value=繋がってて
.
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.
Great idea. I'll add in some more edge case tests around encoding and Unicode characters.
4cfa5d0
to
1d5f17c
Compare
This feature is dependent on #384 and should be rebased and merged after that PR is merged. Do we want to consider this a breaking change? Requires no action on the user's part, but will change their URLs/query strings. |
@delner not really since it doesn't impact the API or anything else. I think it's enough to mention this in our migration guide just to let them know the new behavior. Metrics and anything else is not impacted. |
1d5f17c
to
dacba60
Compare
Adds quantization for Rack URL query string parameters. This behavior is activated by default, but can be configured with: