Skip per-tick sampling of profiler-internal threads by eregon · Pull Request #5955 · DataDog/dd-trace-rb

eregon · 2026-06-25T14:07:32Z

CpuAndWallTimeWorker and IdleSamplingHelper threads are now sampled only once per profiling period (via the on-serialize flush) instead of every 10ms tick, reducing profiling overhead.

What does this PR do?
^

Motivation:
Reduce profiler overhead

Change log entry

Yes, Changed: Profiling: Reduce profiler overhead by sampling profiler-internal threads only once per minute.

Additional Notes:

How to test the change?

datadog-datadog-prod-us1-2 · 2026-06-25T14:08:48Z

Tests

✨ Fix all issues with BitsAI

⚠️ Warnings

🚦 1 Pipeline job failed

Check Pull Request CI Status | all-jobs-are-green

❄️ 2 New flaky tests detected

Datadog::Profiling::Collectors::ThreadContext profiler-internal thread skipping does not increment inactive_thread_samples_skipped

from rspec

expected \`stats.fetch(:inactive_thread_samples_skipped)\` not to have changed, but did change from 1 to 2

Failure/Error: expect { sample }.to_not change { stats.fetch(:inactive_thread_samples_skipped) }
  expected \`stats.fetch(:inactive_thread_samples_skipped)\` not to have changed, but did change from 1 to 2
./spec/datadog/profiling/collectors/thread_context_spec.rb:2105:in &#39;block (3 levels) in &lt;top (required)&gt;&#39;
./spec/spec_helper.rb:313:in &#39;block (2 levels) in &lt;top (required)&gt;&#39;
./spec/spec_helper.rb:207:in &#39;block (2 levels) in &lt;top (required)&gt;&#39;
/usr/local/bundle/gems/webmock-3.26.2/lib/webmock/rspec.rb:39:in &#39;block (2 levels) in &lt;top (required)&gt;&#39;
/usr/local/bundle/gems/rspec-wait-0.0.10/lib/rspec/wait.rb:47:in &#39;block (2 levels) in &lt;top (required)&gt;&#39;
./spec/support/execute_in_fork.rb:32:in &#39;ForkableExample#run&#39;

New test introduced in this PR is flaky.

Datadog::Profiling::Collectors::ThreadContext profiler-internal thread skipping still records one sample per profile period via serialize

from rspec

undefined method \`_native_apply_delta_to_cpu_time_at_previous_sample_ns&#39; for Datadog::Profiling::Collectors::ThreadContext::Testing:Module

      ._native_apply_delta_to_cpu_time_at_previous_sample_ns(thread, delta_ns)
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Did you mean?  _native_sample_skipped_allocation_samples

Failure/Error:
  described_class::Testing
    ._native_apply_delta_to_cpu_time_at_previous_sample_ns(thread, delta_ns)

...

New test introduced in this PR is flaky.

View in Flaky Test Management

ℹ️ Info

No other issues found (see more)

🧪 All tests passed

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

Useful? React with 👍 / 👎

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

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

eregon · 2026-06-25T14:19:07Z

In the benchmarks/profiling_sample_overhead.rb benchmark we still have ~1000 samples for the profiling ovearhead (Datadog::Profiling::Sampling, sampling), even though we didn't sample any thread, but it still took CPU time (~40µs).
What should we do about those? Maybe we should group them by reporting period too to reduce the number of samples?

…changes

eregon · 2026-06-25T14:35:38Z

Before:

{duration: 10.02499,
 samples: 998,
 trigger_simulated_signal_delivery_attempts: 996,
 cpu_sampling_time_ns_total: 30466000,
 cpu_sampling_overhead: "0.30%",
 serialization_time_ns_total: 4696000,
 serialization_overhead: "0.05%",
 sampling_rate: 99.5512214974778,
 sample_every_n_ms: 10.045080160320643,
 inactive_thread_samples_skipped: 3983}

After:

{duration: 10.020218,
 samples: 998,
 trigger_simulated_signal_delivery_attempts: 997,
 cpu_sampling_time_ns_total: 24977000,
 cpu_sampling_overhead: "0.25%",
 serialization_time_ns_total: 2432000,
 serialization_overhead: "0.02%",
 sampling_rate: 99.59863148685987,
 sample_every_n_ms: 10.04029859719439,
 inactive_thread_samples_skipped: 2988,
 profiler_thread_samples_skipped: 1996}

cpu_sampling_overhead and serialization_overhead drop and all regular samples are skipped for the 2 profiler-internal threads.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3d4ae0f42

ℹ️ 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".

pr-commenter · 2026-06-25T14:55:56Z

Benchmarks

Benchmark execution time: 2026-07-01 10:02:43

Comparing candidate commit 22d15f8 in PR branch dont-sample-profiler-internal-threads with baseline commit 75ccdd1 in branch master.

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

scenario:profiling - Allocations ()

🟥 throughput [-224438.090op/s; -181281.901op/s] or [-6.928%; -5.596%]

ivoanjo

This is cool! I'm a bit meh about having the CpuAndWallTimeWorker doing a "fake-serialize" to still get the overhead on those threads so I've suggested an alternative ;)

ivoanjo

Gave it a pass, I think almost almost there!

eregon · 2026-06-30T09:12:35Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cabb4640bd

ℹ️ 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".

ivoanjo

👍 LGTM, There's just the tiny change to the thread_done call and otherwise let's get this in!

CpuAndWallTimeWorker and IdleSamplingHelper threads are now sampled only once per profiling period (via the on-serialize flush) instead of every 10ms tick, reducing profiling overhead. Skip record_sampling_overhead for profiler-internal threads since their timestamps are not updated during per-tick sampling. Seed cpu/wall timestamps when marking a thread as profiler-internal, so the first reporting period produces a real delta instead of zero (INVALID_TIME would cause update_time_since_previous_sample to return 0, losing the entire first period). Each profiler-internal thread records its own final sample before dying via rb_ensure. The worker thread calls profiler_internal_thread_done() in the cleanup path after rb_protect returns. The idle helper wraps its loop body in rb_ensure to guarantee the flush even on exception, receiving the thread_context_collector as an argument — the Ruby reference on the stack keeps it alive, so no GC marking is needed in the native struct. This eliminates the race where the idle helper thread might not have been scheduled yet when on_serialize was called from #stop. Co-Authored-By: Claude Opus 4.6 <[email protected]>

…GVL_INSTRUMENTATION guard This testing helper manipulates per-thread timestamps and is not GVL-specific. Having it inside the #ifndef guard made it unavailable on Ruby <= 3.1, breaking the profiler-internal thread skipping test. Co-Authored-By: Claude Opus 4.6 <[email protected]>

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

eregon force-pushed the dont-sample-profiler-internal-threads branch 2 times, most recently from 6656528 to 3b3cf59 Compare June 25, 2026 14:15

We need bundle exec rake clean compile when the libdatadog version …

10583ec

…changes

eregon force-pushed the dont-sample-profiler-internal-threads branch 2 times, most recently from 10efbe0 to b3d4ae0 Compare June 25, 2026 14:30

eregon marked this pull request as ready for review June 25, 2026 14:31

eregon requested review from a team as code owners June 25, 2026 14:31

chatgpt-codex-connector Bot reviewed Jun 25, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c

Comment thread ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c Outdated

eregon force-pushed the dont-sample-profiler-internal-threads branch from b3d4ae0 to 4909597 Compare June 25, 2026 15:09

ivoanjo reviewed Jun 29, 2026

View reviewed changes

ivoanjo reviewed Jun 30, 2026

View reviewed changes

eregon force-pushed the dont-sample-profiler-internal-threads branch 3 times, most recently from 09c443e to cabb464 Compare June 30, 2026 09:12

chatgpt-codex-connector Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c Outdated

eregon force-pushed the dont-sample-profiler-internal-threads branch from cabb464 to eaa56fa Compare June 30, 2026 13:25

ivoanjo mentioned this pull request Jun 30, 2026

[NO-TICKET] Profiling: Cleanup unneeded sampling_buffer argument #5966

Merged

ivoanjo approved these changes Jun 30, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c Outdated

eregon force-pushed the dont-sample-profiler-internal-threads branch from eaa56fa to 0d9a6b5 Compare June 30, 2026 13:53

eregon enabled auto-merge June 30, 2026 13:56

eregon and others added 3 commits July 1, 2026 11:37

Fix allocation IMEMO test

cd7e3f5

eregon force-pushed the dont-sample-profiler-internal-threads branch from 51be165 to 22d15f8 Compare July 1, 2026 09:37

eregon merged commit ad5a9f1 into master Jul 1, 2026
867 of 869 checks passed

eregon deleted the dont-sample-profiler-internal-threads branch July 1, 2026 11:00

dd-octo-sts Bot added this to the 2.37.0 milestone Jul 1, 2026

p-datadog mentioned this pull request Jul 1, 2026

Fix flaky: CpuAndWallTimeWorker "always simulates signal delivery" #5980

Closed

Uh oh!

Conversation

eregon commented Jun 25, 2026

Uh oh!

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

Uh oh!

⚠️ Warnings

ℹ️ Info

Uh oh!

eregon commented Jun 25, 2026

Uh oh!

eregon commented Jun 25, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

pr-commenter Bot commented Jun 25, 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

scenario:profiling - Allocations ()

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivoanjo 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!

eregon commented Jun 30, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

edited by datadog-prod-us1-3 Bot

Loading

pr-commenter Bot commented Jun 25, 2026 •

edited

Loading