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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
github-actions
Bot
added
the
apm:ecosystem
mtoffl01
changed the title
bb3adb9 to
44a216a
Compare
44a216a to
767938c
Compare
eliottness
left a comment
eliottness
left a comment
There was a problem hiding this comment.
Approving for ˋinternal/appsec` files (Thanks for the fix 😄)
| // 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.
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.
@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.
| } | ||
|
|
||
| func handler200(w http.ResponseWriter, _ *http.Request) { | ||
| func handler200(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
In the body of the function this variable isn't used. Is it needed due to another reason?
4 participants
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_ENABLEDis set to true, we report ahttp.client_iptag whose value is populated from a list of "default headers." This PR fixes this list to includecf-connecting-ipv6instead 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_DIRECTORYis 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!