-
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:HTTP header tagging with DD_TRACE_HEADER_TAGS for servers #2935
Conversation
@@ -0,0 +1,55 @@ | |||
# frozen_string_literal: true |
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 code was moved here from lib/datadog/tracing/contrib/sinatra/env.rb
and lib/datadog/tracing/contrib/sinatra/headers.rb
, with some refactoring taking place.
Sinatra already had helpers to perform header matching for Rack-style headers.
Rack was also performing header matching, but in a less organized fashion.
Because the DD_TRACE_HEADER_TAGS
option has to be added to both Rack and Sinatra, this PR merges both header matching implementations into this file as they are the exact same: header matching for Rack-style headers.
d4b3ed2
to
ae2ce15
Compare
Codecov Report
@@ Coverage Diff @@
## fix-en-var #2935 +/- ##
============================================
Coverage 98.09% 98.09%
============================================
Files 1305 1309 +4
Lines 72745 72972 +227
Branches 3357 3367 +10
============================================
+ Hits 71356 71585 +229
+ Misses 1389 1387 -2
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
header_value = headers[header_name] | ||
|
||
[span_tag, header_value] if header_value | ||
end.compact |
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.
Instead of map
and compact
, can we reduce
?
@request_headers.reduce([]) do |array, (header_name, span_tag)|
header_value = headers[header_name]
array << [span_tag, header_value] if header_value
array
end
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'm not sure, do you think the reduce easier to read?
@request_headers.reduce([]) do |array, (header_name, span_tag)|
# Case-insensitive search
header_value = headers[header_name]
array << [span_tag, header_value] if header_value
array
end
# Returns false if this class was explicitly configured | ||
# or left without configuration. | ||
def configured? | ||
!@header_tags.equal?(EMPTY) | ||
end |
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.
We would be able to remove this logic once we have #2970 in master
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.
Oh yeah, I think so! 👍
I'll work on this when both are merged!
@marcotc code LGTM, but I believe the system failures are related to these changes. We should probably investigated those before merging |
Co-authored-by: Gustavo Caso <gustavocaso@gmail.com>
Co-authored-by: Gustavo Caso <gustavocaso@gmail.com>
def initialize(header_tags) | ||
@request_headers = {} | ||
@response_headers = {} | ||
@header_tags = header_tags || EMPTY |
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.
After a01cde6 we can get rid of EMPTY
now 😄
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 suggestion kind of worked, but I need to fix a small thing in Options
for it to work: #2994
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 rebased this PR on that fix, but everything here remains unchanged.
What does this PR do?
This PR adds support for saving the value of HTTP headers as span tags by configuring the environment variable
DD_TRACE_HEADER_TAGS
.This PR covers HTTP servers: Rack and Sinatra in our case.
A follow up PR will cover HTTP clients.
Motivation
Saving HTTP headers as span tags has been supported for Rack and Sinatra, but there was no standard configuration at application-level.
At Datadog, we have the
DD_TRACE_HEADER_TAGS
variable defined across all tracers as the recommended way to configure HTTP header capture.This PR brings the Ruby tracer on par with other languages that already support this configuration.
Also, this is one of the features supported by Remote Configuration.
How to test the change?
rake spec:main
covers the utility methods.Rack and Sinatra test suites cover the integration testing for this feature.