Repository navigation
Resolve Feature Flags enablement from instrumenter config - #12845
PerfectSlayer wants to merge 6 commits into
Conversation
PerfectSlayer
added
tag: no release notes
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
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. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Feature-flag enablement retains the existing resolver precedence after moving into InstrumenterConfig. Source inspection of configuration resolution, startup ordering, and target-system filtering found no reportable regression.
🤖 Bits Code Review · Commit 0b5a6d8
| private static boolean agentlessLogSubmissionEnabled = false; | ||
| private static boolean appLogsCollectionEnabled = false; | ||
| private static boolean featureFlaggingEnabled = false; | ||
| private static volatile boolean featureFlaggingEnabled = false; |
There was a problem hiding this comment.
just wondering - how did you know to make this volatile?
There was a problem hiding this comment.
This is now set in another thread, no longer initialize in Agent.start() like the other.
I already verified it because I find it weird myself, but having the same question from you again makes me think there is something wrong on the design. I'll keep refining it. Good call.
There was a problem hiding this comment.
The feature flagging config module is no longer needed with this new design. You raised concerned in the past about adding to the internal-api module so I went ahead and simplified the config as any other product - Config/InstrumenterConfig logic + config keys in a dedicated class in the public API.
The product activation detection was also simplified in agent to align with the other product behavior, but the overall FFE config resolution is still in place later down in the product itself.
| @@ -276,6 +270,12 @@ private InstrumenterConfig() { | |||
| CODE_ORIGIN_FOR_SPANS_INTERFACE_SUPPORT, | |||
| DEFAULT_CODE_ORIGIN_FOR_SPANS_INTERFACE_SUPPORT); | |||
| traceEnabled = configProvider.getBoolean(TRACE_ENABLED, DEFAULT_TRACE_ENABLED); | |||
| featureFlaggingEnabled = | |||
| resolveConfiguration( | |||
| configProvider.getBoolean(FEATURE_FLAGS_ENABLED), | |||
There was a problem hiding this comment.
A Codex review pointed out that previously the parser needed exactly 1 or true for feature flags to be enabled, but now empty or whitespace-only values will also result in enabling this flagwith ConfigProvider.getBoolean(). Setting -Ddd.feature.flags.enabled= probably isn't likely, but should we preserve the previous behavior just in case?
There was a problem hiding this comment.
For a bit of context, this is an issue with the current implementation on master: (2 AI bullet points below for context)
- Master wasn't consistent with itself. Its Config already read this setting with configProvider.getBoolean(FEATURE_FLAGS_ENABLED), with the same blank-means-unset rule. On master, -Ddd.feature.flags.enabled= disabled the product at startup while Config.isFeatureFlaggingProviderEnabled() reported it enabled. Resolve Feature Flags enablement from instrumenter config #12845 removes that split.
- The SDK does the same. Settings.getBoolean in dd-openfeature treats blank as unset, and ConfigurationResolutionParityTest relies on the agent and the SDK resolving the same way. Restoring the old behaviour in the agent alone would break that parity.
Overall, I went for:
- consistency - having the product activation and flag parsing as any other product to avoid surprising customers
- simplicity - reuse the existing mechanism in order to have the less code duplication and later maintenance.
I also turned the source to an enum and use the default enum parsing behavior for config following those two principles.
There was a problem hiding this comment.
An explicit Feature Flags configuration source no longer overrides a disabled deprecated provider toggle at bootstrap, so an otherwise enabled subsystem never starts.
🤖 Bits Code Review · Commit 670baa8
Move the Feature Flags enablement resolution to InstrumenterConfig and add the FEATURE_FLAGS instrumentation target system, so instrumentations can be gated on the product being enabled. The agent now reads it from Config instead of resolving the settings itself at premain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve the Feature Flags product activation in Agent.start() with the other products, so it no longer needs to be volatile.
Resolve the product switch and configuration source once in InstrumenterConfig, with Config delegating to it, instead of resolving them twice through feature-flagging-config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the hand-written source normalization with the agent enum config parsing. A blank source is unset, and an unknown or padded one is invalid. Config keeps exposing the source as a string for the existing feature flagging code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Checking the deprecated experimental toggle at agent start disabled the product even when an explicit configuration source wins over it. Only check the product switch there, and let the feature flagging system apply the full configuration resolution. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
45abc33 to
118201b
Compare
InstrumenterConfig is initialized at native-image build time and now holds a FeatureFlaggingSource value, so the enum must be initialized at build time too, like the other config enums. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
More details
Explicit disablement retains precedence over source selection and the legacy toggle, and the new instrumentation target uses resolved enablement. Runtime behavior was assessed from source only.
🤖 Bits Code Review · Commit 6b3d5a1
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
Use ⏳ Waiting for pull request to become mergeable |
This branch has not been deployed
2 participants
What Does This Do
Moves the Feature Flags configuration resolution (product switch and configuration source) to
InstrumenterConfig, parsing the source as aFeatureFlaggingSourceenum, and adds theFEATURE_FLAGSinstrumentation target system. The agent detects the product activation inAgent.start()like the other products.Motivation
The upcoming
dd-openfeature-connectorinstrumentation needs to be gated on Feature Flags being enabled. This is the first PR of the stack that makesdd-openfeaturea standalone SDK (APMLP-1512).Additional Notes
Configdelegates toInstrumenterConfigand still returns the source as a string until #12844.Stack
The full change is in #12709. It was split into this stack to ease the review.
Jira ticket: APMLP-1512
🤖 Generated with Claude Code