Repository navigation
[Configuration] Keep invalid results in composite configuration source - #9522
Open
MilanGarnier wants to merge 1 commit into
Open
MilanGarnier wants to merge 1 commit into
MilanGarnier wants to merge 1 commit into
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9522) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-10-26 15:02:41 Comparing candidate commit 549ceb6 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 13 known flaky benchmarks, 113 flaky benchmarks without significant changes.
|
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 549ceb603b
ℹ️ 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".
Comment on lines
+82
to
+85
| if (!result.IsValid) | ||
| { | ||
| result = value; | ||
| } |
There was a problem hiding this comment.
The same priority-selection block is now duplicated across all six getters, so a future change to the composite precedence contract can silently diverge by value type. Extract this into a private generic helper and keep the selection rule in one place, as required by MAINT-1.2.
AGENTS.md reference: AGENTS.md:L280-L280
Useful? React with 👍 / 👎.
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
Valid values still override invalid values from any source, while all-invalid lookups retain the highest-priority failure for settings’ existing fallback and warning paths.
🤖 Bits Code Review · Commit 549ceb6
Mascarpone
approved these changes
Oct 27, 2026
This branch has not been deployed
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.
Summary of changes
CompositeConfigurationSourcenow returns the highest-priority invalid result (ValidationFailure/ParsingError) when no source has a valid value, instead ofNotFound. A valid value from any source still wins over an invalid one.This makes production follow the invalid-input handling that settings code and unit tests already expect. New behavior, by category:
1. Warnings for unsupported or invalid input
OTEL_PROPAGATORS: together with [Configuration] Ignore unsupported OTEL_PROPAGATORS values with a warning #9515, a value with only unsupported propagators (e.g.jaeger) logs a warning before falling back to the defaults.OTEL_*values (OTEL_LOG_LEVELother thandebug,OTEL_SDK_DISABLED,OTEL_METRICS_EXPORTER,OTEL_LOGS_EXPORTER,OTEL_TRACES_EXPORTER) log a warning too, as does an invalid DD key set next to its OTel equivalent.2. Telemetry
OpenTelemetryConfigInvalidandOpenTelemetryConfigHiddenByDatadogConfigare recorded, and configuration telemetry ends with the value actually in use.3. Behavior on invalid input
DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODEorDD_APPSEC_AUTOMATED_USER_EVENTS_TRACKINGdisables user tracking.DD_TRACE_ENABLED,DD_TRACE_SAMPLE_RATE) hides its OTel equivalent, and a key that fails validation no longer falls back to its aliases, like with a single source.Reason for change
Production settings always read through
GlobalConfigurationSource, which is aCompositeConfigurationSource. It dropped every invalid result, so callers could never seeIsPresent && !IsValid, and the code written for that case never ran. Unit tests use a singleNameValueConfigurationSource, which keeps the invalid result, so they didn't catch it.Impact in production today, by category (importance in brackets):
1. No warning for unsupported or invalid input
OTEL_PROPAGATORS: [Configuration] Ignore unsupported OTEL_PROPAGATORS values with a warning #9515 rejects values that contain only unsupported propagators, such asjaeger,ottraceorxray, and warns about them. Without this fix that warning never fires, and propagation falls back to the defaults without any message, which can silently break distributed traces with services using other propagators.OTEL_*values either, notablyOTEL_LOG_LEVELwhere onlydebugis supported, nor for an invalid DD key set next to its OTel equivalent.2. Telemetry
OpenTelemetryConfigInvalidis never recorded,OpenTelemetryConfigHiddenByDatadogConfigis missing when the DD key is invalid, and configuration telemetry reports the rejected value instead of the default in use.3. Behavior on invalid input
DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE(or the deprecatedDD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING) turns on user tracking inidentificationmode instead of disabling it, which collects user data the customer likely meant to restrict.DD_TRACE_ENABLED=abc+OTEL_TRACES_EXPORTER=nonedisables tracing, an unparseableDD_TRACE_SAMPLE_RATElets the OTel sampler apply), and aliases are tried after a failed validation.Implementation details
In each getter, a present-but-invalid result replaces the current result while no valid one has been found. Sources are iterated from lowest to highest priority, so the highest-priority invalid result is kept. Telemetry recording is unchanged.
Test coverage
SettingsTestsBase.CreateConfigurationSourcenow wraps its source in aCompositeConfigurationSource, so all settings tests built on it run the production path.CompositeConfigurationSourceTests.ReturnsHighestPriorityInvalidResult_WhenNoSourceIsValid, andTelemetry_WhenErrorRecordsTelemetryupdated to expectParseFailure.SecuritySettingsTests.UserEventsAutoInstrumentationMode, and aMutableSettingsTests.TraceEnabledcase for an invalidDD_TRACE_ENABLEDnext toOTEL_TRACES_EXPORTER=none, both through the composite source.Other details
Found while working on #9515.
Existing tests already encode this behavior, and now apply in production too: with an invalid
DD_RUNTIME_METRICS_ENABLEDandOTEL_METRICS_EXPORTER=none, the OTel key is reported as hidden, butnonestill disables runtime metrics (TracerSettingsTests.RuntimeMetricsEnabled).🤖 Generated with Claude Code