Skip to content

SymDB: Add ScopeBatcher for batching extracted scopes#5691

Merged
p-datadog merged 6 commits into
masterfrom
symbol-database-scope-batcher-uploader
May 8, 2026
Merged

SymDB: Add ScopeBatcher for batching extracted scopes#5691
p-datadog merged 6 commits into
masterfrom
symbol-database-scope-batcher-uploader

Conversation

@p-datadog

Copy link
Copy Markdown
Member

What does this PR do?

Adds Datadog::SymbolDatabase::ScopeBatcher, which batches Scope objects produced by the extractor and triggers the Uploader (#5670) at appropriate times.

Two upload triggers:

  • Size-based: 400 scopes per batch (MAX_SCOPES, matches the Java/Python tracers)
  • Time-based: 1 second of inactivity (debounce timer, not periodic)

Plus deduplication of already-uploaded module names, a 10,000-file limit to bound memory in heavily-modularized apps, and Mutex-protected state for concurrent add_scope calls.

The timer is implemented as a single long-lived thread waiting on a ConditionVariable with timeout. Each add_scope signals the CV to reset the deadline; when the timeout expires without a signal, the timer fires and flushes the batch. Avoids the thread-per-add_scope cost of the earlier design.

Motivation:

Extracted from #5431 (in-progress symbol database upload work). Splitting the batching logic into its own PR keeps the diff focused and reviewable. The Component class in #5431 wires ScopeBatcher between Extractor and Uploader; forward-references to ScopeBatcher already merged in scope.rb, uploader.rb, and extractor.rb become accurate with this PR.

Change log entry

None.

Additional Notes:

Companion / parent PR: #5431 (will rebase on top of this once merged).

How to test the change?

26 RSpec examples cover initialization, add_scope (size-trigger, deduplication, file-limit), flush, reset, shutdown, pending?, size, thread-safety under concurrent additions, multi-scope batching, and dedup across multiple flushes. Tests ported from Java's SymbolSinkTest are noted in their context descriptions.

bundle exec rspec spec/datadog/symbol_database/scope_batcher_spec.rb

Extracts ScopeBatcher from PR #5431, the in-progress symbol database
upload work. The class batches Scope objects produced by the extractor
and triggers the Uploader (#5670) at appropriate times.

Two upload triggers:
- Size-based: 400 scopes (MAX_SCOPES, matches Java/Python tracers)
- Time-based: 1s of inactivity (debounce timer, not periodic)

Plus deduplication of already-uploaded module names, a 10,000-file
limit to bound memory in heavily-modularized apps, and Mutex-protected
state for concurrent add_scope calls.

Timer implementation is a single long-lived thread waiting on a
ConditionVariable with timeout. Each add_scope signals the CV to reset
the deadline; when the timeout expires without a signal, the timer
fires and flushes the batch. Avoids the thread-per-add_scope cost of
the earlier design.

The Component class (still in #5431) wires ScopeBatcher between
Extractor and Uploader. Forward-references in scope.rb, uploader.rb,
and extractor.rb that mention ScopeBatcher become accurate with this
PR.

Co-Authored-By: Claude <[email protected]>
@p-datadog
p-datadog requested a review from a team as a code owner May 6, 2026 18:51
@p-datadog p-datadog added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label May 6, 2026
@dd-octo-sts

dd-octo-sts Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 1 steep:ignore comment.

steep:ignore comments (+1-0)Introduced:
lib/datadog/symbol_database/scope_batcher.rb:253

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df526b441d

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

Comment thread lib/datadog/symbol_database/scope_batcher.rb Outdated
CRITICAL fixes from di-pr-review on this PR:

1. Add telemetry reporting in all rescue blocks. Per the symdb error
   handling rule, every rescue must log and report telemetry. Added
   `telemetry: nil` constructor parameter and `@telemetry&.report(e,
   description: '...')` in add_scope, timer_loop, and perform_upload
   rescues. Description strings are constants (no unbounded
   interpolation); exception class/message stay in logger.debug only.

2. RBS: prefix all stdlib classes with `::`. Integer, Float, Proc,
   Array, Mutex, Thread, ConditionVariable, Set, String now resolve
   via global namespace, preventing accidental resolution to a class
   in the current module's namespace.

3. Test coverage for the three rescue blocks: add_scope (broken
   uploaded_modules Set), perform_upload (uploader raises), and
   timer_loop (broken @scopes during debounce). Each test verifies
   logging content AND telemetry.report is called with the right
   description, AND that the exception does not propagate.

Tier 2 fixes:

4. Race condition in shutdown/reset. Both methods set @timer_thread =
   nil outside the mutex, allowing a concurrent add_scope to create a
   new timer thread that gets orphaned. Capture the thread reference
   inside the mutex, nil the field inside the mutex, then join the
   captured local outside the mutex.

5. Move `reset` to private. Was documented `@api private` but exposed
   as a public method. Tests now use `send(:reset)`.

6. Drop the three `# steep:ignore:start`/`:end` blocks. They were
   masking type errors that disappear once `scopes_to_upload` is
   declared as `::Array[Scope]?` via `# @type var` annotations.

Verification:
- bundle exec steep check lib/datadog/symbol_database/scope_batcher.rb
  → No type error detected.
- bundle exec rake standard → clean
- bundle exec rspec spec/datadog/symbol_database/scope_batcher_spec.rb
  → 29 examples, 0 failures (3 new rescue-coverage tests).

Co-Authored-By: Claude <[email protected]>
@p-datadog

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e5e834336

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

Comment thread lib/datadog/symbol_database/scope_batcher.rb Outdated
Address review comment from Codex: @file_count was incremented before
the dedup check, so repeated calls for an already-uploaded scope still
consumed the MAX_FILES budget. In a re-extraction scenario where the
same scopes are offered again (e.g., a Rails reload), 10,000 duplicate
offerings would exhaust the budget and cause subsequent unique scopes
to be dropped at the limit — contrary to the documented "Maximum
unique files" intent.

Move @file_count += 1 to after the dedup check, so only accepted
unique scopes consume the budget. Add a test that offers the same
scope MAX_FILES + 1 times then verifies a subsequent unique scope is
still accepted (the budget was not exhausted by duplicates).

- Fixed in lib/datadog/symbol_database/scope_batcher.rb:73-95
- Test added in spec/datadog/symbol_database/scope_batcher_spec.rb

Verification:
- bundle exec rspec spec/datadog/symbol_database/scope_batcher_spec.rb
  → 30 examples, 0 failures
- bundle exec steep check lib/datadog/symbol_database/scope_batcher.rb
  → No type error detected.
- bundle exec rake standard → clean

Co-Authored-By: Claude <[email protected]>
@datadog-official

datadog-official Bot commented May 6, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 99.15%
Overall Coverage: 97.23% (-0.00%)

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

@pr-commenter

pr-commenter Bot commented May 6, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-05-08 02:11:22

Comparing candidate commit d078214 in PR branch symbol-database-scope-batcher-uploader with baseline commit 2280d85 in branch master.

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

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

👏🏼 Nice and well done.

I have few non-blocking suggestion about the constants in the code and tests.

Comment thread lib/datadog/symbol_database/scope_batcher.rb Outdated
Comment thread spec/datadog/symbol_database/scope_batcher_spec.rb Outdated
Comment thread spec/datadog/symbol_database/scope_batcher_spec.rb Outdated
p-datadog and others added 3 commits May 7, 2026 21:33
Address review comment from Strech: the literal `5` used as the timer-thread
join timeout appeared at two sites (shutdown and reset). Extract into a named
constant alongside the other tuning constants (MAX_SCOPES, INACTIVITY_TIMEOUT,
MAX_FILES) so the magic number's meaning is named at the call site.

- Fixed in lib/datadog/symbol_database/scope_batcher.rb:38-41 (constant)
- Fixed in lib/datadog/symbol_database/scope_batcher.rb:165 (shutdown)
- Fixed in lib/datadog/symbol_database/scope_batcher.rb:204 (reset)
- Fixed in sig/datadog/symbol_database/scope_batcher.rbs:10 (signature)

Verification:
- bundle exec rspec spec/datadog/symbol_database/scope_batcher_spec.rb
  → 30 examples, 0 failures
- bundle exec steep check lib/datadog/symbol_database/scope_batcher.rb
  → No type error detected
- bundle exec standardrb lib/datadog/symbol_database/scope_batcher.rb → clean

Co-Authored-By: Claude <[email protected]>
Address two review comments from Strech on the 'when batch size limit
reached' context:

1. The literals 400 and 401 hardcoded the batch size into the tests,
   making them fragile to MAX_SCOPES changes and tying them to the
   implementation's internal value. Replace with described_class::MAX_SCOPES
   (and MAX_SCOPES + 1) — the file already uses this style at line 139
   (MAX_FILES) and line 326 (MAX_SCOPES).

2. The 'continues batching after upload' test only asserted on the final
   batch size, leaving the door open for a flush bug to satisfy the test
   without an upload actually happening. Capture upload calls and assert
   the upload occurred once with a full batch, then check the leftover
   scope sits in the new batch.

- Fixed in spec/datadog/symbol_database/scope_batcher_spec.rb:50-78

Verification:
- bundle exec rspec spec/datadog/symbol_database/scope_batcher_spec.rb
  → 30 examples, 0 failures
- bundle exec standardrb spec/datadog/symbol_database/scope_batcher_spec.rb
  → clean

Co-Authored-By: Claude <[email protected]>
@p-datadog

Copy link
Copy Markdown
Member Author

Thanks for the careful read — both suggestions addressed. The implementation constants are extracted in 75e01fc, and the spec now goes through described_class::MAX_SCOPES and verifies the upload itself in d078214.

@p-datadog
p-datadog merged commit c1ef660 into master May 8, 2026
575 of 586 checks passed
@p-datadog
p-datadog deleted the symbol-database-scope-batcher-uploader branch May 8, 2026 04:31
@dd-octo-sts dd-octo-sts Bot added this to the 2.33.0 milestone May 8, 2026
p-datadog pushed a commit that referenced this pull request May 14, 2026
`@file_count += 1` was moved from after the dedup check to before it during
the squash rebase of PR #5431 (commit 3eca473). This silently reverted
the fix from PR #5431 commit 0f60682 ("Fix file-count to only track
unique accepted scopes") that addressed a codex review finding. The fix
was carried into master via PR #5691 — the rebase regressed it.

With duplicates consuming the MAX_FILES budget, a re-extraction scenario
(e.g. Rails reload re-offering the same scopes) exhausts the unique-file
budget after MAX_FILES duplicate offerings, rejecting subsequent unique
scopes that should still be accepted.

- Fixed in lib/datadog/symbol_database/scope_batcher.rb:75-91 (add_scope)
  by moving @file_count += 1 to after the dedup check, matching master.
- Restored the explanatory comments on the two checks describing why
  duplicates do not count toward MAX_FILES.
- Restored the regression test 'does not count duplicates toward the
  file limit' in spec/datadog/symbol_database/scope_batcher_spec.rb (the
  test that previously caught this regression and was deleted alongside
  the bug's reintroduction).

Verified: bundle exec rspec spec/datadog/symbol_database/scope_batcher_spec.rb
(30 examples, 0 failures).

Co-Authored-By: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants