Skip to content

contrib/http/net: adding integration-level option to configure error codes#3012

Merged
mtoffl01 merged 13 commits into
mainfrom
rachel.yang/http-net-error-codes
Jan 13, 2025
Merged

contrib/http/net: adding integration-level option to configure error codes#3012
mtoffl01 merged 13 commits into
mainfrom
rachel.yang/http-net-error-codes

Conversation

@rachelyangdog

@rachelyangdog rachelyangdog commented Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.
  • For internal contributors, a matching PR should be created to the v2-dev branch and reviewed by @DataDog/apm-go.

Unsure? Have a question? Request a review!

@rachelyangdog
rachelyangdog requested review from a team as code owners December 5, 2024 20:57
@rachelyangdog
rachelyangdog marked this pull request as draft December 5, 2024 20:57
@github-actions github-actions Bot added the apm:ecosystem contrib/* related feature requests or bugs label Dec 5, 2024
@pr-commenter

pr-commenter Bot commented Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-01-08 15:46:21

Comparing candidate commit 031dbfc in PR branch rachel.yang/http-net-error-codes with baseline commit 4f57a47 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 0 unstable metrics.

@mtoffl01 mtoffl01 self-assigned this Dec 16, 2024
@mtoffl01
mtoffl01 self-requested a review December 16, 2024 15:56
mtoffl01
mtoffl01 requested changes Dec 16, 2024
View reviewed changes
Comment thread contrib/internal/httptrace/before_handle.go Outdated
Comment thread contrib/net/http/option.go Outdated
Comment thread contrib/net/http/option.go Outdated
Comment thread contrib/net/http/trace_test.go Outdated
Comment thread contrib/net/http/trace_test.go Outdated
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Dec 16, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: rachel.yang/http-net-error-codes
Commit report: 8963cc8
Test service: dd-trace-go

✅ 0 Failed, 2996 Passed, 24 Skipped, 2m 32.75s Total Time

mtoffl01
mtoffl01 requested changes Dec 17, 2024
View reviewed changes

@mtoffl01 mtoffl01 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@rachelyangdog
rachelyangdog marked this pull request as ready for review December 18, 2024 17:02
@rachelyangdog rachelyangdog changed the title WIP: http/net error codes http/net error codes Dec 18, 2024
mtoffl01
mtoffl01 approved these changes Dec 18, 2024
View reviewed changes

@mtoffl01 mtoffl01 left a comment

Copy link
Copy Markdown
Contributor

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

@rachelyangdog rachelyangdog changed the title http/net error codes contrib/http/net: adding integration-level option to configure error code Dec 19, 2024
@rachelyangdog rachelyangdog changed the title contrib/http/net: adding integration-level option to configure error code contrib/http/net: adding integration-level option to configure error codes Dec 19, 2024
@rachelyangdog rachelyangdog changed the title contrib/http/net: adding integration-level option to configure error codes contrib/http/net: Closes #2882, adding integration-level option to configure error codes Dec 20, 2024
@rachelyangdog rachelyangdog changed the title contrib/http/net: Closes #2882, adding integration-level option to configure error codes contrib/http/net: adding integration-level option to configure error codes Dec 20, 2024
mtoffl01
mtoffl01 requested changes Dec 20, 2024
View reviewed changes
Comment thread contrib/net/http/trace_test.go
mtoffl01
mtoffl01 requested changes Dec 20, 2024
View reviewed changes
Comment thread contrib/net/http/trace_test.go Outdated
darccio
darccio approved these changes Jan 8, 2025
View reviewed changes

@darccio darccio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mtoffl01
mtoffl01 approved these changes Jan 8, 2025
View reviewed changes

@mtoffl01 mtoffl01 left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread contrib/net/http/trace_test.go Outdated
Comment thread contrib/net/http/trace_test.go
Comment thread contrib/net/http/trace_test.go Outdated
@darccio

darccio commented Jan 13, 2025

Copy link
Copy Markdown
Member

@rachelyangdog @mtoffl01 Should we merge this?

@mtoffl01
mtoffl01 enabled auto-merge (squash) January 13, 2025 16:11
@mtoffl01
mtoffl01 merged commit 471d723 into main Jan 13, 2025
@mtoffl01
mtoffl01 deleted the rachel.yang/http-net-error-codes branch January 13, 2025 16:18
hannahkm pushed a commit that referenced this pull request Jan 14, 2025
@rachelyangdog @mtoffl01 @hannahkm
[v2] contrib/http/net: adding integration-level option to configure e…
a00388d 
…rror codes (#3012)

Co-authored-by: Mikayla Toffler <[email protected]>
@darccio darccio added the v2.0 label Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darccio darccio darccio approved these changes
@mtoffl01 mtoffl01 mtoffl01 approved these changes

Assignees

@mtoffl01 mtoffl01

Labels

apm:ecosystem contrib/* related feature requests or bugs v2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rachelyangdog @darccio @mtoffl01