Conversation
|
👋 Hey @p-datadog, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2026-07-01 23:56:10 UTC |
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 07d2353 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-05-05 00:34:29 Comparing candidate commit fa27be9 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 44 metrics, 1 unstable metrics.
|
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>
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>
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>
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.
Remove the build_symbol_database helper and lift its guard up into Components#initialize: build the component only when symbol_database_enabled? is true, assigning nil otherwise. The helper added nothing over the inline conditional now that the enablement decision and the build call sit one line apart.
- Rename Components.symbol_database_enabled? to enable_symbol_database?. - Remove the duplicate symbol_database_enabled? helper in Capabilities, inlining the settings-level resolution at its only call site. - Drop explanatory comments per review.
Extract the tri-state collapse (explicit value wins; nil yields a caller-supplied fallback) into Datadog::SymbolDatabase.resolve_enabled, called by both Core::Configuration::Components (fallback: DI's built component) and Core::Remote::Client::Capabilities (fallback: the DI setting, since the component tree does not exist at that layer). The branching logic now lives in one place; each layer supplies only its own fallback. The helper takes a plain boolean, so symbol database code still does not reference Dynamic Instrumentation.
Drop the per-layer explanation from the docstring.
The unset (nil) symbol_database default at the capabilities layer used settings.dynamic_instrumentation.enabled (the bare setting), which diverges from the DI branch's advertising condition (!DI::Remote.explicitly_disabled?). DI's enabled default is false, so in the default/unset state — where DI IS advertised so RC can enable it — symbol database would not follow DI. Use the same explicitly_disabled? check so symbol database advertises exactly when DI does. An explicit symbol_database.enabled still wins via resolve_enabled. Also drop the previous comment, which justified the fallback by noting the component tree does not exist yet (trivially true and beside the point). Add a capabilities spec for the DI-default/unset case.
Co-authored-by: domalessi <111786334+domalessi@users.noreply.github.com>
Root cause: with symbol_database.enabled unset (the default), the tri-state resolution treated a *buildable* DI component as "DI is running". Under the always-build model, DI::Component.build returns a non-nil component on any supported runtime (MRI, RC enabled, C ext present, non-dev) even when DD_DYNAMIC_INSTRUMENTATION_ENABLED is unset. So Components.enable_symbol_database? resolved to true and Capabilities advertised LIVE_DEBUGGING_SYMBOL_DB for every supported-runtime customer, and a LIVE_DEBUGGING_SYMBOL_DB RC (or force_upload) could trigger ObjectSpace symbol extraction and upload for applications that never enabled DI. This contradicted the documented/agreed guarantee that Symbol Database is off for tracing/profiling/appsec-only customers. Fix: complete the mirror-DI design. Symbol Database is still advertised and built by default (so the RC channel is ready when DI is enabled implicitly via the UI), but the customer-observable effect (extraction + upload) is now gated on DI actually being active: - Component#upload_allowed? gates start_upload. force_upload and an explicit symbol_database.enabled = true both mean "upload regardless of DI"; only the nil-default case is gated on an injected DI-active predicate. - When an upload signal arrives while DI is inactive, the request is recorded (@upload_pending) and re-attempted via resume_pending_upload once DI is enabled. Tracing::Remote.process_config triggers this after DI implicit enablement, mirroring DI's own probe replay in handle_rc_enablement. - Components injects a live di_active proc (dynamic_instrumentation&.started?) so Symbol Database holds no direct DI reference; the two modules stay independent and orchestration owns the cross-feature knowledge. - Corrected the enable_symbol_database? and configuration.rb comments, which claimed the DI build result was nil when DI was disabled. Verified: spec/datadog/symbol_database/component_spec.rb and spec/datadog/core/configuration/components_spec.rb pass; steep clean on the changed files (pre-existing single-file RubyVersion resolution artifact aside).
The docs claimed Symbol Database upload "tracks the Dynamic Instrumentation feature gate" / "tracks DD_DYNAMIC_INSTRUMENTATION_ENABLED". Reword to the actual behavior: by default it uploads symbols only when DI is actually enabled (explicitly or via implicit UI enablement) and stays off otherwise; an explicit DD_SYMBOL_DATABASE_UPLOAD_ENABLED / c.symbol_database.enabled overrides in either direction (true uploads regardless of DI, false disables entirely).
component_spec: the nil-default case defers start_upload when DI is inactive (no extraction scheduled, @upload_pending set) and runs it via resume_pending_upload once DI becomes active; explicit symbol_database.enabled = true and force_upload both upload regardless of DI. components_spec: correct the misleading premise that a non-nil DI component means "DI is running" — enable_symbol_database? only decides whether to build the (inert) component; upload is gated separately on DI being active.
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>
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.
Address review comments (Copilot): the YARD @param/@return types for `logger` documented Datadog::Core::Logger, but DI::Component wraps the core logger in a DI::Logger facade (component.rb:71) before passing it to ProbeNotificationBuilder and CaptureExpressionEvaluator. The RBS sigs already declare DI::Logger; this aligns the YARD docs with actual usage. - Fixed in lib/datadog/di/probe_notification_builder.rb:18 (@param, initializer) - Fixed in lib/datadog/di/probe_notification_builder.rb:42 (@return, attr_reader) - Fixed in lib/datadog/di/capture_expression_evaluator.rb:28 (@param, initializer) - Fixed in lib/datadog/di/capture_expression_evaluator.rb:52 (@return, attr_reader)
Address review comment (Copilot): the ProbeNotificationBuilder comment described capture_expression_evaluator as "Lazily-constructed", but it is built unconditionally in the initializer. Update the comment to match the implementation. The Instrumenter's evaluator (instrumenter.rb:87) is genuinely lazy (`||=`) and its comment is left unchanged. - Fixed in lib/datadog/di/probe_notification_builder.rb:50
Address review comment (codex): @duration compiles to `(context.duration * 1000)`. The entry-time capture-expression path added in this PR builds the Context with duration: nil, so any capture expression referencing @duration (even isUndefined(@duration), whose argument is evaluated first) raised NoMethodError on `nil * 1000` and the key was dropped with an evaluation error — unlike @return/@exception, which resolve to nil at entry. Guard the multiplication so @duration resolves to nil (undefined) at entry and still yields duration*1000ms at exit. duration: 0 stays 0 (0 is truthy). - Fixed in lib/datadog/di/el/compiler.rb (@duration expansion) - Added mechanism coverage in spec/datadog/di/el/duration_spec.rb (entry nil -> nil / isUndefined true; exit 0.5 -> 500.0 / isUndefined false) - Added e2e coverage in spec/datadog/di/integration/instrumentation_spec.rb (method probe, evaluateAt: ENTRY, @duration capture expression captured as null value with no evaluation error) Verified: rspec (mechanism 4, e2e 1, EL 147, capture-expression 11, notification/instrumenter/limits/dispatch 154, integration+builder 81), StandardRB clean, Steep clean.
…-off' into base-5540
Base Branch
Combines:
Last synced: 2026-07-01