SymDB: Add ScopeBatcher for batching extracted scopes#5691
Conversation
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]>
Typing analysisNote: Ignored files are excluded from the next sections.
|
There was a problem hiding this comment.
💡 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".
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]>
|
@codex review |
There was a problem hiding this comment.
💡 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".
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]>
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: d078214 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-05-08 02:11:22 Comparing candidate commit d078214 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.
|
Strech
left a comment
There was a problem hiding this comment.
👏🏼 Nice and well done.
I have few non-blocking suggestion about the constants in the code and tests.
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]>
`@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]>
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:
MAX_SCOPES, matches the Java/Python tracers)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_scopecalls.The timer is implemented as a single long-lived thread waiting on a
ConditionVariablewith timeout. Eachadd_scopesignals 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, andextractor.rbbecome 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'sSymbolSinkTestare noted in their context descriptions.