Skip to content

Revert "[NO-TICKET] Profiling: Clean up logic for tracking time spent in GC"#6012

Merged
ivoanjo merged 1 commit into
masterfrom
revert-6006-ivoanjo/simplify-gc-tracking-logic
Jul 9, 2026
Merged

Revert "[NO-TICKET] Profiling: Clean up logic for tracking time spent in GC"#6012
ivoanjo merged 1 commit into
masterfrom
revert-6006-ivoanjo/simplify-gc-tracking-logic

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Reverts #6006

We've been seeing some failures in tests caused by this, reverting until we can fix as we're in no rush to ship this one.

Change log entry

None.

@ivoanjo
ivoanjo requested review from a team as code owners July 9, 2026 07:28
@dd-octo-sts

dd-octo-sts Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thank you for updating Change log entry section 👏

Visited at: 2026-07-09 07:29:43 UTC

@dd-octo-sts dd-octo-sts Bot added the profiling Involves Datadog profiling label Jul 9, 2026
@ivoanjo
ivoanjo enabled auto-merge July 9, 2026 07:29
@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Jul 9, 2026 •
edited by datadog-datadog-prod-us1 Bot
Loading

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 90.02% (-0.02%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 51d0677 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Jul 9, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-09 07:55:04

Comparing candidate commit 51d0677 in PR branch revert-6006-ivoanjo/simplify-gc-tracking-logic with baseline commit faab0c8 in branch master.

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

@ivoanjo
ivoanjo merged commit b61ce6f into master Jul 9, 2026
589 checks passed
@ivoanjo
ivoanjo deleted the revert-6006-ivoanjo/simplify-gc-tracking-logic branch July 9, 2026 07:56
@dd-octo-sts dd-octo-sts Bot added this to the 2.38.0 milestone Jul 9, 2026
@ivoanjo ivoanjo mentioned this pull request Jul 9, 2026
Merged
ivoanjo added a commit that referenced this pull request Jul 10, 2026
@ivoanjo
Reapply "[NO-TICKET] Profiling: Clean up logic for tracking time spen…
d63d6ee 
…t in GC"

This reverts commit 51d0677.

Aka #6006 had issues,
and thus in #6012 we
reverted it, and this commit restores back #6006 so we can actually
fix it.
ivoanjo added a commit that referenced this pull request Jul 10, 2026
@ivoanjo
[NO-TICKET] Profiling: Clean up logic for tracking time spent in GC, …
4145a5d 
…try 2

**What does this PR do?**

This PR is a second attempt at
#6006 ; the first time
got reverted in #6012 as
we were seeing flaky failures.

From DataDog/ruby-guild#314:

```
W, [2026-07-08T16:58:22.863694 #11835]  WARN -- datadog: [datadog] CpuAndWallTimeWorker thread error. Operation: "rescued_sample_from_postponed_job" Cause: RuntimeError: BUG: Unexpected sample during GC (thread_id=11835 (10200), gc_tracking.cpu_time_at_start_ns=2802579244, gc_tracking.wall_time_at_start_ns=729604100602, monotonic_wall_time_now_ns=729890337067) Location: /__w/dd-trace-rb/dd-trace-rb/lib/datadog/profiling/collectors/idle_sampling_helper.rb:42:in 'Datadog::Profiling::Collectors::IdleSamplingHelper._native_idle_sampling_loop'
W, [2026-07-08T16:58:22.863821 #11835]  WARN -- datadog: [datadog] Failed to send telemetry before components initialization or within components lifecycle
    records garbage collection cycles (FAILED - 3)
    marks the new thread as fork-safe
    does not create a second thread if start is called again
    with allocation profiling enabled
/__w/dd-trace-rb/dd-trace-rb/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb:43: [BUG] Datadog Ruby profiler detected callback nested inside sample. Please report this at https://github.com/datadog/dd-trace-rb/blob/master/CONTRIBUTING.md#found-a-bug
ruby 4.0.1 (2026-01-13 revision e04267a14b) +PRISM [x86_64-linux]
```

(The "callback nested inside sample" happens because some of our
tests have strict assertions that things like object allocations
can't happen, and they were failing when trying to raise that
`RunntimeError: BUG: Unexpected sample during GC` as well)

The problem we were seeing was due to the `#on_gc_start` specs
never clearing the "thread is in GC" state and thus this state was
leaking to other tests which then would trigger the assertion
we added.

The fix is to make sure we don't leave such leftover state; in the
future #5960 is a more
holistic solution to this whole "leftover state between profiler
instances" problem.

I recommend reviewing this PR commit-by-commit.

**Motivation:**

The original cleanup is useful so we definitely want to keep it!

**Additional Notes:**

```
bundle exec rspec './spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb[1:2:12:1,1:2:12:2,1:2:13,1:2:15:1,1:2:16,1:2:18:1,1:2:18:2:1,1:2:20:1,1:7:1]' './spec/datadog/profiling/collectors/thread_context_spec.rb[1:3:2:1]' --seed 32922
```

is one way of reproducing the issue.

**How to test the change?**

We should see a green CI and no flakiness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eregon eregon eregon approved these changes

Assignees

No one assigned

Labels

profiling Involves Datadog profiling

Projects

None yet

Milestone

2.38.0

Development

Successfully merging this pull request may close these issues.

2 participants

@ivoanjo @eregon