Skip to content
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

ddtrace/tracer: clear global headers in remote config tests #2466

Merged
merged 2 commits into from Dec 26, 2023

Conversation

nsrip-dd
Copy link
Contributor

What does this PR do?

Clear the global headers in the remote config tests which modify them.

Motivation

While following up on #2265, I ran into this failure:

% go test -shuffle=1703169881762824000 ./ddtrace/tracer
-test.shuffle 1703169881762824000
2023/12/21 13:29:29 Datadog Tracer v1.60.0-dev ERROR: Loading features: Get "http://localhost:8126/info": dial tcp 127.0.0.1:8126: connect: connection refused, 2 additional messages skipped (first occurrence: 21 Dec 23 13:29 EST)
--- FAIL: TestWithHeaderTags (0.00s)
    --- FAIL: TestWithHeaderTags/default-off (0.00s)
        option_test.go:1202: 
            	Error Trace:	/Users/nick.ripley/repos/dd-trace-go/ddtrace/tracer/option_test.go:1202
            	Error:      	Not equal: 
            	            	expected: 0
            	            	actual  : 1
            	Test:       	TestWithHeaderTags/default-off
FAIL
FAIL	gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer	37.505s
FAIL

This failure happened because TestOnRemoteConfigUpdate ran before
TestWithHeaderTags and didn't clean up some globally-configured headers.
This is basically the same issue addressed by #2262.

We could consider having each test which accesses global headers clear them at
the start. The downside of that approach would be that we'd miss the chance to
find out which tests don't clean up after themselves (I did in this case by
seeing that X-Test-Header:my-tag-name-in-code was still in the map)

This PR might not be the ideal solution to this problem in general, but it at
least covers the other existing case of a test which reads these values. Once
we start shuffling unit tests we'll ideally catch these kinds of issues as
they're introduced, and we can come up with a more robust way to prevent them
if it keeps happening.

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.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

Clear the global headers in the remote config tests which modify them.
Otherwise, tests like TestWithHeaderTags, which are sensitive to the
current global headers, can fail if they're run after the remote config
tests.

This might not be the ideal solution to this problem in general, but it
at least covers the other existing case of a test which reads these
values. Once we start shuffling unit tests we'll ideally catch these
kinds of issues as they're introduced, and we can come up with a more
robust way to prevent them if it keeps happening.
@nsrip-dd nsrip-dd marked this pull request as ready for review December 21, 2023 18:56
@nsrip-dd nsrip-dd requested a review from a team December 21, 2023 18:56
@pr-commenter
Copy link

pr-commenter bot commented Dec 21, 2023

Benchmarks

Benchmark execution time: 2023-12-26 13:53:17

Comparing candidate commit 5376168 in PR branch nick.ripley/remote-config-test-clear-headers with baseline commit 7938f72 in branch main.

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

Copy link
Contributor

@ahmed-mez ahmed-mez left a comment

Choose a reason for hiding this comment

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

Thanks!

@nsrip-dd nsrip-dd enabled auto-merge (squash) December 26, 2023 13:37
auto-merge was automatically disabled December 26, 2023 13:40

Pull Request is not mergeable

@nsrip-dd nsrip-dd merged commit f986cc5 into main Dec 26, 2023
153 checks passed
@nsrip-dd nsrip-dd deleted the nick.ripley/remote-config-test-clear-headers branch December 26, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants