Skip to content

SymDB: review follow-ups for #5717#5771

Merged
p-datadog merged 6 commits into
masterfrom
symbol-database-review-followups
May 18, 2026
Merged

SymDB: review follow-ups for #5717#5771
p-datadog merged 6 commits into
masterfrom
symbol-database-review-followups

Conversation

@p-datadog

Copy link
Copy Markdown
Member

What does this PR do?

Addresses the review findings raised against #5717 (already merged). Each commit is one focused finding.

# Commit Issue
1 SymDB scope: restore wire-format-vs-Ruby-identifier comment Scope :targetable_lines comment at scope.rb:25 was rewritten in #5717 and lost the Ruby-identifier vs wire-format-key distinction. The same file at lines 83-88 still serializes with the historical has_injectible_lines/injectible_lines keys, so the comment contradicted the implementation. Restored the original comment and renamed one scope_spec.rb test description that propagated the same wrong wording.
2 SymDB extractor spec: restore top-level rescue test #5717 deleted the spec that exercised the top-level rescue in Extractor#extract_all. The production rescue is still in place — the deletion was an unintentional squash artifact, leaving the path uncovered.
3 + 6 + 7 SymDB remote: route diagnostics through component.logger Three Component-Pattern / clarity fixes in SymbolDatabase::Remote and Component: (a) added Component#logger reader and replaced Datadog.logger with component.logger in Remote.process_change / enable_upload / disable_upload / parse_config (the receiver-block fallback at remote.rb:48 keeps Datadog.logger because the component lookup itself may return nil there); (b) added a justification comment to the # steep:ignore NoMethod at remote.rb:106 matching the pattern at line 113; (c) replaced the over-defensive Component.send(:instance_variable_get, ...) in wait_for_idle with Component.instance_variable_get(...) and added a comment explaining why the public predicate isn't used (deadlock — already holding Component.upload_done_mutex).
4 SymDB capabilities spec: cover the new symbol_database branch #5717 added a new conditional in Core::Remote::Client::Capabilities that registers SymDB capabilities only when both DI and symbol_database are enabled. Neither branch was tested. Added three contexts covering both sides of the conditional plus its outer guard.
5 SymDB components spec: cover symbol_database wiring and shutdown #5717 wired SymbolDatabase::Component into Core::Configuration::Components#initialize and #shutdown! but did not extend components_spec.rb. Added an @symbol_database describe block (disabled / enabled-with-RC contexts) and extended #shutdown! to assert SymDB receives shutdown! when present.

(Issue 8 from the review was the merged PR's description; updated separately via gh pr edit on #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?

bundle exec rspec spec/datadog/symbol_database/ \
                  spec/datadog/core/configuration/components_spec.rb \
                  spec/datadog/core/remote/client/capabilities_spec.rb
bundle exec steep check sig/datadog/symbol_database lib/datadog/symbol_database

All passing locally: 220+ symbol_database examples + 40 components + 17 capabilities, Steep clean. CI must validate across the supported Ruby matrix.

p-ddsign added 5 commits May 18, 2026 12:15
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.
@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 18, 2026
@dd-octo-sts dd-octo-sts Bot added the debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database) label May 18, 2026
@dd-octo-sts

dd-octo-sts Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 3 steep:ignore comments, and clears 3 steep:ignore comments.

steep:ignore comments (+3-3)Introduced:
lib/datadog/symbol_database/remote.rb:108
lib/datadog/symbol_database/remote.rb:115
lib/datadog/symbol_database/scope.rb:89
Cleared:
lib/datadog/symbol_database/remote.rb:106
lib/datadog/symbol_database/remote.rb:113
lib/datadog/symbol_database/scope.rb:87

Untyped methods

This PR introduces 1 partially typed method, and clears 1 partially typed method.

Partially typed methods (+1-1)Introduced:
sig/datadog/symbol_database/remote.rbs:22
└── def self.parse_config: (Datadog::Core::Remote::Configuration::Content content, SymbolDatabase::Logger logger) -> ::Hash[::String, untyped]?
Cleared:
sig/datadog/symbol_database/remote.rbs:22
└── def self.parse_config: (Datadog::Core::Remote::Configuration::Content content) -> ::Hash[::String, untyped]?

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.

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented May 18, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 98.48%
Overall Coverage: 97.14% (-0.01%)

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

@p-datadog
p-datadog marked this pull request as ready for review May 18, 2026 17:21
@p-datadog
p-datadog requested a review from a team as a code owner 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".
@p-datadog
p-datadog merged commit 2546c3a into master May 18, 2026
581 checks passed
@p-datadog
p-datadog deleted the symbol-database-review-followups branch May 18, 2026 20:37
@dd-octo-sts dd-octo-sts Bot added this to the 2.34.0 milestone May 18, 2026
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 debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants