Repository navigation
[FFE] Feature Flags SDK - #7896
Merged
Merged
Conversation
BenchmarksBenchmark execution time: 2026-01-12 15:14:59 Comparing candidate commit 688c62c in PR branch Found 9 performance improvements and 9 performance regressions! Performance is the same for 158 metrics, 10 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery netcoreapp3.1
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch net6.0
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net6.0
scenario:Benchmarks.Trace.RedisBenchmark.SendReceive net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes netcoreapp3.1
scenario:Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin net472
|
dromanol
force-pushed
the
dani/apm/feature_flags
branch
from
December 15, 2025 13:03
cba84d3 to
bda4ee9
Compare
This comment has been minimized.
This comment has been minimized.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7896) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (69ms) : 67, 70
master - mean (69ms) : 67, 70
section Bailout
This PR (7896) - mean (72ms) : 71, 73
master - mean (73ms) : 72, 74
section CallTarget+Inlining+NGEN
This PR (7896) - mean (1,011ms) : 961, 1061
master - mean (1,017ms) : 961, 1073
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (106ms) : 103, 109
master - mean (106ms) : 104, 108
section Bailout
This PR (7896) - mean (107ms) : 106, 108
master - mean (107ms) : 106, 108
section CallTarget+Inlining+NGEN
This PR (7896) - mean (742ms) : 682, 802
master - mean (742ms) : 686, 798
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (94ms) : 91, 96
master - mean (94ms) : 92, 96
section Bailout
This PR (7896) - mean (94ms) : 93, 95
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7896) - mean (707ms) : 665, 749
master - mean (715ms) : 690, 740
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (92ms) : 89, 95
master - mean (92ms) : 90, 94
section Bailout
This PR (7896) - mean (93ms) : 92, 94
master - mean (93ms) : 92, 94
section CallTarget+Inlining+NGEN
This PR (7896) - mean (633ms) : 618, 648
master - mean (634ms) : 620, 647
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (193ms) : 186, 200
master - mean (192ms) : 188, 196
section Bailout
This PR (7896) - mean (195ms) : 193, 197
master - mean (195ms) : 193, 197
section CallTarget+Inlining+NGEN
This PR (7896) - mean (1,122ms) : 1066, 1179
master - mean (1,114ms) : 1062, 1165
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (276ms) : 271, 280
master - mean (275ms) : 270, 279
section Bailout
This PR (7896) - mean (277ms) : 272, 281
master - mean (275ms) : 271, 279
section CallTarget+Inlining+NGEN
This PR (7896) - mean (922ms) : 861, 984
master - mean (923ms) : 869, 976
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (269ms) : 264, 274
master - mean (269ms) : 264, 274
section Bailout
This PR (7896) - mean (268ms) : 265, 271
master - mean (267ms) : 263, 271
section CallTarget+Inlining+NGEN
This PR (7896) - mean (920ms) : 874, 966
master - mean (926ms) : 885, 966
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (268ms) : 263, 272
master - mean (268ms) : 261, 274
section Bailout
This PR (7896) - mean (267ms) : 263, 271
master - mean (267ms) : 264, 270
section CallTarget+Inlining+NGEN
This PR (7896) - mean (822ms) : 804, 841
master - mean (819ms) : 801, 836
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dd-oleksii
reviewed
Dec 16, 2025
dd-oleksii
left a comment
dd-oleksii
left a comment
There was a problem hiding this comment.
Did a review pass, mostly focusing on evaluator. This is probably the cleanest evaluation outline that I've seen 👍
Main things to handle before release:
- We should error out on invalid configuration with
PARSE_ERRORinstead of trying to continue and serving potentially unexpected values. - Various conversion inconsistencies (see inline comments).
- Need to check that user-expected type matches
variationType. I'm not sure that usingTypeis a good approach here and all other SDKs use explicit enums. (Example whereTypecan fall short:stringcould be either a string or a JSON flag, and JSON flags could basically be anything.) - It looks like a lot of types and methods are public, which makes it hard to evolve SDK without breaking backward compatibility. We generally prefer exposing little to no surface except OpenFeature provider interface and configuration options.
I have started collecting some of the edge case requirements over here, so we're aligned between SDKs. If you have any questions or notice any requirements missing, just drop me a note and I'll add it 🙏
dromanol
marked this pull request as ready for review
December 17, 2025 13:32
dromanol
marked this pull request as draft
December 17, 2025 13:40
dromanol
marked this pull request as ready for review
December 17, 2025 14:06
e-n-0
reviewed
Dec 18, 2025
e-n-0
approved these changes
Dec 18, 2025
Contributor
Author
|
@codex review |
![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.
ℹ️ 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".
andrewlock
requested changes
Dec 18, 2025
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Mostly just nits, but there's a few important aspects:
- As discussed, we should probably look at a different approach to supporting OpenFeature, instead of a NuGet package. To get this PR merged ASAP I would suggest
- Remove the new NuGet package, and just copy the DatadogProvider into the test project for now
- In a subsequent PR, explore the source generator approach
- We should add public API telemetry to the public APIs
- Do we need all of the public APIs that we've added? If the provider is the expect approach, can/should we just provide that. Because we can never remove these public APIs once they're added, but we can always add new ones later. I would prefer we kept them internal until we're sure they're the correct shape
- There are some problems around initialization and setting handling. We need to handle changes in setting values, and ensure we aren't calling
TracerSettings.FromDefaultSourcesor anything
dromanol
force-pushed
the
dani/apm/feature_flags
branch
2 times, most recently
from
December 22, 2025 18:18
70878df to
4bb7279
Compare
andrewlock
reviewed
Dec 23, 2025
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
This is looking good! A bunch of nits really, but main feedback:
- We should avoid using
!in any new code, there's almost always ways around it, which we should use - Tweaked the config code
- We should think about the public API a little in terms of what we instrument for future proofing. I've added one suggestion
- Create a throw helper for
FormatException - Use concrete types where possible
And then for later:
- Public API telemetry
- Nuget/Source gen discussion for DatadogProvider
I haven't approved yet, just so I can keep track a bit better, but there's nothing specifically blocking now I think, just improvements we can make 😄
dromanol
force-pushed
the
dani/apm/feature_flags
branch
from
December 24, 2025 11:01
231eaa5 to
9f485c7
Compare
It is actually easier to evolve this over time, but by instrumenting the exploded args we can maintain compatibility without extra duck typing
dromanol
force-pushed
the
dani/apm/feature_flags
branch
from
January 12, 2026 14:30
04cb92c to
688c62c
Compare
andrewlock
added a commit
that referenced
this pull request
Jan 15, 2026
## Summary of changes Use the new `PostAsJsonAsync` API to avoid serializing to string and byte array locally ## Reason for change We recently added support for `PostAsJsonAsync` in #8017, and this is a good example where we can easily swap it in in the new Exposures API used by FFE: #7896 ## Implementation details Just updated to use the new APIs. A couple of points to notice: - This doesn't enable gzip. If the endpoint supports it, we should probably enable that, but would need testing - This changes the serialization settings, by using `SerializationHelpers.DefaultJsonSettings`, which omits serializing null members, and uses snake-case serialization, instead of the PascalCase we have currently - However, [looking at the spec](https://github.com/DataDog/dd-source/blob/main/domains/evp-workers/apps/exposures-worker/schemas/batched_exposures.json), I'm pretty sure we're not compliant today, and what we're changing to is correct - Which suggests we're missing tests... ## Test coverage This _should_ be covered by existing tests... and if it's not, then we need to add those tests 😄
pavlokhrebto
added a commit
that referenced
this pull request
Sep 17, 2026
…9260) ## Summary of changes Removes `Datadog.Trace.FeatureFlags.FeatureFlagsSdk`, the manual non-OpenFeature Feature Flags API, from `Datadog.Trace.Manual`, along with its three CallTargets and its test sample. OpenFeature (`Datadog.FeatureFlags.OpenFeature`) becomes the only way to evaluate flags in .NET. ## Reason for change .NET was the only tracer with a second, Datadog-specific evaluation surface — Java, Go and Node ship OpenFeature only. If OpenFeature is the intended supported approach, the duplicate is better removed now, while Feature Flags is experimental and gated behind `DD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED`, than after customers adopt it. The surface shipped in v3.36.0 (#7896). It is undocumented and excluded from the public API snapshot: `PublicApiTestsBase` skips types carrying both `[Browsable(false)]` and `[EditorBrowsable(Never)]`, so `FeatureFlagsSdk` appears nowhere in `PublicApiTests.Datadog.Trace.Manual.PublicApiHasNotChanged.verified.txt`. It also makes sense to remove it now while no customers use it. ## Implementation details Deleted: - `Datadog.Trace.Manual/FeatureFlags/` — `FeatureFlagsSdk`, `EvaluationContext`, `IEvaluation.Manual` - `ManualInstrumentation/FeatureFlags/` — the `Evaluate`, `IsAvailable` and `RegisterOnNewConfigEventHandler` CallTargets - the three `<Compile Include>` links in `Datadog.Trace.Manual.csproj` (`EvaluationReason`, `ValueType`, `IEvaluation`). `Datadog.FeatureFlags.OpenFeature.csproj` links its own copies straight from `Datadog.Trace` and is unaffected - the `Samples.FeatureFlags` project, with its `Datadog.Trace.sln` and CODEOWNERS entries `Program.cs` was shared by both samples through a `<Compile Include>` link, so it moves into `Samples.OpenFeature` and the link is dropped. Generated files regenerated with `CompileManagedSrc` and `RegenerateSolutions`. The diffs are pure deletions of the three rows, with no signature renumbering. ## Test coverage `FeatureFlagsTests`, `OpenFeatureFeatureFlagsTests` and `FeatureFlagsTestsBase` collapse into a single `FeatureFlagsTests` on the OpenFeature sample. Coverage of the manual surface goes away with the surface itself. Two sample-side improvements come with the move: - `Samples.OpenFeature`'s `Evaluator.Init()` awaits `SetProviderAsync` instead of calling `.Wait()` - a new `<INITIALIZED: READY>` probe and assertion — the only coverage of the OpenFeature `InitializeAsync` CallTarget merged in #9044 ## Other details Breaking change for anyone calling `Datadog.Trace.FeatureFlags.FeatureFlagsSdk` directly.
This was referenced Sep 23, 2026
pavlokhrebto
added a commit
that referenced
this pull request
Sep 24, 2026
…ror (#9299) ## Summary of changes - `FeatureFlagsModule.Evaluate` returns the caller's default instead of `null` when no configuration has arrived. - `FeatureFlagsSdk.GetResolutionDetails` returns the caller's default for every evaluation that carries an error, and when the tracer is not attached. - `Datadog.FeatureFlags.OpenFeature` 2.3.1 → 2.3.2. ## Reason for change Before configuration arrives, `GetStringValueAsync("flag", "fallback")` returned `null` instead of `"fallback"`, with `PROVIDER_NOT_READY`. That is the startup window every application passes through. OpenFeature substitutes the caller's default only when a provider throws. This provider reports errors as resolution details instead, so the default must travel with the details. Three layers could have supplied it, and none did: 1. **Tracer** — the module's not-ready shortcut returned `null`. It came in with the original SDK in #7896, where the error string was `No config loaded`. #8367 changed the string to `PROVIDER_NOT_READY` and kept the `null`. The evaluator's own not-ready path already returned `defaultValue`, so the two paths disagreed. 2. **Package** — `GetResolutionDetails` never received the default. It copied whatever value the tracer returned, and returned `default!` when the tracer is not attached. For a string flag that is `null`. It came in with the package in #8077. 3. **OpenFeature client** — it does not step in, because the provider does not throw. The same gap also lost JSON defaults. On error the tracer hands back the caller's OpenFeature `Value`, which `JsonToValue` cannot convert, so the caller got an empty `Value`. ## How we found it system-tests [#7496](DataDog/system-tests#7496) enabled the Feature Flags configuration-source tests for .NET for the first time. On the first run, the five cold-start cases failed with `{'errorCode': 'PROVIDER_NOT_READY', 'value': None}` where they expect the default. Those tests had been `missing_feature` for .NET, and the unit tests on this path asserted only the error, never the value, so nothing had covered it. ## Implementation details The package is the layer that owns the rule: any evaluation with an error type resolves to the caller's default. The tracer fix is still needed on its own, because it ships with the tracer, while the package reaches customers only when they upgrade it. `Resolve<T>` now takes `T defaultValue` instead of `object?`. No CallTarget instruments `Resolve`, so changing the signature is safe. ## Test coverage `Evaluate_WhenNoConfigurationYet_ReturnsTheDefaultValue` covers string, boolean, integer and numeric flags, and asserts the value. The package has no unit-test project. The system-tests cold-start cases cover the not-ready path end to end. ## Other details Jira: [FFL-3325](https://datadoghq.atlassian.net/browse/FFL-3325) Delivery: the tracer fix ships in the next tracer release. The package fix needs `Datadog.FeatureFlags.OpenFeature` 2.3.2. [FFL-3325]: https://datadoghq.atlassian.net/browse/FFL-3325?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
pavlokhrebto
added a commit
that referenced
this pull request
Sep 24, 2026
…iguration file (#9304) ## Summary of changes `FfeProduct.UpdateFromRcm` replaces the stored version of a flags file when a modified version arrives, instead of appending it. ## Reason for change Fixes #9301. When a flag was disabled or removed for an environment, a running process kept evaluating it with its last value until restart. Rule and variant changes propagated; only removals were lost. A modified file arrives under the path it already has, and never in the removed set, so `FfeProduct` kept both the old and the new version. The module merges all stored versions, and a merge only adds or overwrites keys, so a flag missing from the new version was still served from the old one. The stored list also grew by one full copy of the flags file on every update, for the life of the process. Only the Remote Configuration source is affected. The agentless source replaces the whole configuration on every update. ### Origin - [#7896](#7896) made `FfeProduct` append a modified file instead of replacing it. Removals still worked, because the module used only the last entry, so the duplicates were only a memory leak. - [#8074](#8074) changed the module to merge all entries. This is where removals broke. - [#8616](#8616) rewrote `FlagCollection` with the same add-or-overwrite merge, so the behaviour did not change. ## Implementation details Remove any entry stored under the incoming path before adding the new one, as suggested in the issue. Go does the equivalent: each update replaces the provider's configuration. ## Test coverage - `UpdateRemoteConfig_WhenAModifiedFileDropsAFlag_TheFlagIsNoLongerFound`: v1 of a file holds two flags, v2 under the same path holds one. The dropped flag returns `FLAG_NOT_FOUND`, and the kept flag still resolves. - `UpdateFromRcm_WhenTheSameFileIsModifiedRepeatedly_HoldsOneVersionOfIt`: three updates to one path leave exactly one stored version. ## Other details Jira: [FFL-3338](https://datadoghq.atlassian.net/browse/FFL-3338) The issue also reports update events arriving as `ProviderReady` with a null provider name. That was already fixed by #9044, which emits `ProviderConfigurationChanged` with the provider name, and shipped in `Datadog.FeatureFlags.OpenFeature` 2.3.1. [FFL-3338]: https://datadoghq.atlassian.net/browse/FFL-3338?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
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
6 participants
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
Add support for Feature Flag configuration frameworks
Reason for change
Spec document can be found here
Implementation details
Retrieved FF config from backend through Remote Config. Enable SDK functions.
Created
Tracer.FeatureFlags.OpenFeature.nupkgto add support for OpenFeature from V2.0.0 onwardsTest coverage
Added unit and integration tests
Other details