Skip to content

chore(profiler): remove testHooks and runProfile from tests#4533

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits into
mainfrom
nick.ripley/remove-testhooks-runprofile
Mar 16, 2026
Merged

chore(profiler): remove testHooks and runProfile from tests#4533
gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits into
mainfrom
nick.ripley/remove-testhooks-runprofile

Conversation

@nsrip-dd

@nsrip-dd nsrip-dd commented Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

The profiler unit tests still have many places where they hook into
pacakge-internal details. I've wanted to fix this for a while as it frustrates
me every time I try to make changes. I ran into it again recently and I figure
we should rip the band-aid off.

This PR removes the testHook object, and removes all calls to runProfile from
the profiler unit tests. There are three steps (see the commits):

  • Get rid of the testRunProfile cases that just patch in a profile and assert
    that the profiler returns it.
  • Don't touch the internal profile object in the delta test. The test gives us
    some assurance that we're actually wiring up the delta profile stuff
    properly, vs the internal fastdelta tests which check that the computation is
    correct. So we should keep it. We still patch in profiles so that the test
    can make determinisitic, meaningful assertions. But we only make assertions
    about what gets sent to the backend.
  • Keep the goroutine wait profile tests. But only make assertions about the
    profile we would have uploaded. Break them out into separate test functions
    rather than nested in a RunProfile mega test.

I have plans to remove more internal stuff (like unstartedProfiler) in
subsequent changes.

@pr-commenter

pr-commenter Bot commented Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-03-13 13:40:03

Comparing candidate commit 9bfafcc in PR branch nick.ripley/remove-testhooks-runprofile with baseline commit 0957c7c in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 155 metrics, 9 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 ----------------------------------'

@codecov

codecov Bot commented Mar 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.09%. Comparing base (0957c7c) to head (9bfafcc).

Files with missing lines Patch % Lines
profiler/profile.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
profiler/profiler.go 90.63% <100.00%> (ø)
profiler/profile.go 86.34% <50.00%> (ø)

... and 432 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-official

datadog-official Bot commented Mar 13, 2026 •
edited by datadog-datadog-prod-us1-2 Bot
Loading

Copy link
Copy Markdown
Contributor

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
• Patch Coverage: 66.67%
• Overall Coverage: 59.31%

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

@nsrip-dd
chore(profiler): remove useless TestRunProfile cases
83dd001 
The CPU and goroutine profile cases do nothing useful, and hook into
internal stuff, which we shouldn't do. Delete them and remove the
now-unused CPU profile test hooks.
@nsrip-dd
nsrip-dd force-pushed the nick.ripley/remove-testhooks-runprofile branch 4 times, most recently from f463f4a to 49ff9c4 Compare March 13, 2026 13:15
nsrip-dd added 2 commits March 13, 2026 09:18
@nsrip-dd
chore(profiler): make the delta profile test less intrusive
d6ead36 
The testRunDeltaProfile tests call a bunch of internal profiler stuff.
This makes the package painful to refactor. There is some value in
confirming that we do delta profiles correctly end-to-end, though. So
this commit reworks the test to _mostly_ use the public profiler API.
The only internal thing we change is lookupProfile, which we now do
through a single global hook so we can set it up without having to grab
and modify the private profiler object. Using this, we get a simple but
deterministic profiles and can confirm that the delta looks right in the
actual profile output that we'd upload to the backend.
@nsrip-dd
chore(profiler): make the goroutine wait tests less intrusive
9bfafcc 
The goroutine wait tests have the same problem as the delta tests. They
hook into a bunch of profiler internals. The tests do something useful,
so we shouldn't completely get rid of them. But they should be reworked
to test externally visible behavior as much as possible.

As a result of this change, the testHooks struct is unused. The
lookupProfile thing is now a global and constrained to only a few tests
which actually need it and we don't use the CPU hooks. So, delete it.
@nsrip-dd
nsrip-dd force-pushed the nick.ripley/remove-testhooks-runprofile branch from 49ff9c4 to 9bfafcc Compare March 13, 2026 13:19
@nsrip-dd
nsrip-dd marked this pull request as ready for review March 13, 2026 13:42
@nsrip-dd
nsrip-dd requested a review from a team as a code owner March 13, 2026 13:43
felixge
felixge approved these changes Mar 16, 2026
View reviewed changes

@felixge felixge 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, thanks for doing this 🙇

Comment thread profiler/profile_test.go
@nsrip-dd

nsrip-dd commented Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Mar 16, 2026 •
edited
Loading

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-03-16 13:16:11 UTC ℹ️ Start processing command /merge

2026-03-16 13:16:16 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 29m (p90).

2026-03-16 13:26:15 UTC ℹ️ MergeQueue: This merge request was merged

@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot merged commit bbde759 into main Mar 16, 2026
185 checks passed
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot deleted the nick.ripley/remove-testhooks-runprofile branch March 16, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixge felixge felixge approved these changes

Assignees

No one assigned

Labels

mergequeue-status: done

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nsrip-dd @felixge