feat(feature-flags): add agentless configuration keys, settings, and endpoint derivation - #9040
feat(feature-flags): add agentless configuration keys, settings, and endpoint derivation#9040pavlokhrebto wants to merge 18 commits into
Conversation
pavlokhrebto
changed the title
pavlokhrebto
added
type:new-feature
AI Generated
BenchmarksBenchmark execution time: 2026-08-20 15:44:14 Comparing candidate commit 7c7d7e6 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 66 known flaky benchmarks, 60 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9040) and master. ✅ No regressions detected |
![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: ac79e9b3c4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
| Enables Feature Flags Provider (Experimental). | ||
| Default value is <c>false</c> (disabled). | ||
| DD_EXPERIMENTAL_FLAGGING_PROVIDER_INITIALIZATION_TIMEOUT_MS: | ||
| - implementation: B |
There was a problem hiding this comment.
I'm somewhat surprised there's diverging implementations already, is that necessary? 🤔
There was a problem hiding this comment.
The B implementation is for the native (C++) side of the tracer, which reads these keys through a separate config pipeline. We only own the A (managed) implementation in this PR — the native side is out of scope.
There was a problem hiding this comment.
If this is for the native side, then doesn't that mean this should be scoped: native, and we don't read it on the managed side? 🤔 Or am I misunderstanding?
There was a problem hiding this comment.
Previous robot answer was wrong, sorry — it mixed up implementation with scope. Managed vs native is scope, and this key is scope: managed, read only from managed code.
On your actual question: only this one key diverges, and it's the default value. We use 10000ms rather than the canonical 30000ms, and 10s is an already-registered variant (B) rather than something we invented. The reason for 10s is that timing out here isn't fatal — the provider just stays not-ready and evaluations return the caller's default until config arrives — so a long timeout only buys extra blocking time in InitializeAsync, which risks tripping container readiness probes and cold-start budgets. Python landed on the same 10s value.
Happy to move to A/30s if you'd rather avoid the divergence — nothing has shipped on 10s.
| /// </summary> | ||
| internal const string ManagedHostPrefix = "ufc-server.ff-cdn."; | ||
|
|
||
| private AgentlessEndpoint(Uri uri, bool isManaged) |
There was a problem hiding this comment.
Be aware, that you can still create instances of AgentlessEndpoint like this:
AgentlessEndpoint myEndpoint = default;In that scenario, IsManaged==false and Uri will be null despite the nullable annotation etc. Not saying it's necessarily an issue given that this is an internal-only type, just something to bear in mind, that it can be a foot gun.
Given that you're only going to create this type once, for the lifetime of the app, using a class would avoid the potential for mistakes, and the cost of an extra allocation. Up to you which you choose, as long as you understand the implication 🙂
There was a problem hiding this comment.
We kept it as a struct since it's internal, only ever created via TryCreate, and never boxed. Happy to change to a class if you'd prefer.
Tbh for me it looks more like a struct, contains zero to none logic
There was a problem hiding this comment.
For the record struct vs class aren't primarily about "contains logic or not". The big difference is that they change copy/reference semantics (structs are copy be value by default, classes are copy by reference by default), which can have performance implications (in both ways - structs can decrease performance if used incorrectly)
There was a problem hiding this comment.
Changed to a sealed class in 92546cd. You were right that the = default case was a real foot-gun, and it was reachable from our own code: TryCreate had to open with endpoint = default;, so every failure path handed back an instance whose non-nullable Uri was null. As a class the signature is [NotNullWhen(true)] out AgentlessEndpoint? endpoint and failures return null, so misuse is now a compiler warning instead of an NRE.
Also took the point about copy vs reference semantics being the real distinction — thanks.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9040) and master.
|
|||||||||||||||||||||||||||||||||||
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 190.35 ± (190.91 - 191.69) ms | 212.09 ± (212.06 - 212.91) ms | +11.4% | ❌⬆️ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 194.19 ± (194.09 - 194.56) ms | 218.74 ± (218.21 - 219.21) ms | +12.6% | ❌⬆️ |
| .NET Framework 4.8 - CallTarget+Inlining+NGEN | ||||
| duration | 1151.33 ± (1150.44 - 1155.97) ms | 1259.84 ± (1259.66 - 1265.99) ms | +9.4% | ❌⬆️ |
leoromanovsky
left a comment
leoromanovsky
left a comment
There was a problem hiding this comment.
functionality looks in-sync with other implementations 👍
| return ParsingResult<SourceSelection>.Failure(); | ||
| } | ||
|
|
||
| var normalized = value.Trim().ToLowerInvariant(); |
There was a problem hiding this comment.
As discussed before, don't do this 😅
There was a problem hiding this comment.
Fixed in 21d718e — comparisons are now string.Equals(..., StringComparison.OrdinalIgnoreCase), no ToLowerInvariant().
I also want to correct something I said earlier: I claimed the switch was doing ordinal comparison internally, which was wrong and probably made it look like case-insensitivity had been dropped. It hadn't, and OrdinalIgnoreCase now makes it explicit. Surrounding whitespace is still tolerated, but only on a second pass after the direct comparisons fail, so the common path doesn't allocate.
| /// Used as the converter for GetAsClass so the | ||
| /// config framework records both the raw string and the resolved value in telemetry. | ||
| /// </summary> | ||
| private static ParsingResult<SourceSelection> ConvertSource(string? value) |
There was a problem hiding this comment.
private static ParsingResult<FeatureFlagsSource> ConvertSource(string? value)
{
if (StringUtil.IsNullOrWhiteSpace(value))
{
return ParsingResult<FeatureFlagsSource>.Failure();
}
if (value.Equals(AgentlessSourceName, StringComparison.OrdinalIgnoreCase))
{
return ParsingResult<FeatureFlagsSource>.Success(FeatureFlagsSource.Agentless);
}
else if (value.Equals(RemoteConfigSourceName, StringComparison.OrdinalIgnoreCase))
{
return ParsingResult<FeatureFlagsSource>.Success(FeatureFlagsSource.RemoteConfig);
}
else if (value.Equals(OfflineSourceName, StringComparison.OrdinalIgnoreCase))
{
return ParsingResult<FeatureFlagsSource>.Success(FeatureFlagsSource.Disabled);
}
else
{
return ParsingResult<FeatureFlagsSource>.Failure();
}
}There was a problem hiding this comment.
Adopted, with one deliberate difference: an unrecognised value returns Success(FeatureFlagsSource.Disabled) rather than Failure(). Failure() falls back to the default, and the default is agentless, so DD_FEATURE_FLAGS_CONFIGURATION_SOURCE=agentles (typo) would silently start billed CDN polling. Returning Success(Disabled) keeps it failing closed. Blank values still return Failure() so the default applies. Happy to revisit if you'd rather have the fallback.
There was a problem hiding this comment.
The only problem is that this breaks reporting of the config values 🤔 It was a parsing failure, so IMO, customers should be able to see that in the service configuration. The fact that you fall back to agentless by default doesn't seem like a big problem to me, seeing as that's your default anyway? 🤔
There was a problem hiding this comment.
well, yes, but i feel kind of awkward to fallback in case of an error. but can change, sure
| #pragma warning restore 618 | ||
| } | ||
|
|
||
| Source = ResolveSource(enabled, configuredSource, legacyEnabled); |
There was a problem hiding this comment.
This is closer, but still isn't quite right I think 😅 Basically, ResolveSource shouldn't exist, otherwise we're reporting incorrect values 🤔
| .WithKeys(ConfigurationKeys.FeatureFlags.FeatureFlagsConfigurationSource) | ||
| .GetAsClass<SourceSelection>( | ||
| validator: null, | ||
| converter: ConvertSource); |
There was a problem hiding this comment.
Discussed below, but as mentioned before, you shouldn't use ResolveSource, as it doesn't record telemetry correctly.
I believe you should
- get rid of
SourceSelection - change
ConvertSourceas described below - Remove
ResolveSource - Use the following instead:
DefaultResult<FeatureFlagsSource> defaultFlag = enabled switch
{
false => new(FeatureFlagsSource.Disabled, nameof(FeatureFlagsSource.Disabled)),
null when legacyEnabled is not null => legacyEnabled.Value ? new(FeatureFlagsSource.RemoteConfig, nameof(FeatureFlagsSource.RemoteConfig)) : new(FeatureFlagsSource.Disabled, nameof(FeatureFlagsSource.Disabled)),
_ => new(FeatureFlagsSource.Agentless, nameof(FeatureFlagsSource.Agentless)),
};
Source = new ConfigurationBuilder(source, telemetry)
.WithKeys(ConfigurationKeys.FeatureFlags.FeatureFlagsConfigurationSource)
.GetAs(
defaultFlag,
validator: enabled == false ? _ => false : _ => true, // even valid source is forced to default when feature flagging is explicitly disabled
converter: ConvertSource);There was a problem hiding this comment.
Done in 21d718e: SourceSelection.cs is deleted, ResolveSource and FromDefaultSource are gone, and the precedence is now expressed as the DefaultResult<FeatureFlagsSource> switch you suggested, with the kill switch as a validator that rejects any configured value.
| public static FeatureFlagsSettings FromDefaultSource() | ||
| { | ||
| var source = GlobalConfigurationSource.Instance; | ||
| var config = new ConfigurationBuilder(source, TelemetryFactory.Config); | ||
| var site = config.WithKeys(ConfigurationKeys.Site).AsString(DefaultSite, s => !StringUtil.IsNullOrWhiteSpace(s)); | ||
| var env = config.WithKeys(ConfigurationKeys.Environment).AsString(); | ||
| var apiKey = config.WithKeys(ConfigurationKeys.ApiKey).AsRedactedString(); | ||
| return new FeatureFlagsSettings(source, TelemetryFactory.Config, site, env, apiKey); | ||
| } |
There was a problem hiding this comment.
This shouldn't exist, you should hang FeatureFlagSettings off TracerSettings, the same way we do with TelemetrySettings for example. Otherwise you haven't solved anything, this is still broken, and now we have global state access
There was a problem hiding this comment.
Done in 21d718e — FromDefaultSource() is gone and TracerSettings now owns FeatureFlags, constructed from the same source and telemetry. DD_ENV is passed in from Manager.InitialMutableSettings.Environment so it agrees with the env entry of DD_TAGS; per our Slack thread, DD_SITE/DD_API_KEY are still read locally the way TelemetrySettings and DirectLogSubmissionSettings do, and centralising those is left out of this PR.
…lformed sites safely
…nd validation telemetry
… sensitive in yaml
92546cd to
ada921d
Compare
andrewlock
left a comment
andrewlock
left a comment
There was a problem hiding this comment.
Thanks - there's unfortunately still an issue with env which annoyingly may mean we need to redesign this whole thing again 🤦♂️
| // Created after the manager so the environment can be taken from the initial mutable | ||
| // settings, which also honour the "env" entry of DD_TAGS. It is captured once, so a | ||
| // later dynamic-configuration change to "env" does not move the agentless endpoint. | ||
| FeatureFlags = new FeatureFlagsSettings(source, telemetry, Manager.InitialMutableSettings.Environment); |
There was a problem hiding this comment.
Urgh, just realised, through your use of InitialMutableSettings - this is another issue. Customers can change the env in code (And customers do that, we have telemetry that confirms it), and if they do, this is going to break, because they will send requests to the wrong endpoint.
In other words, your AgentlessEndpoint has to also become a mutable setting, that could change at any time. And you will likely need to re-architecture to account for that fact 🙁
In summary
FeatureFlags.AgentlessEndpointmoves toMutableSettings- You don't need to pass
envin anymore 😅 - Anything that wants to use
AgentlessEndpointneeds to subscribe to changes inMutableSettings, and perform whatever necessary caching/invalidation/rebuilding that entails..
It's a pain 😅
…o.khrebto/EX-2703/ffe-config-and-endpoint
3 participants
Summary of changes
Adds the foundational configuration layer for agentless Feature Flags delivery: new configuration keys, a
FeatureFlagsSettingsclass with source resolution and validation, and anAgentlessEndpointstruct for deriving the CDN URL from the Datadog site or a custom base URL.This is PR 1 of a stacked PR series implementing agentless Feature Flags configuration delivery (FFL-2703), porting functionality from dd-trace-py#19331 and dd-trace-java#11892.
Reason for change
The .NET tracer currently delivers Feature Flags configuration exclusively through the Datadog Agent's Remote Configuration. To support agentless (CDN-backed) delivery, we need a configuration layer that:
agentless(new default),remote_config, anddisabledsourcesDD_SITEor accepts a customDD_FEATURE_FLAGS_CONFIGURATION_SOURCE_AGENTLESS_BASE_URLDD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLEDusers onto Remote ConfigurationImplementation details
FeatureFlagsSourceenum —Disabled,Agentless,RemoteConfigFeatureFlagsSettings— reads all new env vars, resolves the source with the cross-SDK precedence contract (kill switch → explicit source → fail-closed → legacy grandfathering → default agentless), validates poll interval (1–3600s) and request timeout (>0s)AgentlessEndpoint— deriveshttps://ufc-server.ff-cdn.<site>/api/v2/feature-flagging/config/rules-based/serverfrom the site, appendsdd_envwhen configured, accepts custom HTTP/HTTPS URLs, and never echoes URLs in error messages (credentials safety)supported-configurations.yaml+ generatedConfigurationKeys.FeatureFlags.g.cs:DD_FEATURE_FLAGS_ENABLED(defaulttrue, supersedesDD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED)DD_FEATURE_FLAGS_CONFIGURATION_SOURCE(defaultagentless)DD_FEATURE_FLAGS_CONFIGURATION_SOURCE_AGENTLESS_BASE_URLDD_FEATURE_FLAGS_CONFIGURATION_SOURCE_AGENTLESS_POLL_INTERVAL_SECONDS(default 30, typeint)DD_FEATURE_FLAGS_CONFIGURATION_SOURCE_AGENTLESS_REQUEST_TIMEOUT_SECONDS(default 5, typeint)DD_EXPERIMENTAL_FLAGGING_PROVIDER_INITIALIZATION_TIMEOUT_MS(default 10000, implementation B)[Obsolete]onFlaggingProviderEnabledwith pragma suppression inFeatureFlagsSettingsThis PR does not wire anything to
FeatureFlagsModuleyet — that happens in a later PR in the stack.Test coverage
FeatureFlagsSettingsTests— 43 test cases covering source resolution (kill switch, explicit source, legacy grandfathering,offlinesentinel, invalid values), blank/whitespace handling, casing normalization, defaults, poll interval validation, request timeout validation, initialization timeout, and base URL readingAgentlessEndpointTests— 18 test cases covering managed endpoint derivation (site lowercasing, staging, govcloud),dd_envquery parameter (addition, null, escaping), custom endpoint path handling (origin-only gets canonical path, custom path used verbatim), HTTP acceptance for custom endpoints, invalid URL rejection, empty/whitespace site rejection, and credentials-in-error safetyOther details
Stacked PRs:
System tests: DataDog/system-tests#7496