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.
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.
marcotc
force-pushed
the
DD_TRACE_HEADER_TAGS
branch
from
d4b3ed2
to
ae2ce15
Compare
marcotc
changed the title
marcotc
changed the title
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.
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.
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.
We would be able to remove this logic once we have #2970 in master
There was a problem hiding this comment.
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.
After a01cde6 we can get rid of EMPTY now 😄
There was a problem hiding this comment.
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.
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_TAGSvariable 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:maincovers the utility methods.Rack and Sinatra test suites cover the integration testing for this feature.