SymDB: review follow-ups for #5717#5771
Merged
Merged
Conversation
The Scope :targetable_lines comment at scope.rb:25 was rewritten in #5717 in a way that lost the distinction between the Ruby identifier (targetable_lines) and the wire-format key (has_injectible_lines). The same file at lines 83-88 still serializes with the historical +injectible+ spelling for backend compatibility, so the line-25 comment contradicted the actual implementation in the same file. Restore the original comment so the wire-format key name is documented next to the Struct member it describes. Also rename the scope_spec.rb test description that propagated the same wrong wording ("includes targetable_lines?: false") to match what the test body actually asserts ("emits has_injectible_lines: false").
#5717 deleted the spec at extractor_spec.rb that exercised the top-level rescue in Extractor#extract_all. The production rescue is still in place at extractor.rb:149-151 — the deletion was an unintentional artifact of the squash rebase and left the rescue path uncovered. Restore the original test so the rescue is exercised again.
#5717 used Datadog.logger directly in Remote.process_change / enable_upload / disable_upload / parse_config — a Component Pattern violation, since the component was in scope at every call site and the logger was the dependency that should have been used. - Expose Component#logger via attr_reader (and update the RBS). - Replace Datadog.logger with component.logger in the four sites where component is available. The receiver-block fallback at remote.rb:48 keeps Datadog.logger because the component lookup itself may return nil there. - Thread the logger explicitly through parse_config(content, logger) rather than relying on a global. - Update the RBS for parse_config and remote_spec.rb's parse_config / component test doubles. Also adds a justification comment to the remote.rb:106 # steep:ignore NoMethod directive, matching the pattern already used at line 113.
#5717 added a new conditional in core/remote/client/capabilities.rb that registers symbol_database RC capabilities only when both DI and symbol_database are enabled. Neither path was exercised by tests. Add three contexts mirroring the existing DI coverage: 1. DI disabled, symbol_database enabled — does not register product 2. DI enabled, symbol_database disabled — does not register product 3. DI enabled, symbol_database enabled — registers product and a receiver matching the LIVE_DEBUGGING_SYMBOL_DB path This covers both sides of the new conditional plus its outer guard.
#5717 wired SymbolDatabase::Component into Core::Configuration::Components (build at @symbol_database, shutdown! during teardown) but did not extend components_spec.rb to cover the new wiring. Add an @symbol_database describe block with two contexts: - symbol_database disabled → components.symbol_database is nil - symbol_database + remote config enabled → components.symbol_database is a Datadog::SymbolDatabase::Component (skipped on JRuby and Ruby < 2.6 per the feature's support matrix) Also extend the #shutdown! test to assert that components.symbol_database receives shutdown! when present, matching the pattern already used for dynamic_instrumentation, appsec, etc.
Contributor
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 8e16e89 | Docs | Datadog PR Page | Give us feedback! |
p-datadog
marked this pull request as ready for review
May 18, 2026 17:21
Use "symbol database" (the feature name) consistently in both @symbol_database describe block test names, instead of mixing "symbol_database component" and "SymbolDatabase::Component".
Strech
approved these changes
May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Addresses the review findings raised against #5717 (already merged). Each commit is one focused finding.
Scope:targetable_linescomment atscope.rb:25was rewritten in #5717 and lost the Ruby-identifier vs wire-format-key distinction. The same file at lines 83-88 still serializes with the historicalhas_injectible_lines/injectible_lineskeys, so the comment contradicted the implementation. Restored the original comment and renamed onescope_spec.rbtest description that propagated the same wrong wording.Extractor#extract_all. The production rescue is still in place — the deletion was an unintentional squash artifact, leaving the path uncovered.SymbolDatabase::RemoteandComponent: (a) addedComponent#loggerreader and replacedDatadog.loggerwithcomponent.loggerinRemote.process_change/enable_upload/disable_upload/parse_config(the receiver-block fallback atremote.rb:48keepsDatadog.loggerbecause the component lookup itself may return nil there); (b) added a justification comment to the# steep:ignore NoMethodatremote.rb:106matching the pattern at line 113; (c) replaced the over-defensiveComponent.send(:instance_variable_get, ...)inwait_for_idlewithComponent.instance_variable_get(...)and added a comment explaining why the public predicate isn't used (deadlock — already holdingComponent.upload_done_mutex).Core::Remote::Client::Capabilitiesthat registers SymDB capabilities only when both DI andsymbol_databaseare enabled. Neither branch was tested. Added three contexts covering both sides of the conditional plus its outer guard.SymbolDatabase::ComponentintoCore::Configuration::Components#initializeand#shutdown!but did not extendcomponents_spec.rb. Added an@symbol_databasedescribe block (disabled / enabled-with-RC contexts) and extended#shutdown!to assert SymDB receivesshutdown!when present.(Issue 8 from the review was the merged PR's description; updated separately via
gh pr editon #5717.)Motivation:
Cleanup of follow-up review findings on the merged #5717 wiring PR. None of these are functional bugs in shipped code — they're documentation drift, coverage gaps, and Component-Pattern hygiene. Folded together because they all touch the same surface area and individually wouldn't justify a PR.
Change log entry
None.
Additional Notes:
Yes — AI was used; I read and understood the changes.
How to test the change?
All passing locally: 220+ symbol_database examples + 40 components + 17 capabilities, Steep clean. CI must validate across the supported Ruby matrix.