Repository navigation
[Feature Flags] Bound exposure shutdown and own the existing EVP sender - #9353
Merged
leoromanovsky merged 4 commits intoOct 9, 2026
Merged
Conversation
This was referenced Oct 1, 2026
BenchmarksBenchmark execution time: 2026-10-08 20:49:58 Comparing candidate commit b5d39d0 in PR branch Found 0 performance improvements and 13 performance regressions! Performance is the same for 59 metrics, 0 unstable metrics, 77 known flaky benchmarks, 49 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9353) and master. ✅ No regressions detected |
leoromanovsky
marked this pull request as ready for review
October 2, 2026 11:55
leoromanovsky
requested review from
sameerank and
typotter
and removed request for
a team
October 2, 2026 11:55
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. |
![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: 9a8280d92d
ℹ️ 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".
typotter
approved these changes
Oct 2, 2026
sameerank
approved these changes
Oct 2, 2026
sameerank
left a comment
sameerank
left a comment
Contributor
There was a problem hiding this comment.
Nothing blocking! Just left some suggestions to strengthen the tests
vjfridge
approved these changes
Oct 2, 2026
This was referenced Oct 5, 2026
bouwkast
reviewed
Oct 7, 2026
bouwkast
left a comment
bouwkast
left a comment
Collaborator
There was a problem hiding this comment.
LGTM
Only file owned is tracer/test/Datadog.Trace.TestHelpers/TransportHelpers/TestRequestFactory.cs
bouwkast
approved these changes
Oct 7, 2026
bouwkast
left a comment
bouwkast
left a comment
Collaborator
There was a problem hiding this comment.
LGTM
Only file owned is tracer/test/Datadog.Trace.TestHelpers/TransportHelpers/TestRequestFactory.cs
Base automatically changed from
leo.romanovsky/fflsdk-189-dotnet-discovery
to
master
October 8, 2026 18:39
leoromanovsky
requested review from
zacharycmontoya
and removed request for
a team
October 8, 2026 18:39
leoromanovsky
added a commit
that referenced
this pull request
Oct 8, 2026
## Motivation
Changing the Agent endpoint at runtime must not make the SDK trust
capabilities learned from the previous Agent. Cached configuration and
late responses currently cross that boundary, which can misroute
telemetry or lose its producer identity.
```mermaid
flowchart LR
A[Old Agent request] --> C[Cached capabilities]
E[Endpoint changes] -. missing invalidation .-> C
C --> B[Replacement Agent]
```
## Changes and Decisions
Bind each discovery result to the immutable exporter settings used for
its request. Replacing the endpoint clears cached configuration and
refresh hashes; late responses from the old endpoint cannot publish
capabilities or hashes. An identical response from the replacement still
notifies consumers.
Discovery also reports whether the relay forwards both EVP
producer-identity headers. This exposes a capability; it does not change
any product's route selection or enable direct intake.
This is the foundation of the delivery stack: discovery here, [bounded
exposure lifecycle in
#9353](#9353), then
[Agentless fallback in
#9235](#9235).
```mermaid
flowchart LR
E[Current exporter settings] --> R[Discovery request]
R --> G{Still current?}
G -->|Yes| C[Publish capabilities for this endpoint]
G -->|No| D[Discard old response]
```
Preserve historical event routing while draining queued exposures safely during module shutdown. Environment: Datadog workspace
Retain module ownership and disposal without allocating an HTTP client for applications that never use exposures. Environment: Datadog workspace
leoromanovsky
force-pushed
the
leo.romanovsky/fflsdk-189-dotnet-exposure-lifecycle
branch
from
October 8, 2026 18:39
1994bbb to
62944ba
Compare
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Assert final-drain waiting, bounded test cleanup, settings subscription ownership, and the fixed-v2 subdomain header. Environment: Datadog workspace
Use a timed synchronous wait while preserving non-throwing disposal for faulted or canceled workers. Environment: Datadog workspace
leoromanovsky
deleted the
leo.romanovsky/fflsdk-189-dotnet-exposure-lifecycle
branch
October 9, 2026 00:01
btthomas
added a commit
that referenced
this pull request
Oct 9, 2026
<!-- dd-meta
{"pullId":"9ac193c2-eb77-42ce-a4d4-e2ed04f9838d","source":"chat","resourceId":"76aa26f6-855a-4421-9a0d-4b2b23657819","workflowId":"be3454f5-53c4-46de-933b-83a266833f7b","codeChangeId":"be3454f5-53c4-46de-933b-83a266833f7b","sourceType":"code_channel"}
-->
## Summary of changes
- Increase the shutdown allowance in
DisposeFlushesEventsQueuedWhileAnEarlierBatchIsSending from 5 seconds to
1 minute and explain its purpose.
## Reason for change
Under thread-pool starvation, the test's async continuations can run
after Dispose's wall-clock shutdown timeout expires. This causes the
assertion that Dispose is still waiting for the final batch to fail even
though the production behavior is correct. Azure build 210959 reported
this failure after 5.02 seconds, matching the previous 5-second
allowance.
## Implementation details
The longer allowance accommodates scheduling delays while the test
coordinates the two blocked sends. All synchronization and assertions
remain unchanged, including checking that Dispose waits for the final
send and that the queued exposure is flushed. Production shutdown
behavior and the separate bounded-wait test remain unchanged.
## Test coverage
Source comparison verified that only this test's shutdown allowance and
explanatory comment changed. Attempted the focused ExposureApiTests on
net8.0, but execution was blocked before build: global.json requires
.NET SDK 11.0.100-rc.1.26425.128 and the sandbox only has SDK 8.0.412.
No local reproduction or passing test run is claimed; CI validation is
still required.
## Other details
<!-- Fixes #{issue} -->
The affected test was introduced in #9353.
<!-- ⚠️ Note:
Where possible, please obtain 2 approvals prior to merging. Unless
CODEOWNERS specifies otherwise, for external teams it is typically best
to have one review from a team member, and one review from apm-dotnet.
Trivial changes do not require 2 reviews.
MergeQueue is NOT enabled in this repository. If you have write access
to the repo, the PR has 1-2 approvals (see above), and all of the
required checks have passed, you can use the Squash and Merge button to
merge the PR. If you don't have write access, or you need help, reach
out in the #apm-dotnet channel in Slack.
-->
---
PR by Bits - [View session in
Datadog](https://app.datadoghq.com/code/76aa26f6-855a-4421-9a0d-4b2b23657819)
Comment @DataDog to request changes
Co-authored-by: datadog-bits <263423550+datadog-bits@users.noreply.github.com>
Co-authored-by: btthomas <9452950+btthomas@users.noreply.github.com>
leoromanovsky
added a commit
that referenced
this pull request
Oct 15, 2026
## Motivation
Agentless configuration lets customers evaluate flags without running an
Agent, but exposure delivery still requires its local EVP proxy.
Evaluations can succeed while their experiment exposures are lost.
```mermaid
flowchart LR
C[Agentless configuration] --> E[Flag evaluation]
E --> X[Exposure batch]
X --> A[Local Agent required]
X -. missing route .-> D[Direct EVP intake]
```
## Changes and Decisions
This is the single-commit final activation after [endpoint-bound
discovery #9352](#9352)
and [bounded exposure lifecycle
#9353](#9353). It keeps
Remote Config on its historical fixed EVP v2 behavior and does not add
flag-evaluation emission.
Agentless delivery prefers local EVP v4, then v2, only when discovery
confirms forwarding of both SDK identity headers. Configured Agent path
prefixes are preserved. Otherwise it uses canonical HTTPS intake; API
keys remain direct-only, relay headers local-only, and redirects are
disabled for Agentless delivery.
Direct selection is sticky. An unavailable route has bounded recovery;
each batch has bounded discovery waiting. The current batch can move to
direct only after local 404/405 or a proven pre-send connection failure.
Ambiguous failures never replay that batch, preventing duplicate
exposures. Identity, credentials and this replay policy ship together.
```mermaid
flowchart LR
E[Agentless exposure batch] --> R{Selected route}
R -->|Identity-capable v4 or v2| A[Local relay]
R -->|No eligible relay| D[Direct HTTPS intake]
A -->|404 / 405 or proven pre-send failure| D
A -->|Ambiguous failure| N[No current-batch replay]
D --> S[Direct remains selected]
```
vjfridge
added a commit
that referenced
this pull request
Oct 19, 2026
…gh the Agent (#9351) ## Pre-merge blocker: no-Agent behavior - [#9352](#9352) and [#9353](#9353) are merged and incorporated into this branch. [#9235](#9235) remains open; revisit no-Agent `flagevaluation` delivery when that transport work lands. - The related shared/exposure transport work does not yet wire up no-Agent `flagevaluation` emission. Before merging this PR, decide and validate transport reuse or an explicit no-Agent gating policy. The current implementation remains Agent-only and can build/queue events that cannot be delivered without an Agent. [Review discussion](#9351 (comment)). ## Summary of changes Adds aggregated `flagevaluation` track EVP events for successful and unsuccessful Datadog OpenFeature evaluations, protecting targeting keys and context by default without changing flag results. - **Agent-only delivery today**, with the no-Agent decision blocked as described above; related transport: [#9235](#9235); replaces the stale [#9139](#9139) approach. - Follows merged [Go](DataDog/dd-trace-go#5151), [Java](DataDog/dd-trace-java#12042), [Python](DataDog/dd-trace-py#20354), and [Ruby](DataDog/dd-trace-rb#6222) implementations, with [PII guidance](https://github.com/DataDog/ffe-codegen-tools/pull/9) and [JS](DataDog/dd-trace-js#10464) as additional references. ## Key decisions / assumptions - **Private by default.** Hash valid targeting keys at output, omit context, and allowlist error codes. Only boolean `true` for `observeFullEvaluationData` allows raw keys and bounded context; consent and timestamp are captured once per evaluation. - **One consent value across merged RCM configs.** Full data requires consent from every currently active file (AND), recomputed on each update—not combined with historical configurations. Any false, absent, null, or malformed value selects protected mode. Existing flag-merging behavior stays unchanged. - **Background processing.** The caller captures metadata/bounded context and enqueues without waiting for space. A dedicated worker aggregates and sends, batching partial queues for up to 50 ms. Explicit flush and shutdown wake it immediately; periodic sends remain every 10 seconds. Flush/shutdown waits are bounded. The five-second request timeout does not bound legacy pipe/socket connection setup; that existing shared-transport gap is [deferred from this PR](#9351 (comment)). - **Stable service tags.** Capture service/env/version when the writer starts, after replaying the latest settings, and keep them for its lifetime—as Go/Java/Ruby/JS do. Later tag changes require writer recreation; Agent-address updates remain independent. - **Bounded entries, not a fixed memory ceiling.** Queue: **4,096**, matching Go/Java/Python/Ruby; full buckets: 10,000 per flag / 131,072 globally; degraded buckets: 32,768. Degraded events omit key/context and exclude consent from bucket identity; exhausted capacity drops telemetry, not evaluations. - **Bounded consented context.** Capture at most 256 fields with depth/size/visit limits, without scanning or sorting arbitrarily large inputs. Rare hash collisions in oversized maps can change the retained subset; only telemetry is affected. - **Scoped transport.** Use the Agent’s EVP v2 route, with no direct-intake fallback, discovery, or retries. Delivery is best-effort; Agent 7.84.0 preserved SDK identity headers in testing, while 7.83.1 stripped them. ## Measured performance overhead **Historical comparison: `b32fca42cc` vs master `f0114c8c71`, before the worker-batching change; 4,096-entry queue.** ARM64 Linux/glibc, .NET 10.0.12; 100,000-evaluation bursts, medians of three runs. CPUs are affinity slots, not quotas; process CPU/allocation include final flush, caller latency does not. | CPUs | Mode | Evaluation p50 / p95 (µs) | CPU (µs/evaluation) | Allocation (bytes/evaluation) | | --- | --- | --- | --- | --- | | 1 | Baseline | 48.47 / 60.21 | 50.47 | 6,088 | | 1 | Events disabled | 55.93 / 68.32 | 56.44 | 6,584 | | 1 | Default protected | 67.77 / 112.38 | 79.26 | 6,837 | | 1 | Full consent | 70.08 / 139.30 | 91.60 | 11,472 | | 2 | Baseline | 10.85 / 11.38 | 11.86 | 6,088 | | 2 | Events disabled | 12.23 / 12.79 | 13.49 | 6,584 | | 2 | Default protected | 23.42 / 27.38 | 34.74 | 6,836 | | 2 | Full consent | 28.18 / 31.96 | 43.64 | 11,470 | - Default protected mode adds **~19 µs / ~13 µs** median evaluation latency and **~57% / ~193%** process CPU per evaluation on one/two CPUs. Disabling events avoids the writer but is **not zero-overhead**. - Consented contexts add allocation. All **88 trials** passed the delivery/privacy audit, including paced, sparse, high-cardinality, and oversized-context workloads; **all 3,136,867 expected observations, including warmup, were retained**. - These synthetic measurements make the cost visible; they do not establish acceptable production overhead. .NET reviewer assessment remains open. A later [paired worker-batching comparison](#9351 (comment)) improved protected burst latency/CPU, with a documented overload/drop tradeoff; it is not a fresh comparison against master. Hook-list reuse ([FFL-3356](https://datadoghq.atlassian.net/browse/FFL-3356)) is now included through merged [#9334](#9334); the historical measurements above predate that change. [Historical methodology, full workload matrix, and memory results](#9351 (comment)). [Earlier experiments and validation history](#9351 (comment)) used different artifacts/settings and are not an isolated optimization comparison. ## Test coverage - **Latest local post-merge validation at [`cef427882a`](cef427882a854bad0710542bacc107d26191dd65):** targeted feature-flag, telemetry, and metadata tests: **1,312 passed / two skipped on each of .NET 10 and .NET Core 3.1**. Tracer builds passed for all four target frameworks with zero warnings/errors. Subsequent commits changed only CODEOWNERS; these runs do not replace final-head external/performance validation. - **At `7fe8e0651f`:** 780 feature-flag tests passed / two platform skips; benchmark project built with zero warnings/errors. Full tracer suite: **13,507 passed / 71 skipped / zero failed**. [Writer-attribution fix and regression coverage](7fe8e06). - **At `b32fca42cc`:** 746 feature-flag tests passed / one skipped; full tracer suite 13,473 passed / 70 skipped / zero failed. Benchmark project built without warnings/errors. - **[System-tests at `b32fca42cc`](#9351 (comment) all 51 Agent-only FFE tests passed, including all 12 EVP cases; baseline passed the other 39 and failed only those 12 unimplemented cases. All 12,000 degradation observations retained. - **[Dogfooding at `b32fca42cc`](#9351 (comment) identical results across 81 live, 24 controlled-consent, and six no-Agent requests. Verified emission, SDK identity, strict consent, and mixed-version/uninstrumented safety. - Signed companion harness fixes: [system-tests #7951](DataDog/system-tests#7951) and [ffe-dogfooding #131](https://github.com/ddoghq/ffe-dogfooding/pull/131), both awaiting review. [Earlier evidence](#9351 (comment)) is historical. ### Reviewer QA Compare flag results with emission on/off and with the Agent disconnected. Verify strict consent, protected payloads, and unchanged exposure/span-enrichment behavior; disabling emission must produce no new `flagevaluation` events. ## Blast radius - Updates the provider/tracer metadata bridge and adds a hook, worker, Agent requests, and diagnostic metrics. Both updated components are required for full functionality; mixed-version and uninstrumented behavior have dedicated tests. - Events default on when feature flagging is active; `DD_FLAGGING_EVALUATION_COUNTS_ENABLED=false` disables them. Hook registration is decided when the provider is constructed; enabling events after starting disabled requires provider recreation or an application restart. Adds CPU/allocation and best-effort shutdown work; consented snapshots retain more data. - No changes to targeting rules, returned values, exposure payloads, or configuration-fetching transport. Exposure Agent requests also receive timeout protection, subject to the legacy connection-setup limitation above. Existing Remote Config and Agentless configuration sources remain supported; event delivery is Agent-only. ## External follow-ups External/performance results above predate the latest review fixes and need a final-head refresh. Remaining coordination, in addition to the no-Agent blocker above: - Release owner: confirm provider/tracer release versions. - Companion harness PRs [#7951](DataDog/system-tests#7951) / [#131](https://github.com/ddoghq/ffe-dogfooding/pull/131): review and manual merge. [FFL-3356]: https://datadoghq.atlassian.net/browse/FFL-3356?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
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
Short-lived applications can lose exposures queued while another batch is being sent. Shutdown currently signals the worker without waiting for a final drain, and a concurrent evaluation can enqueue after shutdown begins.
flowchart LR E[Evaluations] --> Q[Exposure queue] S[Shutdown] -. missing final drain .-> Q Q --> A[Agent EVP v2]Changes and Decisions
Separate the existing local sender from exposure batching, give the Feature Flags module ownership of its lifetime, and dispose its settings subscriptions. Exposure shutdown rejects new enqueue attempts, drains queued events and waits at most ten seconds for the worker; the timed wait does not depend on an available ThreadPool worker, so a stalled network cannot indefinitely block exposure disposal.
This PR targets master following the merge of discovery #9352. Both configuration modes keep their historical fixed EVP v2 route, headers and redirect defaults. There is no discovery-based selection or direct-intake activation here; the complete Agentless policy remains in #9235.
flowchart LR S[Shutdown] --> R[Reject new exposures] R --> F[Final queue drain] F --> T[Existing fixed-v2 sender] T --> A[Local Agent] F --> B[Bounded wait]