Default symbol_database.enabled to dynamic_instrumentation.enabled#5828
Default symbol_database.enabled to dynamic_instrumentation.enabled#5828p-datadog wants to merge 16 commits into
Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2026-05-28 00:18:30 UTC |
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: 888f78d | 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>
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-06-25 15:22:30 Comparing candidate commit 888f78d 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.
symbol_database.enabled becomes tri-state: true/false are explicit overrides; nil (the default) follows dynamic_instrumentation.enabled. The resolution lives in Core::Configuration::Components, the layer that already builds both components, so neither feature depends on the other: - DI does not load or reference symbol_database (di.rb reverted). - symbol_database config/component do not reference DI. - Components.symbol_database_enabled? resolves the tri-state from the settings and is the single source of truth; it is passed to SymbolDatabase::Component.build via a new required enabled: kwarg, and capabilities.rb resolves the same rule inline within its DI-on branch. symbol_database settings are registered off the DI load path (datadog.rb -> symbol_database/extensions), not in core, so a core-only load registers neither group and the build-time respond_to? guards keep it from raising. The config default no longer calls DI::Component.environment_supported?, so Core::Core::NULL_LOGGER and its spec/rbs are removed. Platform gating stays in SymbolDatabase::Component#environment_supported?. Verified locally (vendor bundle): symbol_database suite, components_spec, capabilities_spec, settings_spec, di settings_spec, and loading_spec all pass; Steep clean on changed files.
Summary
Makes
symbol_database.enableddefault to the value ofdynamic_instrumentation.enabledinstead of a fixed boolean.DD_SYMBOL_DATABASE_UPLOAD_ENABLEDor explicitc.symbol_database.enabled = …continue to override via the existing precedence chainThis replaces the unconditional
truedefault that #5818 was reverted to. The config DSL'so.default { … }block is evaluated lazily on firstget(seeDatadog::Core::Configuration::Option#default_value), so the value is taken from whatever DI is configured to at the timesymbol_database.enabledis first read — which in practice isComponent.build, after userDatadog.configureblocks and env vars have been applied.Precedent for this pattern:
lib/datadog/tracing/contrib/redis/configuration/settings.rband several other contrib settings use the sameo.default do … endshape.Changes
lib/datadog/symbol_database/configuration.rb: replaceo.default truewitho.default { Datadog.configuration.dynamic_instrumentation.enabled }; update the comment block.spec/datadog/symbol_database/configuration_spec.rb: drop the env-unset row from the env-var table (its value now depends on DI, which the env table doesn't model). Add a dedicated context that asserts both branches and that the env var still wins.docs/GettingStarted.md: default column now reads "TracksDD_DYNAMIC_INSTRUMENTATION_ENABLED" / "Tracksc.dynamic_instrumentation.enabled".Change log entry
Yes. Dynamic Instrumentation: Symbol Database upload now defaults to the value of
dynamic_instrumentation.enabledinstead of a fixed default; setDD_SYMBOL_DATABASE_UPLOAD_ENABLEDto override.Test plan
bundle exec rspec spec/datadog/symbol_database/— 331 examples, 0 failures