[NO-TICKET] Profiling: Reset per-thread state when starting profiler by ivoanjo · Pull Request #5960 · DataDog/dd-trace-rb

ivoanjo · 2026-06-29T11:05:50Z

What does this PR do?

This PR introduces a new thread_context_collector_global_reset_per_thread_context function in the ThreadContext collector.

This function, as the name indicates, resets all of the per-thread context we keep, and is called by the CpuAndWallTimeWorker right before profiling starts so that we always start with a known-good blank slate, which fixes a few issues (discussed below) of stale state resulting in surprising behaviors.

This also allows us to simplify a few things in the code, as we get all of the below "naturally" from this reset:

We no longer need to allow for sampling_buffer resizes (it comes naturally from reallocating a new buffer at the start of profiling)
We no longer need to separately backfill per-thread context for threads that existed before the profiler loaded and we installed our thread_begin_tracepoint
We no longer need to reset the context after forking

Motivation:

Since #5816, we've been keeping the profiler's per-thread context directly attached to each Ruby thread.

This has a number of advantages:

We can keep as many fields as needed in the state and access them without the GVL
Ruby GC takes care of cleaning up the context from dead threads
We don't need to wait until the next cpu/wall sample to get the context on a thread and thus to start recording info about it, such as for allocation profiling or GVL events
Faster lookups!

Yet, because the per-thread context was being retained from profiler run to profiler run, we ran into a few rare corner cases:

Stopping the profiler for a time period and restarting it again, would carry over state:

Datadog.configure { |c| c.profiling.enabled = true }
Datadog::Profiling.wait_until_running
sleep 5
Datadog.configure { |c| c.profiling.enabled = false }
sleep 5
Datadog.configure { |c| c.profiling.enabled = true } # <-- profiler #2
sleep 1

In #5926 (comment) you can see a screenshot of a 1 second profile with 6 seconds of cpu/wall-time blamed on a single sample, representing that gap when the profiler was stopped.

It caused flakiness in the tests -- leading for instance to the skip in [NO-TICKET] Skip flaky profiling spec until a full fix #5929 .

Some of our tests had very strict assertions so we were playing whack-a-mole with "oh what if this one piece of leftover state was there between tests".

Change log entry

Yes. Profiling: Fix left-over state impacting samples when profiler gets stopped and started again

Additional Notes:

This is my third attempt at addressing this issue, and in particular it replaces the earlier #5926 .

How to test the change?

I've added code coverage for the new behavior, including a spec that asserts there's no carry-over state between executions of the profiler/CpuAndWallTimeWorker.

…avoid cross-profile leftover state This helper is called when the `CpuAndWallTimeWorker` is about to start, and resets all the per-thread state. This avoids any cross-state contamination, and it's also not concurrent with other things since the `CpuAndWallTimeWorker` has a built-in mechanism to make sure a given instance is the only one running at a time.

The global reset now ensures a clean slate before each test (similar to what happens in production with a new profiler run).

This ensures the need for a separate initialization step -- the global reset takes care of it all.

This reverts commit 6b56cfa. Now that `_native_global_reset_per_thread_context` runs before every test-case, the leftover state that was causing the flakiness should not happen anymore. Out of curiosity, I could manually reproduce something very close to the original flakiness with ```ruby describe "#sample_after_gvl_running" do before { skip_if_gvl_profiling_not_supported(self) } fcontext "if thread does not have per-thread context" do before { remove_per_thread_context_for(t1) } # it do # expect(sample_after_gvl_running(t1)).to be false # end it 'weird test setup' do sleep_thread = Thread.new { sleep } sample # Creates context on_gvl_released(sleep_thread) sample # Waiting starts sample # Sets skip pp per_thread_context[sleep_thread] end it "does not sample the thread" do sample_after_gvl_running(t1) expect(samples).to be_empty end end ``` And running with `--order=defined` to make sure the specs ran in this order. (There was one very subtle detail here -- the `was_skipped_at_last_sample` gets reset when the recorder is flushed so the issue in particular that made the spec flaky might not happen in practice in production since when we stop the profiler we flush the recorder)

Now that we can rely on having a `thread_context_collector_global_reset_per_thread_context` that will right-size buffers, we no longer need to keep the buffer resizing logic. I still left behind a buffer size check just in case some bug slips through.

This avoids the benchmarks needing to care about this new detail.

dd-octo-sts · 2026-06-29T11:06:21Z

Typing analysis

Note: Ignored files are excluded from the next sections.

Untyped methods

This PR introduces 2 partially typed methods, and clears 2 partially typed methods. It increases the percentage of typed methods from 65.99% to 66.0% (+0.01%).

Partially typed methods (+2-2)

❌ Introduced:

sig/datadog/profiling/collectors/thread_context.rbs:27
└── def self.for_testing: (
          recorder: Datadog::Profiling::StackRecorder,
          ?max_frames: ::Integer,
          ?tracer: Datadog::Tracing::Tracer?,
          ?endpoint_collection_enabled: bool,
          ?waiting_for_gvl_threshold_ns: ::Integer,
          ?otel_context_enabled: (::Symbol? | bool),
          ?native_filenames_enabled: bool,
          ?trigger_global_reset: bool,
          **untyped
        ) -> Datadog::Profiling::Collectors::ThreadContext
sig/datadog/profiling/collectors/thread_context.rbs:53
└── def safely_extract_context_key_from: (untyped tracer) -> ::Symbol?

✅ Cleared:

sig/datadog/profiling/collectors/thread_context.rbs:27
└── def self.for_testing: (
          recorder: Datadog::Profiling::StackRecorder,
          ?max_frames: ::Integer,
          ?tracer: Datadog::Tracing::Tracer?,
          ?endpoint_collection_enabled: bool,
          ?waiting_for_gvl_threshold_ns: ::Integer,
          ?otel_context_enabled: (::Symbol? | bool),
          ?native_filenames_enabled: bool,
          **untyped
        ) -> Datadog::Profiling::Collectors::ThreadContext
sig/datadog/profiling/collectors/thread_context.rbs:48
└── def safely_extract_context_key_from: (untyped tracer) -> ::Symbol?

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept on the line before the definition to remove it from the stats.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d999114af

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

datadog-datadog-prod-us1-2 · 2026-06-29T11:23:23Z

Tests

✨ Fix all issues with BitsAI

⚠️ Warnings

❄️ 1 New flaky test detected

Datadog::Profiling::Collectors::ThreadContext#thread_context_collector_reset_all_per_thread_contexts resets every existing per-thread context from rspec
undefined method \`_native_on_gvl_released&#39; for Datadog::Profiling::Collectors::ThreadContext::Testing:Module

    described_class::Testing._native_on_gvl_released(thread)
                            ^^^^^^^^^^^^^^^^^^^^^^^^
Did you mean?  _native_on_gc_start

Failure/Error: described_class::Testing._native_on_gvl_released(thread)

NoMethodError:
  undefined method \`_native_on_gvl_released&#39; for Datadog::Profiling::Collectors::ThreadContext::Testing:Module
...
New test introduced in this PR is flaky.
View in Flaky Test Management

ℹ️ Info

No other issues found (see more)

🧪 All tests passed

🔄 Datadog auto-retried 1 job - 1 passed on retry

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 90.04% (+0.00%)

Useful? React with 👍 / 👎

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 4404932 | Docs | Datadog PR Page | Give us feedback!}

…ing logic Codex correctly pointed out: > When `max_frames` changes during `Datadog.configure` reconfiguration, `> replace_components!` builds the new components before calling > `old.shutdown!`, and constructing the new `ThreadContext` updates the > global `latest_max_frames` while the old profiler can still be sampling. > Any thread created in that window gets a buffer sized for the new > collector, so this hard raise makes the still-running old worker report > a profiler failure instead of resizing/capping as it did before. We fix this by setting the `latest_max_frames` not at `ThreadContext` creation, but along with the global reset when the `ThreadContext` is **about to be used**. This avoids the issue above.

pr-commenter · 2026-06-29T11:58:12Z

Benchmarks

Benchmark execution time: 2026-07-09 13:13:00

Comparing candidate commit 4404932 in PR branch ivoanjo/fix-leftover-state-between-profilers-try4 with baseline commit cf99651 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 ----------------------------------'

…rs-try4

ivoanjo · 2026-06-29T14:58:31Z

Marking as draft as my last changes were not correct + I uncovered a latent bug elsewhere

I saw this blowing up in CI due to some other errors on the branch, and in practice raising exceptions here is never correct and it was good that Ruby complained.

ivoanjo · 2026-06-30T13:23:51Z

Ok, I believe this is finally in good shape for review!

eregon

Great stuff, this feels much cleaner and less brittle than explicitly clearing contexts, because we global reset only at known "safe points" i.e. when the profiler is stopped and all hooks are reliably disabled

… `thread_context_collector_reset_all_per_thread_contexts`

Co-authored-by: Benoit Daloze <[email protected]>

…ead_contexts`

…rs-try4

Since #5870 / 41bcd1f overhead samples no longer show up on the thread that does the sampling so we can simplify this.

…llector

Even though these methods don't get used in practice on old Rubies (since they have no GVL instrumentation API hooks), it's useful to have these outside of `NO_GVL_INSTRUMENTATION` for the `Datadog::Profiling::Collectors::ThreadContext#thread_context_collector_reset_all_per_thread_contexts` "resets every existing per-thread context" test, and it seems harmess to always build it.

…er_thread_contexts` specs This way we ensure that there's a non-zero state on the thread for all other cases (e.g. asserts that a no-op `global_reset_per_thread_context` won't trivially pass).

As pointed out very correctly in PR review, `free` && `ruby_xfree` are happy to no-op a NULL so let's just clean this up instead of writing a treaty on conditions where it might not have been initialized.

…rs-try4

…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.

Now that #5960 landed, we don't need these hooks anymore. I've confirmed that #5960 still fixes the issue: ``` 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 ``` doesn't break but commenting out `thread_context_collector_reset_all_per_thread_contexts(state->thread_context_collector_instance);` inside `_native_sampling_loop` in `collectors_cpu_and_wall_time_worker.c` does trigger the issue again, confirming that it's the reset that's now taking care of fixing the issue as expected.

ivoanjo added 11 commits June 26, 2026 16:25

Small tweaks to comments

6ae3a70

Add spec for cleaning up leftover state

02c3073

Remove now-undeeded clearing of state after fork

59f6227

Remove need for clear_per_thread_context_for helper

31698c9

The global reset now ensures a clean slate before each test (similar to what happens in production with a new profiler run).

Let global reset create contexts for threads that didn't have it

16e2a12

This ensures the need for a separate initialization step -- the global reset takes care of it all.

Document some assumptions and important details

87b5d09

Simplify buffer resize logic

35bf1f2

Now that we can rely on having a `thread_context_collector_global_reset_per_thread_context` that will right-size buffers, we no longer need to keep the buffer resizing logic. I still left behind a buffer size check just in case some bug slips through.

Minor cleanups

42fada9

Add global reset to ThreadContext.for_testing

5d99911

This avoids the benchmarks needing to care about this new detail.

ivoanjo requested review from a team as code owners June 29, 2026 11:05

dd-octo-sts Bot added the profiling Involves Datadog profiling label Jun 29, 2026

ivoanjo requested a review from eregon June 29, 2026 11:06

ivoanjo mentioned this pull request Jun 29, 2026

[NO-TICKET] Profiling: Reset leftover per-thread state when creating new ThreadContext #5926

Closed

chatgpt-codex-connector Bot reviewed Jun 29, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_stack.c

Merge branch 'master' into ivoanjo/fix-leftover-state-between-profile…

1807008

…rs-try4

ivoanjo marked this pull request as draft June 29, 2026 14:57

ivoanjo added 3 commits June 30, 2026 09:56

Fix test that didn't properly initialize the ThreadContext

276f31d

Remove raising of exception in situation where we can't raise exceptions

1bbeb3d

I saw this blowing up in CI due to some other errors on the branch, and in practice raising exceptions here is never correct and it was good that Ruby complained.

Skip one more test for the usual "flaky under asan" reasons

1240b4f

ivoanjo marked this pull request as ready for review June 30, 2026 13:23

eregon reviewed Jul 1, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c Outdated

eregon reviewed Jul 2, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

ivoanjo and others added 9 commits July 3, 2026 13:58

Rename thread_context_collector_global_reset_per_thread_context to…

1213b3c

… `thread_context_collector_reset_all_per_thread_contexts`

Tweak note on CpuAndWallTimeWorker._native_reset_after_fork

79370c6

Apply suggestions from code review

6787b64

Co-authored-by: Benoit Daloze <[email protected]>

Improve documentation for `thread_context_collector_reset_all_per_thr…

fa038f2

…ead_contexts`

Merge branch 'master' into ivoanjo/fix-leftover-state-between-profile…

e451897

…rs-try4

Retain is_profiler_internal_thread when resetting context

e2337c0

Use cpu-time for comparisons, avoid comparing cpu to wall-time

8df95e8

Remove filtering of overhead samples on Thread.current

96253d2

Since #5870 / 41bcd1f overhead samples no longer show up on the thread that does the sampling so we can simplify this.

Simplify use of remove_per_thread_context_for by always touching co…

e64108b

…llector

eregon reviewed Jul 7, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c

ivoanjo added 2 commits July 7, 2026 11:22

Fix accidentally clobbering time calculation

589246a

eregon reviewed Jul 7, 2026

View reviewed changes

Comment thread lib/datadog/profiling/collectors/thread_context.rb

ivoanjo mentioned this pull request Jul 7, 2026

[NO-TICKET] Profiling: Fix using state on thread without GVL #6004

Merged

eregon reviewed Jul 7, 2026

View reviewed changes

Comment thread spec/datadog/profiling/collectors/thread_context_spec.rb

eregon reviewed Jul 7, 2026

View reviewed changes

Comment thread spec/datadog/profiling/collectors/thread_context_spec.rb

Minor: Reuse initialization for `thread_context_collector_reset_all_p…

b4021d4

…er_thread_contexts` specs This way we ensure that there's a non-zero state on the thread for all other cases (e.g. asserts that a no-op `global_reset_per_thread_context` won't trivially pass).

ivoanjo force-pushed the ivoanjo/fix-leftover-state-between-profilers-try4 branch from 59b56f4 to b4021d4 Compare July 7, 2026 13:01

Restore comments about concurrent threads after PR discussion

a257577

eregon approved these changes Jul 9, 2026

View reviewed changes

ivoanjo mentioned this pull request Jul 9, 2026

[NO-TICKET] Profiling: Clean up logic for tracking time spent in GC, try 2 #6016

Merged

ivoanjo added 2 commits July 9, 2026 13:11

Remove check for NULL stack_buffer

0857984

As pointed out very correctly in PR review, `free` && `ruby_xfree` are happy to no-op a NULL so let's just clean this up instead of writing a treaty on conditions where it might not have been initialized.

Merge branch 'master' into ivoanjo/fix-leftover-state-between-profile…

4404932

…rs-try4

ivoanjo merged commit c1b6e95 into master Jul 9, 2026
595 of 598 checks passed

ivoanjo deleted the ivoanjo/fix-leftover-state-between-profilers-try4 branch July 9, 2026 13:36

dd-octo-sts Bot added this to the 2.38.0 milestone Jul 9, 2026

Uh oh!

Conversation

ivoanjo commented Jun 29, 2026

Uh oh!

dd-octo-sts Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Typing analysis

Untyped methods

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

datadog-datadog-prod-us1-2 Bot commented Jun 29, 2026 • edited by datadog-datadog-prod-us1 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Warnings

ℹ️ Info

Uh oh!

pr-commenter Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Explanation

More details about the CI and significant changes

Uh oh!

ivoanjo commented Jun 29, 2026

Uh oh!

ivoanjo commented Jun 30, 2026

Uh oh!

Uh oh!

eregon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dd-octo-sts Bot commented Jun 29, 2026 •

edited

Loading

datadog-datadog-prod-us1-2 Bot commented Jun 29, 2026 •

edited by datadog-datadog-prod-us1 Bot

Loading

pr-commenter Bot commented Jun 29, 2026 •

edited

Loading