Skip to content

fix(internal/telemetry): don't use default dependency loader in telemetry tests#4673

Merged
darccio merged 1 commit into
mainfrom
dario.castane/ktlo/gotip-TestSendFailures
Apr 17, 2026
Merged

fix(internal/telemetry): don't use default dependency loader in telemetry tests#4673
darccio merged 1 commit into
mainfrom
dario.castane/ktlo/gotip-TestSendFailures

Conversation

@darccio

@darccio darccio commented Apr 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Disables the default dependency loader on internal/telemetry test TestSendingFailures.

Root cause: commit a45c8032 in golang/go: "cmd/go: include test deps in buildinfo" (merged ~March 17, 2026, landed in gotip). Fixed golang/go#76926 and introduced test deps populated on BuildInfo.Deps.

No other test are vulnerable to this.

Motivation

Fix gotip scheduled runs: https://github.com/DataDog/dd-trace-go/actions/workflows/gotip-testing.yml?query=branch%3Amain

Before a45c8032, test binary BuildInfo.Deps was empty → dependencies.Payload() returned nil → only Logs in flush → test passed. After: test deps populate Deps → AppDependenciesLoaded joins flush → 2 payloads → MessageBatch → test fails.

Apparently this is a flake but went unnoticed. Looking at historical data, odds to fail since the commit landed on gotip were 1 of every 10-ish runs.

Reviewer's Checklist

  • New code is free of linting errors. You can check this by running make lint locally.
  • New code doesn't break existing tests. You can check this by running make test locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.

Unsure? Have a question? Request a review!

@darccio
fix(internal/telemetry): don't use default dependency loader in telem…
44d8419 
…etry tests

Root cause: commit a45c8032 in golang/go: "cmd/go: include test deps in buildinfo" (merged ~March 17, 2026, landed in gotip). Fixed golang/go#76926.

Before this, test binary BuildInfo.Deps was empty → dependencies.Payload() returned nil → only Logs in flush → test passed. After: test deps populate Deps → AppDependenciesLoaded joins flush → 2 payloads → MessageBatch → test fails.

Apparently this is flaky but went unnoticed. Looking at historical data, odds to fail since the commit landed on gotip were 1 of every 10-ish.
@darccio
darccio requested a review from a team as a code owner April 17, 2026 16:31
@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Apr 17, 2026 •
edited by datadog-official Bot
Loading

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 60.85% (+3.92%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 44d8419 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

hannahkm
hannahkm approved these changes Apr 17, 2026
View reviewed changes

@hannahkm hannahkm 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.

🎉 TY!

@darccio

darccio commented Apr 17, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Apr 17, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@pr-commenter

pr-commenter Bot commented Apr 17, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-04-17 17:01:23

Comparing candidate commit 44d8419 in PR branch dario.castane/ktlo/gotip-TestSendFailures with baseline commit 91482d5 in branch main.

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

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@darccio
darccio merged commit 9f3aa1f into main Apr 17, 2026
194 checks passed
@darccio
darccio deleted the dario.castane/ktlo/gotip-TestSendFailures branch April 17, 2026 17:03
@darccio darccio mentioned this pull request Apr 18, 2026
Merged
4 tasks
gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Apr 20, 2026
@darccio
fix(internal/telemetry): don't use default dependency loader in telem…
6114618 
…etry tests (2) (#4676)

### What does this PR do?

Extends #4673 to `TestMetricsDisabled`.

### Motivation

Same `gotip` fix as in previous PR, but this one went unnoticed accidentally. The whole telemetry suite has been tested under `gotip` to ensure no more fixes are needed.

### Reviewer's Checklist

- [x] Changed code has unit tests for its functionality at or near 100% coverage.
- [x] New code is free of linting errors. You can check this by running `make lint` locally.
- [x] New code doesn't break existing tests. You can check this by running `make test` locally.
- [x] Add an appropriate team label so this PR gets put in the right place for the release notes.

Unsure? Have a question? Request a review!


Co-authored-by: dario.castane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hannahkm hannahkm hannahkm approved these changes

Assignees

No one assigned

Labels

team:apm-go

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cmd/go: dependencies not included in buildinfo for tests

2 participants

@darccio @hannahkm