DEBUG-5089 Enable symbol database when dynamic instrumentation is enabled#5828
Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2026-05-28 00:18:30 UTC |
dd-octo-sts
Bot
added
the
debugger
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c297bba664
ℹ️ 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".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 38dc4dd | Docs | Datadog PR Page | Give us feedback! |
Replaces the unconditional `true` default with a block that reads `Datadog.configuration.dynamic_instrumentation.enabled` at access time. The default is evaluated lazily on first `get`, so env vars and explicit assignment continue to take precedence via the existing chain. Spec asserts both branches (DI on → symdb default true, DI off → symdb default false) and verifies the env var still wins over the derived default. Docs updated to describe the conditional default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
p-datadog
changed the title
Typing analysisNote: Ignored files are excluded from the next sections.
|
The DI Settings module is extended lazily by lib/datadog/di.rb (via Datadog::DI::Extensions.activate!) and is not always loaded before symbol_database's default fires. CI hit `NoMethodError: undefined method 'dynamic_instrumentation' for ... Settings` when a Settings instance was constructed without DI's extension active. Default now returns false in that scenario (no DI loaded → no symdb), which preserves the intended "symdb tracks DI" semantics. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
BenchmarksBenchmark execution time: 2026-07-03 21:38:42 Comparing candidate commit 38dc4dd in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
Address codex P2 review comment on PR #5828 (configuration.rb:34). The previous default for symbol_database.enabled was config.respond_to?(:dynamic_instrumentation) && config.dynamic_instrumentation.enabled which ties symdb to DI's user-intent setting. The setting being true is not the same proposition as 'DI is actually going to run': DI::Component.build refuses to start in Rails development mode, on non-MRI engines, when the DI C extension failed to load, and when Remote Configuration is off. Under the old default, an app that set DD_DYNAMIC_INSTRUMENTATION_ENABLED=true in one of those environments would still default symdb to true, advertise the LIVE_DEBUGGING_SYMBOL_DB remote-config capability, and run ObjectSpace extraction even though no DI component would ever consume the symbols. Expand the default block to consult DI::Component.environment_supported? in addition to the DI setting. The two are now evaluated together at default-resolution time, keeping symdb's default in lockstep with DI's actual start gate. environment_supported? takes a logger; pass the new Datadog::Core::NULL_LOGGER (also added in this commit) so the diagnostic warnings emitted by the predicate are suppressed here \u2014 DI::Component.build runs shortly afterward with the operational logger and emits them once at the right layer. The gate sits at the default-value layer, not at Component.build. An explicit DD_SYMBOL_DATABASE_UPLOAD_ENABLED=true (or programmatic settings.symbol_database.enabled = true) wins over the default and enables symdb regardless of DI's state \u2014 Symbol Database and Dynamic Instrumentation are independently configured features and the default is a UX convenience for the common case, not a structural coupling. An inline code comment on the option records this explicitly. Files: - lib/datadog/core/null_logger.rb (new): Datadog::Core::NULL_LOGGER \u2014 frozen ::Logger.new(::IO::NULL). Process-wide singleton for cases where an API takes a logger but the caller has a structural reason not to emit output. - sig/datadog/core/null_logger.rbs (new): Steep signature. - spec/datadog/core/null_logger_spec.rb (new): 17 examples covering type, frozen-ness, singleton identity, all standard log methods, and no-output behavior. - lib/datadog/symbol_database/configuration.rb: expanded default block with the DI-environment gate and the independence comment. - spec/datadog/symbol_database/configuration_spec.rb: new context 'when dynamic_instrumentation.enabled is true but DI environment is not supported' \u2014 asserts default-derived false and explicit-env- override-wins. Existing positive test updated to set internal.development = true (the documented dev-mode escape hatch) so rspec's Execution.development? = true doesn't make the new gate evaluate to false in the spec env. Verified: - bundle exec rspec spec/datadog/symbol_database/ spec/datadog/core/null_logger_spec.rb spec/datadog/core/logger_spec.rb: 357 examples, 0 failures - bundle exec rake standard typecheck: clean Co-Authored-By: Claude <noreply@anthropic.com>
|
So how does this interact with #5525 ? Specifically, does this mean that symbol database will get enabled if DI gets enabled (via remote configuration or local config) OR is there any situation where symbol database gets enabled for existing customers not using DI? |
|
@ivoanjo SymDB will be enabled by default if DI is enabled, via either of the routes (local configuration via env var or implicit enablement via RC). The reason is that, from customer's point of view, SymDB is needed to make DI UI actually work/be usable. SymDB has independent toggles and may be turned off or on explicitly (via env var, etc.) regardless of DI state. So, a customer may turn on SymDB while leaving DI off. I am unaware of any guidance instructing customers to do this presently but this might be a use case that materializes in the future (AI-adjacent products/tooling would be my guess). |
Got it -- I have no concerns with customers that opt-into the feature (even when it's not being used); although we could explore some kind of warning that this configuration is not recommended. So just to be extra sure: for customers using right now only combinations of tracing/profiling/appsec and not going into DI in the UX, are they guaranteed that SymDB will not be turned on? This is the case I want to make sure we don't impact such customers. |
|
Yes SymDB will be off in those cases. SymDB is enabled via user actions only: either env var set or user turning Live Debugger on in UI. |
|
Ack, perfect, I think that's a great compromise 🙏 |
The newly added "when dynamic_instrumentation.enabled is true" test expected the symdb default to resolve to true after setting dynamic_instrumentation.enabled = true and internal.development = true. Locally it passed because the libdatadog_api C extension was present, which satisfies environment_supported?'s DI.respond_to?(:exception_message) check. In CI's spec:main job that extension is not compiled, so environment_supported? returned false and the default fell back to false. Fix by stubbing environment_supported? to return true \u2014 the same pattern the sibling "DI environment is not supported" context uses (with return value false). This isolates the layering under test (default tracks DI's runtime gate) from the predicate's internal mechanics, and removes the dev-mode escape hatch that was being used as a partial stand-in for the full environment_supported? contract. Fixes the 12 CI failures on PR #5828: - Ruby 2.5\u20134.0 / build & test (standard) [0] - Test Nix (x86_64-linux, arm64-darwin, aarch64-linux, 24.05) all hitting: spec/datadog/symbol_database/configuration_spec.rb:98 Failure/Error: expect(settings.symbol_database.enabled).to be(true) expected true, got false Co-Authored-By: Claude <noreply@anthropic.com>
Comment had a trailing )> typo. Also drops the respond_to?(:dynamic_instrumentation) and defined?(::Datadog::DI::Component) guards — both are always true when datadog is loaded normally (di/component is transitively required via core/configuration/components.rb), so the if/else just adds noise. Co-Authored-By: Claude <noreply@anthropic.com>
… loaded
spec/loading_spec.rb "load core only and configure library with no
settings" failed on every Ruby version and on macOS/Nix (18 CI jobs, all
the same single example at loading_spec.rb:94).
Root cause: the symbol_database.enabled default block, added in this PR,
read `Datadog.configuration.dynamic_instrumentation.enabled`. The
dynamic_instrumentation settings group is only added to core
Configuration::Settings by Datadog::DI::Configuration::Settings, which is
extended only when datadog/di is loaded. The loading_spec example requires
datadog/core alone and calls Datadog.configure {}, which builds components;
SymbolDatabase::Component.build reads settings.symbol_database.enabled,
evaluating the default block. With DI not loaded, `config.dynamic_instrumentation`
is an undefined method -> NoMethodError -> the subprocess exits non-zero ->
the example fails. (The "load datadog and enable DI" example passed because
DI is loaded there.)
Fix: guard the default block so the DI-absent case returns false without
dereferencing DI. Check `config.respond_to?(:dynamic_instrumentation)` before
reading it, and `defined?(::Datadog::DI::Component)` before calling
environment_supported?. When DI isn't loaded it is off, so SymDB correctly
defaults to off. When DI is loaded the behavior is unchanged.
Coverage: spec/loading_spec.rb (core-only configure) is the regression test
for the DI-absent path; spec/datadog/symbol_database/configuration_spec.rb
covers the DI-present true/false paths (DI is loaded in that suite). The
guard's short-circuit was verified in isolation (DI-absent -> false, no
raise; DI-present+enabled -> environment_supported? path).
Verified: ruby -c clean; guard short-circuit verified standalone. The full
loading_spec was not run locally -- it requires the installed gem
dependencies and `bundle install` fails here with Bundler::PermissionError
(gem dir not writable). CI validates across the matrix.
The comment stated the downstream component logs the same condition with the real logger 'shortly afterward'. That is wrong on two counts: the settings-default block is evaluated lazily on first config read and can run before, after, or independently of the component build; and on the remote-config-disabled path DI::Component.build returns before reaching environment_supported?, so the condition is not logged via DI at all. Rephrase to state only what holds: the owning component logs the condition with the real logger when it evaluates the predicate during its own build, and this sink keeps the config-read path silent.
…/core is loaded" This reverts commit f6eca7d.
Drops the require_relative and extend of SymbolDatabase::Configuration::Settings from core's Settings class.
The previous commit removed the symbol_database settings group from core's Settings class. Re-register it via a new SymbolDatabase::Extensions.activate!, invoked from datadog/di.rb alongside DI's own Extensions.activate!. This keeps the invariant that the symbol_database settings group exists only when the dynamic_instrumentation settings group does, so the symbol_database.enabled default can read dynamic_instrumentation.enabled without guarding for an absent DI settings group: - Full library load (require 'datadog' -> datadog/di): both groups registered; the default evaluates DI's enabled + environment_supported?. - Core-only load (require 'datadog/core'): neither group registered; SymbolDatabase::Component.build's respond_to?(:symbol_database) guard short-circuits, so the default is never evaluated and nothing raises. The extensions file is deliberately not required by datadog/core.
p-datadog
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f1783b359
ℹ️ 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".
The resume_pending_upload replay was unit-tested on the component, but the Tracing::Remote.process_config wiring that triggers it on a DI enable signal was untested. Add process_config specs: dynamic_instrumentation_enabled=true replays the deferred Symbol Database upload; =false does not.
- Use argument form for the static-string deferral debug log (block form is for interpolated strings; matches the arg-form convention elsewhere in the file). - Add a reason to the # steep:ignore NoMethod on the di_active call (Steep does not narrow the proc to non-nil after the .nil? check). - Document SymbolDatabase::Extensions.activate!. - Tighten the enable_symbol_database? inline comment: the default follows whether DI's component was built, not "runtime readiness" (the component is built-but-inert when DI defaults off).
Two review findings on the DI-active upload gate: - codex (tracing/remote.rb): when a later APM_TRACING payload disables DI, the DI component stops but Symbol Database's TracePoint/scheduler stayed armed and kept uploading in the nil-default (follows-DI) case. Add Component#stop_for_di_disable and call it from Tracing::Remote when RC disables DI. Only the nil-default case stops; an explicit symbol_database.enabled = true (or force_upload) is independent of DI and keeps running. - Copilot (component.rb): start_upload marked @upload_pending for any upload_allowed? == false, so an explicit symbol_database.enabled = false would be retried by resume_pending_upload. Only defer (and retry) when blocked by the nil-default DI gate (deferred_by_di_gate?); for an explicitly disabled feature, clear pending and log a skip message. Tests: component_spec covers stop_for_di_disable (nil-default stops; explicit true and force_upload keep running) and explicit-false start_upload (no defer, no pending). remote_spec covers process_config stopping/replaying Symbol Database on DI disable/enable.
codex: with symbol_database.enabled at its nil default, capabilities advertised LIVE_DEBUGGING_SYMBOL_DB whenever DI advertised, but DI supports Ruby 2.6 while Symbol Database requires MRI 2.7+. On 2.6 the product was advertised with no component to service the upload config. Add Datadog::SymbolDatabase.supported? (MRI 2.7+, no logging) and gate the capabilities registration on it; Component.environment_supported? now delegates to it as the single source of truth. Test: capabilities_spec asserts the product is not advertised when SymbolDatabase.supported? is false, while DI is still advertised.
Copilot: the settings table listed c.symbol_database.enabled as Boolean, but it is now tri-state (nil = follows DI). Update the type column to "Boolean, nil".
…symdb-default-off # Conflicts: # lib/datadog/core/remote/client/capabilities.rb # lib/datadog/symbol_database.rb # lib/datadog/symbol_database/component.rb # sig/datadog/symbol_database.rbs # spec/datadog/core/remote/client/capabilities_spec.rb
Tracing::Remote.process_config now replays (resume_pending_upload) or stops (stop_for_di_disable) the Symbol Database component when a dynamic_instrumentation_enabled RC signal arrives — added in this PR to sync Symbol Database lifecycle with DI enable/disable via remote config. The pre-existing implicit_enablement_spec builds a Components stand-in (instance_double) that did not expose symbol_database, so process_config raised "received unexpected message :symbol_database" on every RC enable/disable, failing all 9 examples on Ruby 2.7+. Expose a Datadog::SymbolDatabase::Component instance_double on the Components stand-in that accepts the two lifecycle calls. Symbol Database upload behavior itself is verified in spec/datadog/tracing/remote_spec.rb; this DI integration spec only needs the calls to not raise. Verified locally on Ruby 3.2 (libdatadog_api extension compiled): 10 examples, 0 failures (was 9 failures). Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ce1c1d4cd
ℹ️ 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".
| | Environment variable | Type | Description | Default | | ||
| |---|---|---|---| | ||
| | `DD_SYMBOL_DATABASE_UPLOAD_ENABLED` | `Boolean` | Enable or disable symbol database upload. | `false` | | ||
| | `DD_SYMBOL_DATABASE_UPLOAD_ENABLED` | `Boolean` | Enable or disable symbol database upload. | Unset: uploads only when Dynamic Instrumentation is actually enabled | |
There was a problem hiding this comment.
nit: For consistency with the c.symbol_database.enabled, should this also say that Unset refers to nil?
There was a problem hiding this comment.
I don't think so because the env vars and settings are different: the settings are always defined (and thus might have nil as value), but env vars can be not set (in which case referencing ENV['WHATEVER'] would return nil but it's not because WHATEVER is actually in ENV, if it was the value would be the empty string).
The existing text is consistent with the DD_DYNAMIC_INSTRUMENTATION_ENABLED text above which also uses "unset" terminology.
When symbol_database.enabled is nil (follows-DI default) and DI is toggled off then on via remote configuration, uploads did not resume. On DI disable stop_for_di_disable called stop_upload, which cleared the deferral flag; resume_pending_upload then found nothing pending, and remote config does not re-dispatch the unchanged LIVE_DEBUGGING_SYMBOL_DB config, so the tracer never restarted extraction. This mirrors the bug DI solves with replay_current_probes. Track a sticky @upload_requested desire, set whenever an upload is requested and allowed or deferred by the DI gate, cleared only when RC explicitly disables uploads. stop_for_di_disable now suspends scheduling (new private suspend_scheduling) without clearing the desire, so resume_pending_upload restarts the upload on DI re-enable. Verified: component_spec + components_spec green, standardrb and steep clean.
DD_INTERNAL_FORCE_SYMBOL_DATABASE_UPLOAD is documented as always uploading, but enable_symbol_database? ignored it: with symbol_database.enabled unset (nil) and DI's component not built, the resolver returned false, so the component was never built and the force-upload path inside Component.build was unreachable. Return true when force_upload is set so the component is built and force mode works independent of DI.
Type the dynamic_instrumentation parameter as Datadog::DI::Component? (matching the components attr_reader) instead of untyped; settings stays untyped like the other build_* signatures because the symbol_database group is registered dynamically. Remove a spurious double blank line in capabilities.rbs.
@upload_pending was assigned in initialize, start_upload, and suspend_scheduling but never read anywhere in lib/ — its comment claimed "for logging only" while the log branches use static strings. The only readers were spec assertions, coupling tests to dead internal state. The deferred-vs-disabled distinction that mattered is already carried by @upload_requested (read by resume_pending_upload): a DI-gate deferral sets it true so the upload resumes when DI is enabled; an explicit disable clears it. Redirect the affected spec assertions to @upload_requested and drop @upload_pending from lib, rbs, and the @upload_requested comment. Also return nil from the void lifecycle methods suspend_scheduling and resume_pending_upload so they honor their `-> void` RBS contract instead of leaking Array#clear / start_upload return values. Verified: rspec spec/datadog/symbol_database/component_spec.rb (61 examples, 0 failures); rake typecheck clean.
deferred_by_di_gate? duplicated upload_allowed?'s force_upload / enabled guards and the `@di_active.nil? || @di_active.call` gate expression, carrying its own steep:ignore for the @di_active narrowing. After the force_upload and enabled.nil? guards, upload_allowed? reduces to exactly the DI-active gate, so a disallowed upload there is precisely a DI-gate deferral. Express it as `!upload_allowed?`, removing the duplicated gate and the second steep:ignore. Verified: rspec spec/datadog/symbol_database/component_spec.rb (61 examples, 0 failures); rake typecheck clean.
p-datadog
changed the title
What does this PR do?
Makes
symbol_database.enableda tri-state setting (true/false/nil) that defaults tonil, and resolves the unconfigured default in the core orchestration layer to enable SymDB when DI is actually enabled.Accounts for DI implicit enablement - SymDB component is built when DI component is built but there is no SymDB upload scheduled until DI is actually enabled via RC.
Symbol Database upload behavior
LIVE_DEBUGGING_SYMBOL_DBDD_DYNAMIC_INSTRUMENTATION_ENABLED=false),symbol_databaseunsetsymbol_database.enabled = falsesymbol_database.enabled = true(DI off)DD_DYNAMIC_INSTRUMENTATION_ENABLED=true)force_upload(DD_INTERNAL_FORCE_SYMBOL_DATABASE_UPLOAD, internal/testing only) always uploads regardless of DI.Constraints
Datadog::SymbolDatabaseandDatadog::DIdo not reference each other. The DI-active check is an opaque proc injected byCore::Configuration::Components; the orchestration layer owns the cross-feature knowledge.symbol_database.enabled = trueuploads even with DI off;= falsenever uploads even with DI on.DD_DYNAMIC_INSTRUMENTATION_ENABLED=false⇒ no advertising, no build, no upload whensymbol_database.enabledis unset. An explicitsymbol_database.enabled=trueis an independent opt-in and still advertises/builds/uploads (Constraint 3 wins).upload_symbols: truedoes not extract when DI is inactive.allow_initialization: false, since they run on the remote-config worker thread.force_uploadmeans always upload. The internal/testingforce_uploadoverride bypasses the DI-active gate.Motivation:
Symbol Database powers the Dynamic Instrumentation autocomplete UI, so it should upload symbols when DI is actually usable and stay off otherwise — without coupling the two independent modules or running ObjectSpace extraction in environments where DI never starts.
Change log entry
Yes. Dynamic Instrumentation: Symbol Database is now enabled by default when DI is enabled.
How to test the change?
Unit tests + manual testing.
Manual test with DI being implicitly enabled:
