-
Notifications
You must be signed in to change notification settings - Fork 441
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
Fix and Feature: standardize config behavior across tracing libs #2873
Conversation
BenchmarksBenchmark execution time: 2024-10-03 19:34:39 Comparing candidate commit 09b6d5d in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 56 metrics, 1 unstable metrics. scenario:BenchmarkInjectW3C-24
scenario:BenchmarkSetTagStringer-24
|
9c829ae
to
48ea87b
Compare
…4xx codes, not 5xx
2f959a7
to
4bd670f
Compare
8912f64
to
72c22f6
Compare
72c22f6
to
8d242b8
Compare
0989e3d
to
c7e46fe
Compare
Small note: Only overwrite 0 -> 200 code if 0 was not specified in DD_TRACE_HTTP_SERVER_ERROR_STATUSES
c7e46fe
to
ca9fedd
Compare
57de51d
to
bb3adb9
Compare
bb3adb9
to
44a216a
Compare
44a216a
to
767938c
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.
Approving for ˋinternal/appsec` files (Thanks for the fix 😄)
contrib/net/http/roundtripper.go
Outdated
// treat 5XX as errors | ||
if res.StatusCode/100 == 5 { | ||
// Client spans consider 4xx as errors | ||
if res.StatusCode/100 == 4 { |
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.
Isn't this a breaking behavioural change? Should we introduce an environment variable to allow keeping the old behaviour or would it be enough to set DD_TRACE_HTTP_CLIENT_ERROR_STATUSES
to the right values?
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.
@darccio , yes it is a breaking change, although in my opinion it's a "fix" as the http client spans should always have had 4xx as errors, not 5xx.
We can discuss further.
contrib/net/http/http_test.go
Outdated
@@ -516,11 +516,11 @@ func router(muxOpts ...Option) http.Handler { | |||
return mux | |||
} | |||
|
|||
func handler200(w http.ResponseWriter, _ *http.Request) { | |||
func handler200(w http.ResponseWriter, r *http.Request) { |
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.
In the body of the function this variable isn't used. Is it needed due to another reason?
Description
This change makes Go "consistent" with other tracing libraries for a given list of configurations; for some configs, the behavior has been changed; for others, support is introduced for the first time. RFC is linked under "Motivation." This is not an exhaustive list.
Also, adds telemetry report for
trace_rate_limit
.Each change is in its own commit for ease of review, except for the very last commit, which just does some cleanup.
Telemetry changes: Commit
Configurations made consistent:
(This means they already existed in dd-trace-go, but their behavior was modified):
DD_TRACE_CLIENT_IP_ENABLED
is set to true, we report ahttp.client_ip
tag whose value is populated from a list of "default headers." This PR fixes this list to includecf-connecting-ipv6
instead ofcf-connecting-ip6
(The former is correct according to the spec). CommitConfigurations introduced:
DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING
: Boolean that determines whether or not to include query params in http.url tag for http client spans. CommitDD_TRACE_HTTP_CLIENT_ERROR_STATUSES
: Customize errors on http client spans. CommitDD_TRACE_HTTP_CLIENT_ERROR_STATUSES
: Customize errors on http server spans. CommitNote: support for
DD_TRACE_LOG_DIRECTORY
is also added as part of this effort, but for ease of review, the changes are in a separate PR since the implementation was slightly more complex.Motivation
RFC
Ensure behavior is consistent across tracing libs for a given list of configurations.
Reviewer's Checklist
Unsure? Have a question? Request a review!