-
Notifications
You must be signed in to change notification settings - Fork 443
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
contrib/http/net: adding integration-level option to configure error codes #3012
base: main
Are you sure you want to change the base?
Conversation
BenchmarksBenchmark execution time: 2025-01-08 15:46:21 Comparing candidate commit 031dbfc in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 0 unstable metrics. |
Co-authored-by: Mikayla Toffler <[email protected]>
Datadog ReportBranch report: ✅ 0 Failed, 5124 Passed, 70 Skipped, 2m 35.95s Total Time |
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.
System test is failing because, with your changes, DD_TRACE_HTTP_SERVER_ERROR_STATUSES
no longer applies to net/http.
When set, DD_TRACE_HTTP_SERVER_ERROR_STATUSES
should impact all http integrations — this is kind of best effort, but for the purposes of this integration, let's say DD_TRACE_HTTP_SERVER_ERROR_STATUSES
should impact all integrations that use contrib/internal/httptrace
to start/finish spans.
However, if both DD_TRACE_HTTP_SERVER_ERROR_STATUSES
and integration-level config WithStatusCheck
are set, the former loses its effect and only the latter applies.
Make sure the system test passes by making sure DD_TRACE_HTTP_SERVER_ERROR_STATUSES
applies to the net/http integration when WithStatusCheck
has not been called. Take a look at contrib/go-chi for hints.
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.
LGTM, but make sure the PR title conforms to Go standards: https://github.com/DataDog/dd-trace-go/blob/main/CONTRIBUTING.md#contributing
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.
LGTM
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.
Tiny nit, but really good work, good to merge!
What does this PR do?
Closes #2882
Users want a way to configure error codes for http server spans generated from the net/http integration. If configured, these codes are the only codes that should be considered errors on relevant spans.
Motivation
Reviewer's Checklist
v2-dev
branch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!