Repository navigation
[Feature Flags] Add per-subject evaluation telemetry opt-out - #9426
leoromanovsky wants to merge 5 commits into
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9426) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-10-19 02:27:36 Comparing candidate commit 773c321 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 10 known flaky benchmarks, 116 flaky benchmarks without significant changes.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Static review supports the per-evaluation isolation of opt-out decisions and preservation of flag results and exposures. Runtime verification was blocked by the missing .NET 11 SDK.
🤖 Bits Code Review · Commit 9bde62f
| /// <summary> Gets provider hooks for flag evaluation metrics tracking. </summary> | ||
| /// <returns> Returns the list of provider hooks. </returns> | ||
| public override IImmutableList<Hook> GetProviderHooks() => _providerHooks; | ||
| public override IImmutableList<Hook> GetProviderHooks() |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
Heads up: #9351 adds flagevaluation EVP events through a new provider hook. Both PRs edit DatadogProvider.cs and DatadogProvider.Experimental.cs, so whichever lands second has two small follow-ups.
I merged #9351's head (e452d59) with this PR's head (d7a8fdd) and kept both sides of each conflict. This method then wraps the EVP hook as well: with the parameter on, the probe saw no EVP enqueue call for an opted-out subject, async or sync. This PR's tests also catch a resolution that drops its changes. What remains:
README.md:143says "The provider does not currently produce evaluation EVP events." That becomes false.- No test asserts EVP suppression. The Java PR (Add per-subject evaluation telemetry opt-out dd-trace-java#12884) lists EVP events among the suppressed telemetry.
Evidence — #9351 merged with this PR
The handwritten conflicts are in DatadogProvider.cs (the comment above _providerHooks) and DatadogProvider.Experimental.cs (ResolveSync). I kept #9351's _evpHook argument to Resolve and this PR's GetProviderHooks() call. The four generated ConfigurationKeys.FeatureFlags.g.cs files also conflict; building Datadog.Trace regenerated the net6.0 copy with both keys. GetProviderHooks() itself merges cleanly, because #9351 does not change it. The merged provider suite passes 834/834 on net8.0.
Probe with an injected FlagEvalEVPHook (test-only seam below):
| Path | Parameter | Opted out | Enqueue callback calls |
|---|---|---|---|
| Async | off | yes | 1 |
| Async | on | no | 1 |
| Async | on | yes | 0 |
| Sync | on | no | 1 |
| Sync | on | yes | 0 |
Resolutions that drop this PR's changes:
- Restoring
GetProviderHooks() => _providerHooksturns 27 of this PR's tests red. - Taking [Feature Flags] Add privacy-protected flagevaluation EVP events through the Agent #9351's side of
ResolveSync(_providerHooks) turns 1 red:SynchronousEvaluationUsesItsSuppliedContext.
Possible EVP test — needs an injection seam for the EVP hook
Seam in the internal constructor of the merged provider:
internal DatadogProvider(DatadogProviderOptions options, FeatureFlagsSdk.EvaluationCallback? evaluate, Action<long?, bool, string?, bool, string, object?>? accumulateSpanEnrichment, FlagEvalEVPHook? evpHook = null)
{
// ...
if (evpHook is not null)
{
_evpHook = evpHook;
}
else if (FeatureFlagsSdk.IsEvaluationEventsEnabled())
{
_evpHook = new FlagEvalEVPHook();
}
// ...
}Test, as I ran it in the merged tree:
// <copyright file="ProbeEvpCompositionTests.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>
#nullable enable
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Datadog.Trace.FeatureFlags;
using OpenFeature;
using OpenFeature.Model;
using Xunit;
using Xunit.Abstractions;
namespace Datadog.FeatureFlags.OpenFeature.Tests;
// REVIEW PROBE (local only): #9351's EVP hook inside #9426's opt-out wrapper.
[Collection(nameof(EvaluationTelemetryOptOutCollection))]
public class ProbeEvpCompositionTests(ITestOutputHelper output) : IAsyncLifetime
{
public Task InitializeAsync() => Task.CompletedTask;
public Task DisposeAsync() => Api.Instance.ShutdownAsync();
[Theory]
[InlineData(false, true, 1)]
[InlineData(true, false, 1)]
[InlineData(true, true, 0)]
public async Task AsyncEvpEventsFollowTheOptOut(bool enabled, bool optedOut, int expected)
{
var enqueues = 0;
using var provider = CreateProvider(enabled, () => Interlocked.Increment(ref enqueues));
var domain = Guid.NewGuid().ToString("N");
await Api.Instance.SetProviderAsync(domain, provider);
var result = await Api.Instance.GetClient(domain).GetBooleanDetailsAsync("flag", false, Context(optedOut));
output.WriteLine($"async enabled={enabled} optedOut={optedOut} value={result.Value} evp_enqueues={enqueues}");
Assert.True(result.Value);
Assert.Equal(expected, enqueues);
}
[Theory]
[InlineData(false, 1)]
[InlineData(true, 0)]
public void SyncEvpEventsFollowTheOptOut(bool optedOut, int expected)
{
var enqueues = 0;
using var provider = CreateProvider(enabled: true, () => Interlocked.Increment(ref enqueues));
#pragma warning disable DDFF001
var result = provider.ResolveBooleanValue("flag", false, Context(optedOut));
#pragma warning restore DDFF001
output.WriteLine($"sync optedOut={optedOut} value={result.Value} evp_enqueues={enqueues}");
Assert.True(result.Value);
Assert.Equal(expected, enqueues);
}
private static DatadogProvider CreateProvider(bool enabled, Action onEnqueue)
=> new(
new DatadogProviderOptions { EnableEvaluationTelemetryOptOutParameter = enabled },
(key, _, _, _, _) => new StubEvaluation(key),
accumulateSpanEnrichment: null,
evpHook: new FlagEvalEVPHook(() => true, (_, _, _, _, _, _, _, _) => onEnqueue(), () => { }, FlagEvaluationContextSnapshot.Capture));
private static EvaluationContext Context(bool optedOut)
=> EvaluationContext.Builder().SetTargetingKey("subject").Set("dd_evaluation_telemetry_opt_out", optedOut).Build();
private sealed class StubEvaluation(string flagKey) : IEvaluation
{
public string FlagKey => flagKey;
public object? Value => true;
public EvaluationReason Reason => EvaluationReason.Static;
public string? Variant => "on";
public string? Error => null;
public IDictionary<string, string>? FlagMetadata { get; } = new Dictionary<string, string> { ["__dd_allocation_key"] = "allocation" };
}
}There was a problem hiding this comment.
Leaving this open as the merge-time follow-up for #9351, which is still unmerged. This branch has no EVP evaluation-event producer. When the branches combine, the remaining work is to preserve the opt-out wrappers on both paths, add EVP enqueue-count tests, and update the README. The current shared runs explicitly exclude EVP evaluation events; they verify metrics, span annotations, unchanged exposures, targeting, and context retention.
| { | ||
| internal static CallTargetReturn<bool> OnMethodEnd<TTarget>(bool returnValue, Exception? exception, in CallTargetState state) | ||
| { | ||
| var enabled = Datadog.Trace.Tracer.Instance.Settings.FeatureFlags.EvaluationTelemetryPerSubjectOptOutEnabled; |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] Optional: add a unit test for this OnMethodEnd, so a regression here fails without the native profiler.
The new FeatureFlagsSettingsTests cover the configuration side well. No unit test calls this method, though. If it returned returnValue (the provider's environment fallback) instead of the tracer setting, all 47 FeatureFlagsSettingsTests would still pass. Without such a test, only an instrumented run where the tracer setting differs from the environment fallback would expose it.
Evidence — a broken integration passes all 47 FeatureFlagsSettingsTests
At 773c321 on net8.0, Datadog.Trace.Tests filtered to FeatureFlagsSettingsTests plus the test below:
| Integration | 47 FeatureFlagsSettingsTests |
Test below (2 rows) |
|---|---|---|
| As written | pass | pass |
Returns returnValue |
pass | 2 red |
Suggested test — tracer/test/Datadog.Trace.Tests/FeatureFlags/OpenFeatureSdkIsEvaluationTelemetryOptOutEnabledIntegrationTests.cs
// <copyright file="OpenFeatureSdkIsEvaluationTelemetryOptOutEnabledIntegrationTests.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>
#nullable enable
using Datadog.Trace.ClrProfiler.AutoInstrumentation.ManualInstrumentation.OpenFeature;
using Datadog.Trace.Configuration;
using Datadog.Trace.TestHelpers;
using FluentAssertions;
using Xunit;
namespace Datadog.Trace.Tests.FeatureFlags;
[Collection(nameof(TracerInstanceTestCollection))]
[TracerRestorer]
public class OpenFeatureSdkIsEvaluationTelemetryOptOutEnabledIntegrationTests
{
[Theory]
[InlineData(true)]
[InlineData(false)]
public void ReplacesTheProviderFallbackWithTheTracerSetting(bool configured)
{
Tracer.Configure(TracerSettings.Create(new()
{
{ ConfigurationKeys.FeatureFlags.EvaluationTelemetryPerSubjectOptOutEnabled, configured },
{ ConfigurationKeys.StartupDiagnosticLogEnabled, false },
{ ConfigurationKeys.TracerMetricsEnabled, false },
}));
// The provider's uninstrumented result is the opposite, so only the tracer setting can pass.
OpenFeatureSdkIsEvaluationTelemetryOptOutEnabledIntegration
.OnMethodEnd<object>(!configured, exception: null, state: default)
.GetReturnValue()
.Should()
.Be(configured);
}
}
|
|
||
| Before OpenFeature 2.5.0, the provider cannot see a replacement context when a later Before hook throws. | ||
| Setting opt-out in the supplied context covers this aborted evaluation path on those versions. | ||
| The suite checks both this limitation and the corrected behavior in newer OpenFeature versions. |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] Optional: say which OpenFeature version CI runs this test with. As written, a reader may assume CI covers both behaviors.
The test adapts to the installed OpenFeature version, and that works: it passes on OpenFeature 2.3.0, 2.4.0, 2.5.0, and 2.14.1, and on 2.14.1 it still catches an injected bug in the Finally fallback. But the test project gets OpenFeature 2.3.0 from the provider's reference and adds none of its own. So CI runs only the pre-2.5.0 branch.
Evidence — runs at 773c321 on net8.0
Datadog.FeatureFlags.OpenFeature.csproj:40references OpenFeature 2.3.0.Datadog.FeatureFlags.OpenFeature.Tests.csprojhas no OpenFeature reference.- With the test project forced to each version, the provider suite passes 750/750 on 2.3.0, 2.4.0, 2.5.0, and 2.14.1.
- Mutant
(_optedOut ?? false)inEvaluationTelemetryOptOutHook.FinallyAsync: on 2.14.1,BeforeFailureUsesTheContextExposedByOpenFeaturegoes red (4 red in total). On 2.3.0 that test stays green (3 red, all from other tests).
Suggested text — in place of line 65
"The error-path test adapts to the installed OpenFeature version. CI runs it with OpenFeature 2.3.0, so the newer behavior is checked only when the test project resolves 2.5.0 or later."
andrewlock
left a comment
andrewlock
left a comment
There was a problem hiding this comment.
Please follow the PR template.
Mostly just looked at the apm-owned files, and I don't think they should be changed. Also noticed problematic changes in the csproj file, but didn't review the FF SDK code much.
I'd strongly recommend rewriting the README by hand, as it's written for customers so should clearly explain the feature, not simply be a list list of caveats and design decisions.
| An unset option uses the tracer's normal configuration sources, including environment variables, | ||
| `datadog.json`, application configuration, and stable configuration. |
There was a problem hiding this comment.
nit: I would probably remove the explicit listing of configuration sources.
| An unset option uses the tracer's normal configuration sources, including environment variables, | |
| `datadog.json`, application configuration, and stable configuration. | |
| An unset option uses the tracer's normal configuration sources. |
| An explicit `true` or `false` option overrides tracer configuration. | ||
| An unset option uses the tracer's normal configuration sources, including environment variables, | ||
| `datadog.json`, application configuration, and stable configuration. | ||
| Without instrumentation, the provider reads the process environment only. |
There was a problem hiding this comment.
Without instrumentation, none of this works anyway?
| Without instrumentation, the provider reads the process environment only. |
| An unset option uses the tracer's normal configuration sources, including environment variables, | ||
| `datadog.json`, application configuration, and stable configuration. | ||
| Without instrumentation, the provider reads the process environment only. | ||
| Missing or invalid values disable the parameter. |
There was a problem hiding this comment.
This is just how configuration works in general, I don't think it should be listed here, especially as the flag is a boolean in code, so they can't get it wrong
| Missing or invalid values disable the parameter. |
| `datadog.json`, application configuration, and stable configuration. | ||
| Without instrumentation, the provider reads the process environment only. | ||
| Missing or invalid values disable the parameter. | ||
| The provider copies the resolved setting at construction. Later configuration or option changes do not affect it. |
There was a problem hiding this comment.
What happens if they call await OpenFeature.Api.Instance.SetProviderAsync(provider); again? Is that something we have tested or support in general? Should we?
| Only boolean `true` suppresses new Datadog evaluation metrics and automatic flag | ||
| span annotations. Missing values, strings, numbers, null, and boolean `false` | ||
| preserve telemetry. The provider does not currently produce evaluation EVP events. |
There was a problem hiding this comment.
This doesn't seem like it should be customer-facing documentation, I find it more confusing than anything?
| <ItemGroup Condition="'$(TargetFramework)' == 'net8.0'"> | ||
| <!-- FlagEvalMetrics needs DiagnosticSource 9; newer OpenFeature versions do not bring it transitively. --> | ||
| <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="9.0.0" /> | ||
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
This is almost certainly not the correct approach 🤔 .NET 8 includes System.Diagnostics.DiagnosticSource v8.0.0, which is the same version that OpenFeature references by default. I don't see any reason that you should be forcing 9.0.0 here? If you're using features only introuduced in 9.0.0 then you would break other TFMs too 🤷♂️
|
|
||
| return null; | ||
| } | ||
| public static bool? ToBoolean(this string value) => BooleanConfigurationParser.Parse(value); |
There was a problem hiding this comment.
What was the reason for this change? This seems entirely unrelated to the intent of this PR, so should be extracted to a separate PR, explaining why it's needed etc
| { | ||
| try | ||
| { | ||
| var value = Environment.GetEnvironmentVariable(EvaluationTelemetryOptOutEnabled); |
There was a problem hiding this comment.
This seems wrong - AFAICT you're trying to "handle" the case where instrumentation is failed, but 1. You shouldn't do that - if instrumentation has failed, everything is broken anyway 2. This adds complexity and confusion 3. It only partially works anyway
|
|
||
| public class FeatureFlagsSettingsTests | ||
| { | ||
| [Theory] |
There was a problem hiding this comment.
I don't think these tests add any value - they're just testing the config system, which is already extensively tested.
| // These tests change the OpenFeature API context and transaction propagator. | ||
| [CollectionDefinition(nameof(EvaluationTelemetryOptOutCollection), DisableParallelization = true)] | ||
| public class EvaluationTelemetryOptOutCollection | ||
| { | ||
| } |
There was a problem hiding this comment.
Can't you write the tests without exercising global state?
This branch has not been deployed
3 participants
Motivation
Applications need per-subject control of flag evaluation telemetry while retaining flag results and experiment exposures.
Changes and Decisions
Enable support with
DD_FEATURE_FLAGS_EVALUATION_TELEMETRY_PER_SUBJECT_OPT_OUT_ENABLED=truethrough the tracer's normal configuration sources, or use the provider option below.An explicit provider option overrides tracer configuration, including
false. Support is disabled by default.Only boolean
trueatdd_evaluation_telemetry_opt_outsuppresses new evaluation metrics and automatic flag span annotations.The attribute remains available to targeting and exposure reporting.
Per-evaluation hook wrappers preserve context overrides and isolate nested and concurrent calls.
Flag results, experiment exposures, and previously recorded data remain unchanged.
flowchart LR C[Merged context] --> E[Flag evaluation] E --> R[Result and experiment exposure] C --> G{Option enabled and boolean true?} G -->|No| T[Evaluation metrics and span annotations] G -->|Yes| S[No new evaluation telemetry]The provider version becomes 2.3.3. Its net8.0 package explicitly declares DiagnosticSource 9.0, which newer OpenFeature versions do not supply.
OpenFeature versions before 2.5.0 can hide a Before hook's context change when a later Before hook throws.
For those versions, supply opt-out in the initial context to cover that error path.
This provider has no EVP evaluation-event producer; this change does not add one.
Validation
Local system tests passed against signed merge commit
773c32182e4972531bbc20bdedbd62978711a112.Harness:
DataDog/system-tests@20ce91eb06b1c893f6fbb781b8267f4e5e3dcf4d, with unchanged assertions and manifests.All four scenarios passed: enabled and disabled through the environment, then through JSON with the activation environment variable removed.
Each run passed three feature checks and two schema checks: 20 checks total, no failures or skips.
EVP evaluation events were excluded because their producer is in separate PR #9351.
The captures confirm suppression, positive telemetry controls, unchanged exposures and results, targeting, and context-attribute retention.
Native and managed components were rebuilt from the tested commit. Installed checksums and process memory maps confirm which SDK files ran.
Runtime image:
sha256:ca08859fa51085f8032555508e66bf92f4057762df1f5d10c4cdb1401af3ac5b.Runtime: Linux ARM64 and .NET 8.0.32. Native binaries used a Debug build; this is not performance evidence.
The JSON-only image uses the same SDK binaries. Its entrypoint selects a true or false JSON configuration and removes the activation environment variable.
Configuration telemetry confirms
origin=dd_config. Each run reports5 passed, 3641 deselected.Build commands, checksums, JUnit reports, and HTTP captures are retained locally in
binaries/dotnet-9426-merge-773c3218/; they are not uploaded.