feat(FeatureFlags): FFE APM feature-flag span enrichment - #8795
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8795) 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 (8795) - mean (71ms) : 69, 74
master - mean (71ms) : 69, 74
section Bailout
This PR (8795) - mean (79ms) : 74, 83
master - mean (78ms) : 75, 82
section CallTarget+Inlining+NGEN
This PR (8795) - mean (1,089ms) : 1054, 1125
master - mean (1,092ms) : 1035, 1149
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 (8795) - mean (113ms) : 106, 119
master - mean (113ms) : 107, 119
section Bailout
This PR (8795) - mean (114ms) : 109, 119
master - mean (113ms) : 108, 118
section CallTarget+Inlining+NGEN
This PR (8795) - mean (781ms) : 761, 801
master - mean (784ms) : 766, 802
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8795) - mean (100ms) : 96, 105
master - mean (100ms) : 94, 106
section Bailout
This PR (8795) - mean (99ms) : 97, 101
master - mean (102ms) : 96, 107
section CallTarget+Inlining+NGEN
This PR (8795) - mean (944ms) : 911, 978
master - mean (944ms) : 908, 980
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8795) - mean (96ms) : 92, 100
master - mean (95ms) : 93, 97
section Bailout
This PR (8795) - mean (100ms) : 96, 105
master - mean (97ms) : 95, 98
section CallTarget+Inlining+NGEN
This PR (8795) - mean (822ms) : 782, 862
master - mean (818ms) : 785, 850
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 (8795) - mean (195ms) : 191, 199
master - mean (191ms) : 188, 194
section Bailout
This PR (8795) - mean (198ms) : 196, 201
master - mean (195ms) : 193, 197
section CallTarget+Inlining+NGEN
This PR (8795) - mean (1,189ms) : 1138, 1240
master - mean (1,173ms) : 1121, 1224
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 (8795) - mean (278ms) : 272, 284
master - mean (274ms) : 271, 277
section Bailout
This PR (8795) - mean (279ms) : 274, 283
master - mean (274ms) : 271, 277
section CallTarget+Inlining+NGEN
This PR (8795) - mean (949ms) : 935, 964
master - mean (942ms) : 926, 958
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8795) - mean (272ms) : 266, 278
master - mean (267ms) : 264, 271
section Bailout
This PR (8795) - mean (272ms) : 267, 276
master - mean (267ms) : 265, 270
section CallTarget+Inlining+NGEN
This PR (8795) - mean (1,157ms) : 1121, 1192
master - mean (1,140ms) : 1083, 1196
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8795) - mean (270ms) : 264, 276
master - mean (266ms) : 261, 271
section Bailout
This PR (8795) - mean (271ms) : 267, 274
master - mean (266ms) : 263, 269
section CallTarget+Inlining+NGEN
This PR (8795) - mean (1,035ms) : 991, 1078
master - mean (1,028ms) : 986, 1070
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-07-29 09:55:05 Comparing candidate commit d074d7c in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 59 known flaky benchmarks, 67 flaky benchmarks without significant changes.
|
leoromanovsky
added
type:new-feature
area:tracer
leoromanovsky
marked this pull request as ready for review
June 22, 2026 14:18
leoromanovsky
requested review from
dd-oleksii,
typotter and
vandonr
and removed request for
a team
June 22, 2026 14:18
leoromanovsky
changed the title
![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: b294dd157d
ℹ️ 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
reviewed
Jun 22, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
I haven't finished, but figured there's a lot to be going on with, so would give some initial feedback!
The bulk of it is:
- Do we need this to be a public API, if not, it's preferable to not
- Not a fan of the AI comments, they add a lot of noise but not much value IMO😅
- I would really rather avoid the global static accesses, these are a nightmare for testing, and a source of ongoing flakiness in our tests
dromanol
reviewed
Jul 2, 2026
andrewlock
previously requested changes
Jul 2, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
I started looking through the detailed changes, and they look better thanks 👍
However, reading again, I have some fundamental concerns about the overall design. This seems to be adding a "central" singleton store which is then hit whenever a trace is closed. That requires a bunch of synchronization and other work to make it thread safe, but it also forces you to do a whole bunch of work in the hot path.
What's more, I'm not sure that this actually flows async context correctly? Won't you end up with random spans being tagged with feature flags from completely different traces (which were running concurrently)?
Can/should we take a different design, where you have a per-trace-context span enrichment object. That way you get proper async-context flow, eliminate most of the lock contention, and can delay serialization yo background threads?
| // Serial id of the experiment split, used for APM span enrichment. | ||
| // Nullable: absent in UFC shapes that predate span enrichment. Deserialized | ||
| // from the UFC "serialId" field (Newtonsoft case-insensitive matching). | ||
| [JsonProperty("serialId")] |
Member
There was a problem hiding this comment.
Is this property really necessary? 🤔 It seems like all the other properties would need it if so. Case-insensitive matching seems to suggest that this property is not required?
Contributor
|
Hey @andrewlock ! Thanks a lot for the detailed review — really helpful. Went through all of it, here's where I landed. Most of your feedback centers on the central The one part I think we'd rather not touch is the serializer ( Rundown of your comments and what I'm proposing:
A few questions for you:
We'll get the state-on-trace redesign + fixes onto a stacked branch. Two clarifications on the state-on-trace plan so I'm not overselling it:
Thanks again 🙏 |
typotter
approved these changes
Jul 6, 2026
| catch (Exception ex) | ||
| { | ||
| // Enrichment must never break flag evaluation. | ||
| System.Diagnostics.Debug.WriteLine($"[Datadog] SpanEnrichmentHook.FinallyAsync failed: {ex}"); |
There was a problem hiding this comment.
This log will only be visible in dev/staging.
DatadogLogging.GetLoggerFor + Log.Debug would surface this error in prod logging aiui
Contributor
There was a problem hiding this comment.
Good instinct, but DatadogLogging isn't reachable from here: Datadog.FeatureFlags.OpenFeature is a standalone shim with no reference to Datadog.Trace — it only source-links a few contract files (IEvaluation, FeatureFlagMetadataKeys, etc.), and DatadogLogging is internal. Wiring it up would mean taking a dependency the package intentionally avoids.
The accumulation that matters actually runs in the tracer's CallTarget integration and SpanEnrichmentState.Accumulate, both of which already log at Debug inside Datadog.Trace. This catch only guards local arg-building in the shim, so Debug.WriteLine is really just a dev aid — happy to drop it entirely if you'd prefer nothing here.
Member
There was a problem hiding this comment.
The https://www.nuget.org/packages/OpenFeature package depends on the .NET logging packages, which suggests that the "correct" place to log this is using the .NET ILogger abstraction. That said, given we don't use it anywhere here currently, and plumbing it through is prob a bit of a pain, my preference is to just remove the System.Diagnostics.Debug.WriteLine anyway. It literally isn't in the compiled code we ship, so it mostly ends up just being noise
| // lock and performs encoding/JSON serialization after releasing it. | ||
| private readonly object _gate = new(); | ||
|
|
||
| private readonly HashSet<long> _serialIds = new(); |
There was a problem hiding this comment.
Would using a SortedSet here and moving the sorting logic to AddSerialId instead of sorting the whole list in ToSpanTag be a useful shift of work? Wanting to optimize for the flush pushes the sort work to AddSerialId which is probably called a lot more often? Seems cleaner to pass the sorted set around instead of hash set to array/collection back to sorted set.
not blocking, but something to consider.
Contributor
There was a problem hiding this comment.
Considered it, but I think HashSet + sort-at-flush is the better fit here:
- The
ULEB128encoder is the frozen cross-SDK codec and already dedupes + sorts internally, so a pre-sorted set wouldn't remove the flush-time sort unless we changed the encoder (which has to stay byte-identical). AddSerialIdis the hot accumulate path —HashSetkeeps inserts O(1) and we sort once at flush.SortedSetwould push O(log n) onto every insert, i.e. onto the path that's called most often.
So I'd keep HashSet.
Thanks though — good to sanity-check the work placement.
pavlokhrebto
added a commit
that referenced
this pull request
Jul 7, 2026
…d of a central store (#8869) ## Summary of changes Stacked on top of #8795. Replaces the central `SpanEnrichmentStore` with per-trace FFE span-enrichment state stored on `TraceContext`, and folds in the review nits from that PR. The `ffe_*` tags are still written at `Span.Finish()`, now reading the trace's own state. Wire output is unchanged (byte-identical). ## Reason for change The review on #8795 flagged that a process-wide `ConcurrentDictionary<rootSpanId, state>` is suboptimal: it forces synchronization and hot-path lookups for every root span, needs a cap/eviction/warn to avoid leaks, and — because it keys by span id — risks tagging spans with feature flags from a different concurrent trace. Storing the state on the trace itself removes all of that. ## Implementation details - **State on `TraceContext`** — a lazily-created `SpanEnrichmentState?`, exposed via `FeatureFlagEnrichment` / `GetOrCreateFeatureFlagEnrichment()`. It lives and dies with the trace, so there is no central map, no `SpanId` key, no cap, and no eviction. - **Accumulate path** — the OpenFeature and native evaluate integrations write directly to the active trace's state, gated on `Settings.IsSpanEnrichmentEnabled`. Because an evaluation can only ever touch its own trace's state, cross-trace contamination is structurally impossible. - **Write path** — `Span.Finish()` reads `Context.TraceContext?.FeatureFlagEnrichment` for the local root span; no store lookup, no gate. Errors are logged at `Debug` to avoid per-span log flooding. - **Deletions** — `SpanEnrichmentStore`, its `TracerManager` wiring, the `ClearSpanEnrichment` seam/integration, and the provider-close bridge are all removed. `IsSpanEnrichmentEnabled` reads the setting directly. - **Concurrency** — `SpanEnrichmentState` keeps its per-instance lock (concurrent evals under one trace, e.g. `Task.WhenAll`); there is no shared-map contention. - **Review nits** — tightened member visibility, pre-sized `AsDictionary()` in the OpenFeature structure conversion, documented `Split.SerialId` as optional for config skew, and removed cross-SDK comment references. The frozen wire contract (tag names, ULEB128 encoding, SHA256 hashing, per-eval limits, golden vector) is unchanged. ## Test coverage - Existing codec golden-vector / round-trip and accumulator cap/dedupe tests, retargeted off the deleted store onto `SpanEnrichmentState`. - New: lazy state creation with a stable instance; two separate traces never cross-contaminate; `ffe_*` tags land on the local root only, never on child spans; gate-off allocates no state and emits nothing. ## Other details The native `generated_calltargets.g.cpp` entry for the removed `ClearSpanEnrichment` call target was removed by hand (the Nuke generator can't run on macOS); CI regeneration on Windows/Linux should confirm it's byte-identical. The store-era `SpanFinish_EnrichmentThrows` test was dropped because it depended on a test-only fault hook on the store; the never-throw guard remains in `Span.Finish()`.
Contributor
|
@andrewlock comments addressed in #8869 and merged in this branch |
Contributor
|
@andrewlock — ready for another look. The redesign from your review is in: enrichment state now lives on
No cap and no serializer deferral, which also matches what the other SDKs (JS/Python/Ruby) do. I've resolved the threads that are done and left this so the mapping is in one place |
pavlokhrebto
dismissed
andrewlock’s stale review
July 7, 2026 12:24
Dismissing to unblock the PR - @andrewlock is PTO for the next 2 weeks and his comments were addressed.
Will ask someone else to check
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in, experimental Feature Flags (FFE) APM span-enrichment pipeline for OpenFeature evaluations in dd-trace-dotnet, emitting the cross-SDK ffe_* tag wire format and integrating enrichment into the root-span finish path.
Changes:
- Introduces span-enrichment state/codec (
SpanEnrichmentState,ULeb128Encoder, SHA256 subject hashing) and writesffe_*tags on rootSpan.Finish. - Extends feature-flag evaluation metadata (split serial id + do_log) and adds OpenFeature hook + manual-instrumentation calltargets to accumulate enrichment.
- Adds unit + integration tests and updates generated calltarget/config outputs for the new env var gate (
DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED).
Reviewed changes
Copilot reviewed 24 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/test/test-applications/integrations/Samples.FeatureFlags/Program.cs | Adds an enrich mode to wrap evaluations in a root+child span for integration testing. |
| tracer/test/Datadog.Trace.Tests/FeatureFlags/SpanEnrichmentTests.cs | New unit tests for codec, accumulation limits, concurrency, and Span.Finish “never throw” behavior. |
| tracer/test/Datadog.Trace.Tests/FeatureFlags/FeatureFlagsEvaluatorTests.cs | Adds Split JSON deserialization tests for optional serialId. |
| tracer/test/Datadog.Trace.TestHelpers/FeatureFlagsHelpers.cs | Adds deterministic serial ids to test flag models for span-enrichment assertions. |
| tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/FeatureFlags/SpanEnrichmentIntegrationTests.cs | New integration tests validating ffe_* tags land on root only and are gated off by default. |
| tracer/src/Datadog.Tracer.Native/Generated/generated_calltargets.g.cpp | Updates native calltarget definitions (generated) for new OpenFeature methods/integrations. |
| tracer/src/Datadog.Trace/Util/Sha256Helper.cs | Adds SHA256 helper for subject-key hashing used by span enrichment. |
| tracer/src/Datadog.Trace/TraceContext.cs | Adds per-trace lazy SpanEnrichmentState storage and accessor. |
| tracer/src/Datadog.Trace/Span.cs | Writes ffe_* tags during root-span Finish, guarded and never-throw. |
| tracer/src/Datadog.Trace/Generated/netstandard2.0/Datadog.Trace.SourceGenerators/InstrumentationDefinitionsGenerator/InstrumentationDefinitions.g.cs | Registers new OpenFeature manual instrumentation integrations (generated). |
| tracer/src/Datadog.Trace/Generated/netstandard2.0/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.FeatureFlags.g.cs | Adds generated config key constant for span-enrichment gate. |
| tracer/src/Datadog.Trace/Generated/netcoreapp3.1/Datadog.Trace.SourceGenerators/InstrumentationDefinitionsGenerator/InstrumentationDefinitions.g.cs | Registers new OpenFeature manual instrumentation integrations (generated). |
| tracer/src/Datadog.Trace/Generated/netcoreapp3.1/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.FeatureFlags.g.cs | Adds generated config key constant for span-enrichment gate. |
| tracer/src/Datadog.Trace/Generated/net6.0/Datadog.Trace.SourceGenerators/InstrumentationDefinitionsGenerator/InstrumentationDefinitions.g.cs | Registers new OpenFeature manual instrumentation integrations (generated). |
| tracer/src/Datadog.Trace/Generated/net6.0/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.FeatureFlags.g.cs | Adds generated config key constant for span-enrichment gate. |
| tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/InstrumentationDefinitionsGenerator/InstrumentationDefinitions.g.cs | Registers new OpenFeature manual instrumentation integrations (generated). |
| tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.FeatureFlags.g.cs | Adds generated config key constant for span-enrichment gate. |
| tracer/src/Datadog.Trace/FeatureFlags/ULeb128Encoder.cs | Implements base64 ULEB128 delta-varint encoding used by ffe_flags_enc / subjects values. |
| tracer/src/Datadog.Trace/FeatureFlags/SpanEnrichmentState.cs | Implements bounded per-trace enrichment accumulation + snapshot-to-tags logic. |
| tracer/src/Datadog.Trace/FeatureFlags/Rcm/Model/Split.cs | Adds optional SerialId to UFC Split model for enrichment metadata. |
| tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsEvaluator.cs | Surfaces split serial id + do_log into evaluation metadata for downstream enrichment. |
| tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagMetadataKeys.cs | Defines shared metadata keys for split serial id + do_log. |
| tracer/src/Datadog.Trace/Configuration/TracerSettings.cs | Adds IsSpanEnrichmentEnabled setting from env/config. |
| tracer/src/Datadog.Trace/Configuration/supported-configurations.yaml | Documents new env var gate for span enrichment. |
| tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/ManualInstrumentation/OpenFeature/OpenFeatureSdkIsSpanEnrichmentEnabledIntegration.cs | Instruments OpenFeature SDK to read the tracer’s gate setting. |
| tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/ManualInstrumentation/OpenFeature/OpenFeatureSdkAccumulateSpanEnrichmentIntegration.cs | Instruments OpenFeature SDK to accumulate enrichment on the active trace context. |
| tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/ManualInstrumentation/FeatureFlags/FeatureFlagsSdkEvaluateIntegration.cs | Accumulates enrichment for native FeatureFlags SDK evaluations into the trace context. |
| tracer/src/Datadog.Trace.Trimming/build/Datadog.Trace.Trimming.xml | Adds trimming hint for SortedSet<T>.Enumerator. |
| tracer/src/Datadog.FeatureFlags.OpenFeature/SpanEnrichmentHook.cs | Adds OpenFeature FinallyAsync hook to feed evaluation metadata into enrichment. |
| tracer/src/Datadog.FeatureFlags.OpenFeature/Sdk/FeatureFlagsSdk.cs | Adds new SDK surface for enrichment accumulation + gate check (instrumented). |
| tracer/src/Datadog.FeatureFlags.OpenFeature/DatadogProvider.cs | Conditionally registers the new hook only when the gate is enabled. |
| tracer/src/Datadog.FeatureFlags.OpenFeature/Datadog.FeatureFlags.OpenFeature.csproj | Links shared metadata keys into the OpenFeature package build. |
| tracer/build/supported_calltargets.g.json | Updates supported calltargets manifest (generated). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+59
to
+62
| // The sample wraps flag evaluation in "ffe.root" and a child "ffe.child" (including an eval | ||
| // after an await). Wait for both spans. | ||
| var spans = await agent.WaitForSpansAsync(2, operationName: "ffe.root", returnAllOperations: true); | ||
|
|
Contributor
There was a problem hiding this comment.
Correct — WaitForSpansAsync's loop only counts spans whose name matches operationName, so count: 2 + operationName: "ffe.root" never breaks early (the sample emits one ffe.root + one ffe.child) and burned the full timeout. Fixed by dropping the operationName filter so it waits for 2 total spans.
Comment on lines
+116
to
+119
| Assert.Contains("Exit. OK", output); | ||
|
|
||
| var spans = await agent.WaitForSpansAsync(2, operationName: "ffe.root", returnAllOperations: true); | ||
|
|
Contributor
There was a problem hiding this comment.
Same fix applied here — removed the operationName: "ffe.root" filter so the negative-control test waits for both spans (root + child) instead of two root spans.
- Add experimental FFE APM span enrichment for OpenFeature evaluations in `dd-trace-dotnet`. - Emit the `ffe_*` wire format used by `dd-trace-js#8343` so backend/Trino decode uses the same tags and encodings. - Keep enrichment disabled by default behind `DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED`. - Adds `DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED` in `TracerSettings` and `supported-configurations.yaml`. - Adds split serial ID support to the UFC `Split` model. - Adds `ULeb128Encoder` for delta-varint feature-flag serial ID encoding. - Adds per-root-span `SpanEnrichmentState` and `SpanEnrichmentStore`. - Adds `SpanEnrichmentHook` for the OpenFeature `Finally` hook path. - Adds manual-instrumentation integrations for the OpenFeature hook path. - Writes `ffe_flags_enc`, `ffe_subjects_enc`, and `ffe_runtime_defaults` on `Span.Finish`. - Adds synchronization and snapshotting around `SpanEnrichmentState` mutation and `ToSpanTags`. - Adds a never-throw guard around the `Span.Finish` enrichment block. - Bounds `SpanEnrichmentStore` growth for root spans that never finish. - Unwraps `OpenFeature.Model.Value` runtime defaults into plain CLR values before JSON serialization. - Adds xUnit coverage for required span-enrichment cases, max-200 serial IDs, concurrency, never-throw behavior, and store growth bounds. - Updates generated source-generator outputs. - The feature is opt-in and off by default. - No store or accumulator is created when `DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED=false`. - No `ffe_*` tags are emitted when the gate is off. - State is keyed by local root span and cleaned up on root-span finish and provider close. - Store growth is bounded for root spans that never finish. - `ffe_*` tags are bare span `meta` tags, not `_dd.`-prefixed tags. - Subject keys are SHA256 hashes and are emitted only when `doLog=true`. - Runtime default values are JSON-serialized as the caller value, not as the OpenFeature wrapper object. - Frozen `system-tests` parametric suite: ```bash TEST_LIBRARY=dotnet ./run.sh PARAMETRIC -k span_enrichment Library: dotnet@3.46.0 ===================== 18 passed, 2605 deselected in 46.26s ===================== ``` - FFE dogfooding local trace-intake run: - Provider path: `DatadogProvider` / OpenFeature client. - Root span: `aspnet_core.request`. - Service: `ffe-dogfooding-dotnet`. - Gate on aggregation: `ffe_flags_enc` decoded to `[829, 1442, 2311, 2312]`. - Gate on subjects: `ffe_subjects_enc` contained 2 SHA256 subject keys. - Gate off: 3 spans flushed; 0 `ffe_*` tags. - Runtime defaults: unicode string and object defaults serialized as raw UTF-8; long unicode value truncated to 64 codepoints. - Codec parity: `ZAgUAg==` decoded to `[100, 108, 128, 130]`. - Staging Datadog APM proof: - Runtime: dogfooding staging APM proof against locally built SDK sha `b294dd1` and the staging Agent with `DD_SITE=datad0g.com`. - Validation id: `ffe-span-dotnet-20260619121831-b294dd1`. - Service: `ffe-dogfooding-dotnet`. - Env: `staging`. - Scenario: `span_enrichment.aggregate`. - Query site: `https://dd.datad0g.com`. - Query: `service:ffe-dogfooding-dotnet env:staging @validation_id:ffe-span-dotnet-20260619121831-b294dd1`. - Query result: 8 matching spans. - Trace Explorer: https://dd.datad0g.com/apm/traces?end=1781904227862&historicalData=true&paused=true&query=service%3Affe-dogfooding-dotnet+env%3Astaging+%40validation_id%3Affe-span-dotnet-20260619121831-b294dd1&start=1781817827862 - Indexed span: `trace_id=6a35358d0000000091168b7fb4b815a2`, `span_id=15600877720192993068`, `resource=POST /scenario`, `ffe_flags_enc=vQblBKMP`. - Indexed span: `trace_id=6a3534fd000000002e93ea8b4f41ec5d`, `span_id=12009548985036661845`, `resource=POST /scenario`, `ffe_flags_enc=vQblBKMP`. Original commits: * cd01e22 feat(02-03): add split serial id to UFC model + surface FFE metadata + span-enrichment gate * dccfd4e feat(02-03): ULeb128 codec + per-root-span enrichment state/store + Finally hook + Span.Finish write * b30c08d test(02-03): xUnit L0 span-enrichment suite (7 required cases + max-200) + finish-time SetTag fix * 56a5789 fix(02-03): synchronize SpanEnrichmentState mutation + snapshot ToSpanTags (CR-01) * 7022962 fix(02-03): wrap FFE enrichment block in Span.Finish in never-throw guard (WR-02) * c7ad723 fix(02-03): bound SpanEnrichmentStore growth when a root never finishes (WR-03) * 444cf7a test(02-03): concurrency + never-throw + growth-bound regression tests (CR-01/WR-02/WR-03) * f763d31 chore: sync generated calltarget + trimming descriptor files for span-enrichment integrations * d694d56 [FeatureFlags] Make span-enrichment gate-off inert and store lifecycle atomic * e5fc41e [FeatureFlags] Add OpenFeature span-enrichment integration test * f97218f [FeatureFlags] Strip internal planning labels from span-enrichment comments * 1ea7d22 [FeatureFlags] Unwrap OpenFeature Value for object runtime-default span enrichment * 21c394e Fix span enrichment test member ordering * b340190 Fix native feature flag span enrichment * b294dd1 Enable OpenFeature span enrichment for netstandard * 0795f28 Tighten FFE span enrichment shim * 7544055 Move FFE span enrichment state to tracer manager * 7c55c34 Address FFE span enrichment review comments * 0bb0c30 Merge branch 'master' of github.com:DataDog/dd-trace-dotnet into leo.romanovsky/ffe-apm-span-enrichment * d43b132 Regenerate generated files after merging master * bcabc1c [FeatureFlags] Store FFE span enrichment state on TraceContext instead of a central store (#8869) * 64c10ac Nits fix * 88c2127 Merge branch 'master' into leo.romanovsky/ffe-apm-span-enrichment * b9ae537 Merge branch 'master' into leo.romanovsky/ffe-apm-span-enrichment * 658c197 Address Copilot feedback: Volatile.Read FFE state getter, fix integration-test span wait * e307716 Merge master * 2d45803 Merge branch 'leo.romanovsky/ffe-apm-span-enrichment' of github.com:DataDog/dd-trace-dotnet into leo.romanovsky/ffe-apm-span-enrichment Signed-off-by: Pavel <pavlo.khrebto@datadoghq.com>
pavlokhrebto
force-pushed
the
leo.romanovsky/ffe-apm-span-enrichment
branch
from
July 15, 2026 12:22
2d45803 to
401a696
Compare
dudikeleti
reviewed
Jul 15, 2026
dudikeleti
left a comment
dudikeleti
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this.
Overall, the changes look good to me from the areas I’m familiar with. I left a few non-blocking comments. PTAL.
| var manager = tracer.TracerManager; | ||
| var res = manager.FeatureFlags?.Evaluate(parameters.FlagKey, parameters.TargetType, parameters.DefaultValue, parameters.TargetingKey ?? string.Empty, parameters.Attributes); | ||
| var traceContext = tracer.InternalActiveScope?.Span?.Context.TraceContext; | ||
| traceContext?.GetOrCreateFeatureFlagEnrichment()?.AccumulateEvaluation(res, parameters.TargetingKey); |
Contributor
There was a problem hiding this comment.
Consider (if its not too large refactor) to check first if we really need to accumulate before creating the state, currently we allocate a bunch of objects when we might drop everything later. checking eligibility first would keep these no-op evaluations allocation-free. (same in OpenFeatureSdkAccumulateSpanEnrichmentIntegration.cs)
Contributor
There was a problem hiding this comment.
The gate already lives in GetOrCreateFeatureFlagEnrichment(), which returns null when enrichment is disabled — so with the feature off these callsites (?.AccumulateEvaluation(...) / ?.Accumulate(...)) are fully no-op and allocate nothing. When it's on, the state is created lazily once per trace (not per evaluation) and reused, so a non-recording eval doesn't allocate a new state; the only cost is the one-time state on the first eligible eval. Given that, an extra eligibility pre-check would add branching for little gain — happy to revisit if profiling shows otherwise.
Contributor
There was a problem hiding this comment.
The gate already lives in GetOrCreateFeatureFlagEnrichment(), which returns null when enrichment is disabled
The disabled case is indeed allocation-free. My concern is when enrichment is enabled but none of the feature-flag evaluations in a trace are eligible for recording. The first evaluation still creates SpanEnrichmentState and its collections, which remain empty and unused for the entire trace. This may be an edge case, so it's up to you whether it's worth changing, but determining eligibility before creating the state would avoid those allocations.
Sort+dedupe serial ids via Array.Sort instead of a SortedSet (drop the unused overload) and write varint bytes directly into the payload buffer; skip the runtime-default value conversion in the OpenFeature hook unless the eval records a default; and only emit do_log/split-serial-id metadata (and its ToString) when span enrichment is enabled.
…ataDog/dd-trace-dotnet into leo.romanovsky/ffe-apm-span-enrichment
andrewlock
reviewed
Jul 24, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Thanks for the tracecontext refactor, It's looking much better! Most of my suggestions are about ways to improve perf in general - one of the concerns is that basically all of the production code runs on the hot path currently, so we need to be careful, especially with allocations.
Obviously a good way to mitigate that would be to move the serialization to the background thread, but I'm fine with deferring that to a follow up PR obviously!
| catch (Exception ex) | ||
| { | ||
| // Enrichment must never break flag evaluation. | ||
| System.Diagnostics.Debug.WriteLine($"[Datadog] SpanEnrichmentHook.FinallyAsync failed: {ex}"); |
Member
There was a problem hiding this comment.
The https://www.nuget.org/packages/OpenFeature package depends on the .NET logging packages, which suggests that the "correct" place to log this is using the .NET ILogger abstraction. That said, given we don't use it anywhere here currently, and plumbing it through is prob a bit of a pain, my preference is to just remove the System.Diagnostics.Debug.WriteLine anyway. It literally isn't in the compiled code we ship, so it mostly ends up just being noise
Early-return when feature flags are disabled; drop IDisposable and the Debug.WriteLine from the OpenFeature hook and pattern-match Value access; return a concrete List and inline the default truncation; use fluent assertions and move codec/Sha256 tests into dedicated test classes.
Contributor
|
Thanks @andrewlock! Agreed across the board. I pushed the quick wins and will take the bigger allocation/serialization work as the follow-up PR you offered. Addressed in this PR:
Follow-up PR (the hot-path/allocation work, since it's all tied to the serializer move):
Re |
## Summary of changes Stacked on top of #8795 (FFE APM span enrichment). Moves the `ffe_*` tag encoding/serialization off the customer's `Span.Finish()` path and onto the serializer thread, plus two follow-ups Andrew agreed to defer from that PR: `ULeb128Encoder` allocation reductions and a wire-format snapshot test. ## Reason for change In #8795, encoding the feature-flag serial ids (ULEB128 + base64), serializing the subjects/runtime-defaults JSON, and hashing the targeting keys all ran synchronously inside `Span.Finish()` — i.e. on the customer's thread. This work is only needed at serialization time, so it belongs on the background serializer thread where it stays off the hot path. ## Implementation details - **Serializer-thread write path.** Removed the `ffe_*` write block from `Span.Finish()`. `SpanMessagePackFormatter.WriteTags` now writes the tags for the local-root span only, reading `Context.TraceContext.FeatureFlagEnrichment`. Because the formatter back-patches the meta-map count, conditionally injecting tags is safe. - **`SpanEnrichmentState.BuildSpanTags()`** replaces `ToSpanTags()`, returning a new `readonly struct FeatureFlagSpanTags` (no `List`/`Dictionary` for the tag set). It snapshots the bounded state under the lock, then encodes/serializes after releasing it, and never throws — on failure it returns `default` so enrichment can never break span serialization. - **Delay-hashed subjects.** Targeting keys are now stored raw and SHA256-hashed in `BuildSpanTags()` (serializer thread) instead of at accumulation time, so the hash no longer runs on the evaluation path. - **`ULeb128Encoder` allocations.** Small id sets encode entirely on the stack on modern runtimes (`stackalloc` + `Span.Sort` + `ReadOnlySpan` base64); larger sets and `net461`/`netstandard2.0` use `ArrayPool` for both buffers. Output is byte-identical (golden vector `ZAgUAg==` unchanged). - CI-visibility formatter intentionally untouched — FFE enrichment is APM-only. ## Test coverage - Guardrail tests rewritten to assert `ffe_*` tags via the real serialize → deserialize (`MockSpan`) path rather than off `Span.Finish()`: gate-on/gate-off controls, no-data, per-trace isolation, root-only placement, a "enrichment throws → serialization still succeeds" test, and a concurrent Add-racing-`BuildSpanTags` regression. - New `Serialize_FfeWireFormat_MatchesFrozenSnapshot`: byte-exact snapshot of the three `ffe_*` values through the formatter, locking the frozen cross-SDK contract (tag names + codec output + compact JSON). - `ULeb128EncoderTests` unchanged and still green (output contract preserved). ## Other details Stacked PR — base is `leo.romanovsky/ffe-apm-span-enrichment`; review/merge #8795 first.
andrewlock
approved these changes
Jul 29, 2026
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.
Motivation
dd-trace-dotnet.ffe_*wire format used bydd-trace-js#8343so backend/Trino decode uses the same tags and encodings.DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED.Changes
DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLEDinTracerSettingsandsupported-configurations.yaml.Splitmodel.ULeb128Encoderfor delta-varint feature-flag serial ID encoding.SpanEnrichmentStateandSpanEnrichmentStore.SpanEnrichmentHookfor the OpenFeatureFinallyhook path.ffe_flags_enc,ffe_subjects_enc, andffe_runtime_defaultsonSpan.Finish.SpanEnrichmentStatemutation andToSpanTags.Span.Finishenrichment block.SpanEnrichmentStoregrowth for root spans that never finish.OpenFeature.Model.Valueruntime defaults into plain CLR values before JSON serialization.Decisions
DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED=false.ffe_*tags are emitted when the gate is off.ffe_*tags are bare spanmetatags, not_dd.-prefixed tags.doLog=true.Validation
system-testsparametric suite:TEST_LIBRARY=dotnet ./run.sh PARAMETRIC -k span_enrichment Library: dotnet@3.46.0 ===================== 18 passed, 2605 deselected in 46.26s =====================FFE dogfooding local trace-intake run:
DatadogProvider/ OpenFeature client.aspnet_core.request.ffe-dogfooding-dotnet.ffe_flags_encdecoded to[829, 1442, 2311, 2312].ffe_subjects_enccontained 2 SHA256 subject keys.ffe_*tags.ZAgUAg==decoded to[100, 108, 128, 130].Staging Datadog APM proof:
b294dd1and the staging Agent withDD_SITE=datad0g.com.ffe-span-dotnet-20260619121831-b294dd1.ffe-dogfooding-dotnet.staging.span_enrichment.aggregate.https://dd.datad0g.com.service:ffe-dogfooding-dotnet env:staging @validation_id:ffe-span-dotnet-20260619121831-b294dd1.trace_id=6a35358d0000000091168b7fb4b815a2,span_id=15600877720192993068,resource=POST /scenario,ffe_flags_enc=vQblBKMP.trace_id=6a3534fd000000002e93ea8b4f41ec5d,span_id=12009548985036661845,resource=POST /scenario,ffe_flags_enc=vQblBKMP.