From 34aa3937707ba0d8e94132ebbb62e9660fabc678 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Wed, 22 Jul 2026 17:36:57 -0400 Subject: [PATCH 01/19] Parse observeFullEvaluationData and hash targeting_key in flagevaluations events Adds the top-level observeFullEvaluationData boolean to the UFC model, plumbs it through to the EVP flagevaluation event serializer, and gates PII handling on it: when the flag is absent/false the targeting key is SHA-256 hashed (sha256_) and the raw evaluation context is omitted from the wire; when true the raw targeting key and context are emitted. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 --- .../api/openfeature/DDEvaluatorTest.java | 2 +- .../featureflag/FeatureFlaggingGateway.java | 10 +++ .../ufc/v1/ServerConfiguration.java | 3 + .../FeatureFlaggingGatewayTest.java | 22 ++++++ .../featureflag/FlagEvaluationPayloads.java | 19 ++++- .../featureflag/FlagEvaluationWriterImpl.java | 8 ++- .../FlagEvaluationPayloadsTest.java | 67 +++++++++++++++++- .../FlagEvaluationWriterImplTest.java | 70 +++++++++++++++++++ .../JsonApiUfcResponseParserTest.java | 41 +++++++++++ .../featureflag/ULeb128EncoderTest.java | 39 +++++++++++ 10 files changed, 274 insertions(+), 7 deletions(-) create mode 100644 products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ULeb128EncoderTest.java diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index da02bf72706..d663191a456 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -181,7 +181,7 @@ public void testNoAllocations() { flags.put("null-allocation", new Flag("target", true, null, null, null)); flags.put("empty-allocation", new Flag("target", true, null, null, emptyList())); final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); - evaluator.accept(new ServerConfiguration("", "", null, flags)); + evaluator.accept(new ServerConfiguration("", "", false, null, flags)); final EvaluationContext ctx = new MutableContext("target").setTargetingKey("allocation"); diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java index 0cca3d03de0..5cbe151c164 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java @@ -99,6 +99,16 @@ public static boolean isFlagEvaluationEnqueueEnabled() { return flagEvalEnqueueEnabled; } + /** + * Returns whether the currently active UFC environment has {@code observeFullEvaluationData} + * enabled. {@code false} (privacy-preserving default) when no UFC has been dispatched yet or when + * the field was absent/false on the last dispatched configuration. + */ + public static boolean isObserveFullEvaluationDataEnabled() { + final ServerConfiguration current = CURRENT_CONFIG.get(); + return current != null && current.observeFullEvaluationData; + } + public static void addSpanEnrichmentListener(final SpanEnrichmentListener listener) { SPAN_ENRICHMENT_LISTENERS.add(listener); } diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java index 221fc74079a..8128eca7b32 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java @@ -5,16 +5,19 @@ public class ServerConfiguration { public final String createdAt; public final String format; + public final boolean observeFullEvaluationData; public final Environment environment; public final Map flags; public ServerConfiguration( final String createdAt, final String format, + final boolean observeFullEvaluationData, final Environment environment, final Map flags) { this.createdAt = createdAt; this.format = format; + this.observeFullEvaluationData = observeFullEvaluationData; this.environment = environment; this.flags = flags; } diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java b/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java index e1a3291f5da..998dd40b3a0 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java @@ -1,11 +1,14 @@ package datadog.trace.api.featureflag; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import datadog.trace.api.featureflag.exposure.ExposureEvent; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; +import java.util.Collections; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -96,6 +99,25 @@ void testAttachingASpanEnrichmentListener() { verifyNoMoreInteractions(spanEnrichmentListener); } + @Test + void isObserveFullEvaluationDataEnabledDefaultsToFalseWithNoConfig() { + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + assertFalse(FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()); + } + + @Test + void isObserveFullEvaluationDataEnabledReflectsLastDispatchedConfig() { + final ServerConfiguration enabled = + new ServerConfiguration("", "", true, null, Collections.emptyMap()); + FeatureFlaggingGateway.dispatch(enabled); + assertTrue(FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()); + + final ServerConfiguration disabled = + new ServerConfiguration("", "", false, null, Collections.emptyMap()); + FeatureFlaggingGateway.dispatch(disabled); + assertFalse(FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()); + } + private static void clearCurrentServerConfiguration() { FeatureFlaggingGateway.dispatch((ServerConfiguration) null); } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java index c2b3cbfb517..142cef2a6a7 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java @@ -194,7 +194,9 @@ static class FlagEvaluationEvent { static FlagEvaluationEvent fromBucket( final FlagEvaluationAggregator.EvalBucket bucket, final boolean isFullTier, + final boolean observeFullEvaluationData, final long flushTimeMs) { + final boolean includeRawContext = isFullTier && observeFullEvaluationData; return new FlagEvaluationEvent( flushTimeMs, bucket.flagKey, @@ -203,10 +205,23 @@ static FlagEvaluationEvent fromBucket( bucket.count, bucket.variant, bucket.allocationKey, - isFullTier ? bucket.targetingKey : null, + resolveTargetingKey(bucket.targetingKey, isFullTier, observeFullEvaluationData), bucket.runtimeDefaultUsed, bucket.errorMessage, - isFullTier ? bucket.prunedAttrs : null); + includeRawContext ? bucket.prunedAttrs : null); + } + + private static String resolveTargetingKey( + final String rawTargetingKey, + final boolean isFullTier, + final boolean observeFullEvaluationData) { + if (!isFullTier || rawTargetingKey == null) { + return null; + } + if (observeFullEvaluationData) { + return rawTargetingKey; + } + return "sha256_" + ULeb128Encoder.hashTargetingKey(rawTargetingKey); } FlagEvaluationEvent withoutTargetingKeyAndContext() { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java index 685d85fec79..5777ed3e08e 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java @@ -471,15 +471,19 @@ void flush() { private List buildEventList() { final long flushTimeMs = System.currentTimeMillis(); + final boolean observeFullEvaluationData = + FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); final List events = new ArrayList<>(aggregator.bucketCount()); for (final FlagEvaluationAggregator.EvalBucket bucket : aggregator.fullBuckets()) { events.add( - FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket(bucket, true, flushTimeMs)); + FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( + bucket, true, observeFullEvaluationData, flushTimeMs)); } for (final FlagEvaluationAggregator.EvalBucket bucket : aggregator.degradedBuckets()) { events.add( - FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket(bucket, false, flushTimeMs)); + FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( + bucket, false, observeFullEvaluationData, flushTimeMs)); } return events; } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java index e3c2d120b00..65a8fa6e791 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java @@ -67,7 +67,7 @@ void eventFromFullBucketUsesFlushTimeAndEvaluationBounds() throws Exception { FlagEvaluationPayloads.buildPayloads( java.util.Collections.singletonList( FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( - bucket, true, flushTimeMs)), + bucket, true, true, flushTimeMs)), CONTEXT, 1_000_000)); @@ -88,7 +88,8 @@ void degradedTierEventOmitsTargetingKeyAndContext() throws Exception { firstPayload( FlagEvaluationPayloads.buildPayloads( java.util.Collections.singletonList( - FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket(bucket, false, EVAL_MS)), + FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( + bucket, false, true, EVAL_MS)), CONTEXT, 1_000_000)); @@ -97,6 +98,68 @@ void degradedTierEventOmitsTargetingKeyAndContext() throws Exception { assertNull(ev.get("context")); } + @Test + void fullTierWithObserveFullEvaluationDataTrueEmitsRawTargetingKeyAndContext() throws Exception { + final Map attrs = new HashMap<>(); + attrs.put("region", "us-east-1"); + final FlagEvaluationAggregator.EvalBucket bucket = + new FlagEvaluationAggregator.EvalBucket( + "pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", null, EVAL_MS, false, attrs); + + final Map json = + firstPayload( + FlagEvaluationPayloads.buildPayloads( + java.util.Collections.singletonList( + FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( + bucket, true, true, EVAL_MS)), + CONTEXT, + 1_000_000)); + + final Map ev = firstEvent(json); + assertEquals("jane.doe@datadoghq.com", ev.get("targeting_key")); + final Map ctx = (Map) ev.get("context"); + assertNotNull(ctx); + final Map evalAttrs = (Map) ctx.get("evaluation"); + assertNotNull(evalAttrs); + assertEquals("us-east-1", evalAttrs.get("region")); + } + + @Test + void fullTierWithObserveFullEvaluationDataFalseHashesTargetingKeyAndOmitsContext() + throws Exception { + final Map attrs = new HashMap<>(); + attrs.put("region", "us-east-1"); + final FlagEvaluationAggregator.EvalBucket bucket = + new FlagEvaluationAggregator.EvalBucket( + "pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", null, EVAL_MS, false, attrs); + + final FlagEvaluationPayloads.EncodedPayloads payloads = + FlagEvaluationPayloads.buildPayloads( + java.util.Collections.singletonList( + FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( + bucket, true, false, EVAL_MS)), + CONTEXT, + 1_000_000); + final String rawJson = + new String(payloads.bodies.get(0), java.nio.charset.StandardCharsets.UTF_8); + + // The raw wire bytes must carry the hashed key and must not leak the raw PII value or the + // per-event evaluation context — these are the exact properties system-tests asserts over the + // wire. (The batch envelope has its own top-level "context" field, so we guard on the nested + // "evaluation" key instead, which only appears inside a per-event context object.) + assertTrue( + rawJson.contains( + "sha256_b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b")); + assertFalse(rawJson.contains("jane.doe@datadoghq.com")); + assertFalse(rawJson.contains("\"evaluation\":")); + + final Map ev = firstEvent(parse(payloads.bodies.get(0))); + assertEquals( + "sha256_b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b", + ev.get("targeting_key")); + assertFalse(ev.containsKey("context")); + } + @Test void splitPayloadsByEncodedSize() throws Exception { final Map attrs = new HashMap<>(); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index 280b3028803..350edf2e83b 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -6,12 +6,14 @@ import static com.datadog.featureflag.FlagEvaluationTestSupport.clearCoreMetrics; import static com.datadog.featureflag.FlagEvaluationTestSupport.event; import static com.datadog.featureflag.FlagEvaluationTestSupport.eventForFlag; +import static com.datadog.featureflag.FlagEvaluationTestSupport.flushAndCapture; import static com.datadog.featureflag.FlagEvaluationTestSupport.flushAndCaptureJson; import static com.datadog.featureflag.FlagEvaluationTestSupport.metricSum; import static com.datadog.featureflag.FlagEvaluationTestSupport.repeat; import static com.datadog.featureflag.FlagEvaluationTestSupport.simpleEvent; import static java.util.Collections.emptyMap; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -32,6 +34,7 @@ import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.trace.api.featureflag.FeatureFlaggingGateway; import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; +import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import datadog.trace.api.intake.Intake; import datadog.trace.api.telemetry.CoreMetricCollector; import datadog.trace.api.telemetry.MetricCollector; @@ -62,6 +65,8 @@ void clearCoreMetricsAfter() { clearCoreMetrics(); FeatureFlaggingGateway.setFlagEvalWriter(null); FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + // Reset the dispatched UFC state so observeFullEvaluationData can't leak into other tests. + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); } @Test @@ -533,6 +538,71 @@ void splitPostFailureDoesNotRetryAlreadySentPayloads() throws Exception { assertEquals(2, posts.get()); } + private static final String HASHED_JANE_DOE = + "sha256_b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b"; + + @Test + void observeFullEvaluationDataTrueEmitsRawTargetingKeyAndContext() throws Exception { + dispatchObserveFullEvaluationData(true); + final BackendApi mockEvp = mock(BackendApi.class); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); + setup.handler.add(piiEvent()); + + final Map json = flushAndCapture(setup).parsed; + + final Map ev = eventForFlag(json, "pii-flag"); + assertNotNull(ev); + assertEquals("jane.doe@datadoghq.com", ev.get("targeting_key")); + final Map ctx = (Map) ev.get("context"); + assertNotNull(ctx); + final Map evalAttrs = (Map) ctx.get("evaluation"); + assertNotNull(evalAttrs); + assertEquals("us-east-1", evalAttrs.get("region")); + } + + @Test + void observeFullEvaluationDataFalseHashesTargetingKeyAndOmitsContext() throws Exception { + dispatchObserveFullEvaluationData(false); + assertHashedTargetingKeyAndOmittedContext(); + } + + @Test + void observeFullEvaluationDataAbsentDefaultsToHashedBehavior() throws Exception { + // No UFC dispatched (default state) — must behave exactly like the explicit "false" case. + assertHashedTargetingKeyAndOmittedContext(); + } + + private void assertHashedTargetingKeyAndOmittedContext() throws Exception { + final BackendApi mockEvp = mock(BackendApi.class); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); + setup.handler.add(piiEvent()); + + final FlagEvaluationTestSupport.CapturedJson captured = flushAndCapture(setup); + + final Map ev = eventForFlag(captured.parsed, "pii-flag"); + assertNotNull(ev); + assertEquals(HASHED_JANE_DOE, ev.get("targeting_key")); + assertFalse(ev.containsKey("context")); + // The raw wire bytes must carry the hashed key and never leak the raw PII value or a per-event + // evaluation context (the batch envelope owns the top-level "context" key, so guard on the + // nested "evaluation" field instead). + assertTrue(captured.raw.contains(HASHED_JANE_DOE)); + assertFalse(captured.raw.contains("jane.doe@datadoghq.com")); + assertFalse(captured.raw.contains("\"evaluation\":")); + } + + private static FlagEvalEvent piiEvent() { + final Map attrs = new HashMap<>(); + attrs.put("region", "us-east-1"); + return event("pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", 1000L, attrs); + } + + private static void dispatchObserveFullEvaluationData(final boolean value) { + FeatureFlaggingGateway.dispatch( + new ServerConfiguration( + "2024-04-17T19:40:53.716Z", "SERVER", value, null, java.util.Collections.emptyMap())); + } + private static Object lifecycleLock(final FlagEvaluationWriterImpl writer) throws Exception { final Field field = FlagEvaluationWriterImpl.class.getDeclaredField("lifecycleLock"); field.setAccessible(true); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java index a31d47889ba..239c9c434eb 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java @@ -2,6 +2,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -73,10 +74,50 @@ void rejectsTrailingJson() { + "}}{}")); } + @Test + void observeFullEvaluationDataDefaultsToFalseWhenAbsent() throws Exception { + final ServerConfiguration configuration = parse(wrap(emptyConfig())); + assertNotNull(configuration); + assertFalse(configuration.observeFullEvaluationData); + } + + @Test + void observeFullEvaluationDataParsesTrue() throws Exception { + final ServerConfiguration configuration = + parse(wrap(configWithObserveFullEvaluationData(true))); + assertNotNull(configuration); + assertTrue(configuration.observeFullEvaluationData); + } + + @Test + void observeFullEvaluationDataParsesFalse() throws Exception { + final ServerConfiguration configuration = + parse(wrap(configWithObserveFullEvaluationData(false))); + assertNotNull(configuration); + assertFalse(configuration.observeFullEvaluationData); + } + private static ServerConfiguration parse(final String json) throws Exception { return JsonApiUfcResponseParser.INSTANCE.parse(json.getBytes(UTF_8)); } + private static String wrap(final String attributes) { + return "{\"data\":{\"type\":\"universal-flag-configuration\",\"attributes\":" + + attributes + + "}}"; + } + + private static String configWithObserveFullEvaluationData(final boolean value) { + return "{" + + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," + + "\"observeFullEvaluationData\":" + + value + + "," + + "\"environment\":{\"name\":\"Test\"}," + + "\"flags\":{}" + + "}"; + } + private static String emptyConfig() { return "{" + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ULeb128EncoderTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ULeb128EncoderTest.java new file mode 100644 index 00000000000..f47fdeaef1c --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ULeb128EncoderTest.java @@ -0,0 +1,39 @@ +package com.datadog.featureflag; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class ULeb128EncoderTest { + + @Test + void hashTargetingKeyMatchesCanonicalPiiVector() { + // Canonical vector shared across all SDK implementations — see system-tests PR #7316. + assertEquals( + "b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b", + ULeb128Encoder.hashTargetingKey("jane.doe@datadoghq.com")); + } + + @Test + void hashTargetingKeyPreservesWhitespaceExactly() { + assertNotEquals( + ULeb128Encoder.hashTargetingKey("jane.doe@datadoghq.com"), + ULeb128Encoder.hashTargetingKey(" jane.doe@datadoghq.com ")); + } + + @Test + void hashTargetingKeyPreservesCaseExactly() { + assertNotEquals( + ULeb128Encoder.hashTargetingKey("jane.doe@datadoghq.com"), + ULeb128Encoder.hashTargetingKey("JANE.DOE@DATADOGHQ.COM")); + } + + @Test + void hashTargetingKeyIsLowercase64CharHex() { + final String hash = ULeb128Encoder.hashTargetingKey("some-arbitrary-key"); + assertEquals(64, hash.length()); + assertTrue(hash.matches("[0-9a-f]{64}")); + } +} From d65c79f266332b4ad0611acd20c415ca9d77ebdd Mon Sep 17 00:00:00 2001 From: "vickie.fridge" Date: Thu, 23 Jul 2026 15:58:41 +0000 Subject: [PATCH 02/19] Extract hashed targeting key prefix into a named constant Replace the inline "sha256_" literal with a documented HASHED_TARGETING_KEY_PREFIX constant describing the cross-SDK wire contract for privacy-preserving hashed targeting keys. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 --- .../datadog/featureflag/FlagEvaluationPayloads.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java index 142cef2a6a7..3a8734fd516 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationPayloads.java @@ -13,6 +13,15 @@ final class FlagEvaluationPayloads { private static final byte[] PAYLOAD_SUFFIX = FeatureFlagEvpPublisher.utf8Bytes("]}"); private static final byte[] JSON_COMMA = FeatureFlagEvpPublisher.utf8Bytes(","); + + /** + * Wire prefix identifying a privacy-preserving, hashed targeting key. Emitted for full-tier rows + * when {@code observeFullEvaluationData} is off. The suffix is the lower-case hex SHA-256 of the + * UTF-8 targeting key (see {@link ULeb128Encoder#hashTargetingKey}). This is a cross-SDK wire + * contract - keep it in sync with the other server SDKs and the UFC/EVP spec. + */ + private static final String HASHED_TARGETING_KEY_PREFIX = "sha256_"; + private static final JsonAdapter EVENT_JSON_ADAPTER; private static final JsonAdapter> CONTEXT_JSON_ADAPTER; @@ -221,7 +230,7 @@ private static String resolveTargetingKey( if (observeFullEvaluationData) { return rawTargetingKey; } - return "sha256_" + ULeb128Encoder.hashTargetingKey(rawTargetingKey); + return HASHED_TARGETING_KEY_PREFIX + ULeb128Encoder.hashTargetingKey(rawTargetingKey); } FlagEvaluationEvent withoutTargetingKeyAndContext() { From 4e9a0eadef62303dabdde2835a96a758004468c6 Mon Sep 17 00:00:00 2001 From: "vickie.fridge" Date: Thu, 23 Jul 2026 16:23:30 +0000 Subject: [PATCH 03/19] Test observeFullEvaluationData parsing edge cases Parameterize the true/false config-parsing assertions with @ValueSource and add a test locking in the fail-closed behaviour for an explicit JSON null: malformed config is rejected so full evaluation data is never observed off the back of it. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 --- .../JsonApiUfcResponseParserTest.java | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java index 239c9c434eb..3db21f3c8dc 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java @@ -11,6 +11,8 @@ import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import java.io.IOException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; class JsonApiUfcResponseParserTest { @@ -81,20 +83,24 @@ void observeFullEvaluationDataDefaultsToFalseWhenAbsent() throws Exception { assertFalse(configuration.observeFullEvaluationData); } - @Test - void observeFullEvaluationDataParsesTrue() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void observeFullEvaluationDataParsesExplicitValue(final boolean value) throws Exception { final ServerConfiguration configuration = - parse(wrap(configWithObserveFullEvaluationData(true))); + parse(wrap(configWithObserveFullEvaluationData(value))); assertNotNull(configuration); - assertTrue(configuration.observeFullEvaluationData); + assertEquals(value, configuration.observeFullEvaluationData); } @Test - void observeFullEvaluationDataParsesFalse() throws Exception { - final ServerConfiguration configuration = - parse(wrap(configWithObserveFullEvaluationData(false))); - assertNotNull(configuration); - assertFalse(configuration.observeFullEvaluationData); + void observeFullEvaluationDataRejectsExplicitNull() { + // An explicit null for this boolean is malformed input. Parsing rejects the whole + // configuration, which is the fail-closed outcome we want: full evaluation data is never + // observed off the back of a malformed config. Callers (AgentlessConfigurationSource and the + // remote-config poller) swallow the failure and keep the last-known-good config, and the + // gateway defaults to the privacy-preserving behaviour when no valid config was dispatched. + // Servers send true/false or omit the field; null is not a value they emit. + assertThrows(Exception.class, () -> parse(wrap(configWithNullObserveFullEvaluationData()))); } private static ServerConfiguration parse(final String json) throws Exception { @@ -118,6 +124,15 @@ private static String configWithObserveFullEvaluationData(final boolean value) { + "}"; } + private static String configWithNullObserveFullEvaluationData() { + return "{" + + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," + + "\"observeFullEvaluationData\":null," + + "\"environment\":{\"name\":\"Test\"}," + + "\"flags\":{}" + + "}"; + } + private static String emptyConfig() { return "{" + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," From 81af153ca75559668c7b34f53fd8ec80bee1ed1a Mon Sep 17 00:00:00 2001 From: "vickie.fridge" Date: Thu, 23 Jul 2026 17:59:53 +0000 Subject: [PATCH 04/19] Capture observeFullEvaluationData per bucket at aggregation time The flush-time read of FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled() was a TOCTOU bug: CURRENT_CONFIG could be overwritten by a later RC update between when an evaluation happened and when the batch flushed, so events could be emitted under the wrong environment's consent (the system test observed a targeting key hashed even though the active UFC said observeFullEvaluationData=true). Capture consent when the evaluation is folded into its EvalBucket instead. On merge the value is folded with AND, so any no-consent evaluation in a bucket's lifetime sinks the whole bucket to hashed/omitted (fail-closed). buildEventList now reads bucket.observeFullEvaluationData rather than the gateway. The gateway accessor is retained; it is read at aggregation time. Adds a writer-level regression guard (a bucket aggregated under consent-off stays hashed even if the gateway later reports consent-on) plus aggregator fold tests, and an end-to-end parse->dispatch->flush test. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 --- .../featureflag/FlagEvaluationAggregator.java | 31 +++++++-- .../featureflag/FlagEvaluationWriterImpl.java | 9 +-- .../FlagEvaluationAggregatorTest.java | 50 +++++++++++++- .../FlagEvaluationPayloadsTest.java | 24 +++++-- .../FlagEvaluationWriterImplTest.java | 67 +++++++++++++++++++ 5 files changed, 166 insertions(+), 15 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java index a146501f825..86b33a4e10c 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java @@ -1,5 +1,6 @@ package com.datadog.featureflag; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; import java.util.HashMap; import java.util.Map; @@ -44,6 +45,11 @@ final class FlagEvaluationAggregator { void aggregate(final FlagEvalEvent event) { final boolean isDefault = event.variant == null; + // Capture consent now, when the evaluation is folded into a bucket, so it reflects the + // configuration active at evaluation time rather than whatever CURRENT_CONFIG happens to be at + // flush. Existing buckets fold with AND: one no-consent evaluation sinks the bucket to hashed. + final boolean observeFullEvaluationData = + FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); final Map prunedAttrs = pruneContext(event.contextAttributes()); final String ctxKey = canonicalContextKey(prunedAttrs); final FullKey fullKey = buildFullKey(event, ctxKey); @@ -51,6 +57,7 @@ void aggregate(final FlagEvalEvent event) { EvalBucket bucket = fullTier.get(fullKey); if (bucket != null) { bucket.merge(event.evalTimeMs, isDefault); + bucket.observeFullEvaluationData &= observeFullEvaluationData; return; } @@ -66,7 +73,8 @@ void aggregate(final FlagEvalEvent event) { event.errorMessage, event.evalTimeMs, isDefault, - prunedAttrs)); + prunedAttrs, + observeFullEvaluationData)); globalFullCount.incrementAndGet(); perFlagCount.put(event.flagKey, flagCount + 1); return; @@ -76,6 +84,7 @@ void aggregate(final FlagEvalEvent event) { bucket = degradedTier.get(degradedKey); if (bucket != null) { bucket.merge(event.evalTimeMs, isDefault); + bucket.observeFullEvaluationData &= observeFullEvaluationData; return; } @@ -90,7 +99,8 @@ void aggregate(final FlagEvalEvent event) { event.errorMessage, event.evalTimeMs, isDefault, - null)); + null, + observeFullEvaluationData)); return; } @@ -142,7 +152,7 @@ void simulateFullTierAtCap() { final String key = "synthetic-full-" + i; fullTier.put( new FullKey(key, "on", "alloc", false, null, null, ""), - new EvalBucket(key, "on", "alloc", null, null, 1L, false, null)); + new EvalBucket(key, "on", "alloc", null, null, 1L, false, null, false)); globalFullCount.incrementAndGet(); perFlagCount.merge(key, 1, Integer::sum); } @@ -153,7 +163,7 @@ void simulateDegradedTierAtCap() { final String key = "synthetic-dg-" + i; degradedTier.put( new DegradedKey(key, "on", "alloc", false, null), - new EvalBucket(key, "on", "alloc", null, null, 1L, false, null)); + new EvalBucket(key, "on", "alloc", null, null, 1L, false, null, false)); } } @@ -173,7 +183,8 @@ void addDegradedBucketForTest( errorMessage, evalTimeMs, variant == null, - null)); + null, + false)); } private static FullKey buildFullKey(final FlagEvalEvent event, final String ctxKey) { @@ -272,6 +283,12 @@ static class EvalBucket { String targetingKey; String errorMessage; Map prunedAttrs; + // Consent to emit raw PII (targeting key + context) captured when this bucket was created, not + // read at flush time. CURRENT_CONFIG can be overwritten by a later RC update between capture + // and flush, so reading the gateway at flush would apply the wrong environment's consent. On + // merge the value is folded with AND (see aggregate()): if any evaluation in the bucket's + // lifetime saw consent off, the whole bucket falls back to hashed/omitted — fail-closed. + boolean observeFullEvaluationData; EvalBucket( final String flagKey, @@ -281,7 +298,8 @@ static class EvalBucket { final String errorMessage, final long evalTimeMs, final boolean runtimeDefaultUsed, - final Map prunedAttrs) { + final Map prunedAttrs, + final boolean observeFullEvaluationData) { this.flagKey = flagKey; this.variant = variant; this.allocationKey = allocationKey; @@ -292,6 +310,7 @@ static class EvalBucket { this.count = 1; this.runtimeDefaultUsed = runtimeDefaultUsed; this.prunedAttrs = prunedAttrs; + this.observeFullEvaluationData = observeFullEvaluationData; } int prunedContextFieldCount() { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java index 5777ed3e08e..b62c3728cf9 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java @@ -471,19 +471,20 @@ void flush() { private List buildEventList() { final long flushTimeMs = System.currentTimeMillis(); - final boolean observeFullEvaluationData = - FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); + // Consent is read per bucket from the value captured at aggregation time, not from the + // gateway here: CURRENT_CONFIG may have been overwritten by a later RC update since these + // evaluations happened, and reading it at flush would apply the wrong environment's consent. final List events = new ArrayList<>(aggregator.bucketCount()); for (final FlagEvaluationAggregator.EvalBucket bucket : aggregator.fullBuckets()) { events.add( FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( - bucket, true, observeFullEvaluationData, flushTimeMs)); + bucket, true, bucket.observeFullEvaluationData, flushTimeMs)); } for (final FlagEvaluationAggregator.EvalBucket bucket : aggregator.degradedBuckets()) { events.add( FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket( - bucket, false, observeFullEvaluationData, flushTimeMs)); + bucket, false, bucket.observeFullEvaluationData, flushTimeMs)); } return events; } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java index 57bc7251c1e..d324ca3012f 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java @@ -6,7 +6,9 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; +import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -212,7 +214,7 @@ void flagEvalEventDoesNotCarryReason() { void evalBucketTracksBoundsDefaultStateAndNullContextFieldCount() { final FlagEvaluationAggregator.EvalBucket bucket = new FlagEvaluationAggregator.EvalBucket( - "bucket-flag", "on", "alloc1", "user-1", null, 1000L, false, null); + "bucket-flag", "on", "alloc1", "user-1", null, 1000L, false, null, false); assertEquals(0, bucket.prunedContextFieldCount()); @@ -266,6 +268,52 @@ void degradedKeyEqualityUsesEveryDimension() { assertNotEquals(base, degradedKey("flag", "on", "alloc", false, "other")); } + @Test + void observeFullEvaluationDataFoldsToFalseWhenAnyMergedEvaluationLacksConsent() { + final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); + try { + FeatureFlaggingGateway.dispatch(observeConfig(true)); + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, emptyMap())); + // A later RC update turns consent off; the second evaluation folds into the same bucket. + FeatureFlaggingGateway.dispatch(observeConfig(false)); + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, emptyMap())); + + final FlagEvaluationAggregator.EvalBucket bucket = + aggregator.snapshot().fullTier.values().iterator().next(); + assertEquals(2, bucket.count); + // Conservative fold: one no-consent evaluation sinks the whole bucket to hashed/omitted. + assertFalse(bucket.observeFullEvaluationData); + } finally { + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + } + } + + @Test + void observeFullEvaluationDataStaysTrueWhenEveryMergedEvaluationConsents() { + final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); + try { + FeatureFlaggingGateway.dispatch(observeConfig(true)); + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, emptyMap())); + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, emptyMap())); + + final FlagEvaluationAggregator.EvalBucket bucket = + aggregator.snapshot().fullTier.values().iterator().next(); + assertEquals(2, bucket.count); + assertTrue(bucket.observeFullEvaluationData); + } finally { + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + } + } + + private static ServerConfiguration observeConfig(final boolean observeFullEvaluationData) { + return new ServerConfiguration( + "2024-04-17T19:40:53.716Z", + "SERVER", + observeFullEvaluationData, + null, + java.util.Collections.emptyMap()); + } + private static FlagEvalEvent event( final String flagKey, final String variant, diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java index 65a8fa6e791..d4ca517fffd 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationPayloadsTest.java @@ -58,7 +58,7 @@ void fullTierPayloadUsesWorkerWireShape() throws Exception { void eventFromFullBucketUsesFlushTimeAndEvaluationBounds() throws Exception { final FlagEvaluationAggregator.EvalBucket bucket = new FlagEvaluationAggregator.EvalBucket( - "ts-flag", "on", "alloc1", "user-1", null, EVAL_MS, false, emptyMap()); + "ts-flag", "on", "alloc1", "user-1", null, EVAL_MS, false, emptyMap(), true); bucket.merge(EVAL_MS + 10, false); final long flushTimeMs = EVAL_MS + 5_000; @@ -82,7 +82,7 @@ void eventFromFullBucketUsesFlushTimeAndEvaluationBounds() throws Exception { void degradedTierEventOmitsTargetingKeyAndContext() throws Exception { final FlagEvaluationAggregator.EvalBucket bucket = new FlagEvaluationAggregator.EvalBucket( - "dg-flag", "on", "alloc1", null, null, EVAL_MS, false, null); + "dg-flag", "on", "alloc1", null, null, EVAL_MS, false, null, false); final Map json = firstPayload( @@ -104,7 +104,15 @@ void fullTierWithObserveFullEvaluationDataTrueEmitsRawTargetingKeyAndContext() t attrs.put("region", "us-east-1"); final FlagEvaluationAggregator.EvalBucket bucket = new FlagEvaluationAggregator.EvalBucket( - "pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", null, EVAL_MS, false, attrs); + "pii-flag", + "on", + "alloc1", + "jane.doe@datadoghq.com", + null, + EVAL_MS, + false, + attrs, + true); final Map json = firstPayload( @@ -131,7 +139,15 @@ void fullTierWithObserveFullEvaluationDataFalseHashesTargetingKeyAndOmitsContext attrs.put("region", "us-east-1"); final FlagEvaluationAggregator.EvalBucket bucket = new FlagEvaluationAggregator.EvalBucket( - "pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", null, EVAL_MS, false, attrs); + "pii-flag", + "on", + "alloc1", + "jane.doe@datadoghq.com", + null, + EVAL_MS, + false, + attrs, + false); final FlagEvaluationPayloads.EncodedPayloads payloads = FlagEvaluationPayloads.buildPayloads( diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index 350edf2e83b..7d00ec2b3d1 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -560,6 +560,38 @@ void observeFullEvaluationDataTrueEmitsRawTargetingKeyAndContext() throws Except assertEquals("us-east-1", evalAttrs.get("region")); } + @Test + void observeFullEvaluationDataTrueFromParsedUfcEmitsRawTargetingKey() throws Exception { + // Exercises the config-source -> gateway -> writer seam that the hand-built dispatch skips: + // parse a UFC exactly as the (default) agentless source does, dispatch it, then flush. Guards + // against observeFullEvaluationData being lost between config parsing and the flush-time gate + // read. Field placement mirrors the system-test fixture (after "flags", with format SERVER). + final String attributes = + "{" + + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," + + "\"format\":\"SERVER\"," + + "\"environment\":{\"name\":\"Test\"}," + + "\"flags\":{}," + + "\"observeFullEvaluationData\":true" + + "}"; + final String wrapped = + "{\"data\":{\"type\":\"universal-flag-configuration\",\"attributes\":" + attributes + "}}"; + final ServerConfiguration parsed = + JsonApiUfcResponseParser.INSTANCE.parse( + wrapped.getBytes(java.nio.charset.StandardCharsets.UTF_8)); + assertNotNull(parsed); + assertTrue(parsed.observeFullEvaluationData); + FeatureFlaggingGateway.dispatch(parsed); + + final BackendApi mockEvp = mock(BackendApi.class); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); + setup.handler.add(piiEvent()); + + final Map ev = eventForFlag(flushAndCapture(setup).parsed, "pii-flag"); + assertNotNull(ev); + assertEquals("jane.doe@datadoghq.com", ev.get("targeting_key")); + } + @Test void observeFullEvaluationDataFalseHashesTargetingKeyAndOmitsContext() throws Exception { dispatchObserveFullEvaluationData(false); @@ -572,6 +604,41 @@ void observeFullEvaluationDataAbsentDefaultsToHashedBehavior() throws Exception assertHashedTargetingKeyAndOmittedContext(); } + @Test + void bucketCapturedUnderFalseStaysHashedEvenIfGatewayLaterReportsTrue() throws Exception { + // Regression guard for the flush-time TOCTOU bug: consent is captured when the evaluation is + // aggregated, not read from the gateway at flush. A bucket aggregated while consent was off + // must stay hashed even if a later RC update turns consent on before the flush drains. + dispatchObserveFullEvaluationData(false); + final BackendApi mockEvp = mock(BackendApi.class); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); + setup.handler.add(piiEvent()); + setup.handler.drainAndAggregate(); // captures consent=false into the bucket + + // Simulate a later RC update flipping consent on; the already-aggregated bucket must not + // follow. + dispatchObserveFullEvaluationData(true); + + final java.util.List captured = new java.util.ArrayList<>(); + when(mockEvp.post(eq("flagevaluation"), any(RequestBody.class), any(), any(), eq(false))) + .thenAnswer( + inv -> { + captured.add(inv.getArgument(1)); + return null; + }); + setup.handler.flush(); + + assertEquals(1, captured.size()); + final FlagEvaluationTestSupport.CapturedJson json = + FlagEvaluationTestSupport.readJson(captured.get(0)); + final Map ev = eventForFlag(json.parsed, "pii-flag"); + assertNotNull(ev); + assertEquals(HASHED_JANE_DOE, ev.get("targeting_key")); + assertFalse(ev.containsKey("context")); + assertTrue(json.raw.contains(HASHED_JANE_DOE)); + assertFalse(json.raw.contains("jane.doe@datadoghq.com")); + } + private void assertHashedTargetingKeyAndOmittedContext() throws Exception { final BackendApi mockEvp = mock(BackendApi.class); final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); From 0d204bd22370f5cacf675f6fae6b0e031a7aa986 Mon Sep 17 00:00:00 2001 From: "vickie.fridge" Date: Fri, 24 Jul 2026 20:06:52 +0000 Subject: [PATCH 05/19] Capture observeFullEvaluationData consent at evaluation time Snapshot the PII consent flag on the evaluation thread (in the OpenFeature hook) and carry it on FlagEvalEvent, instead of reading the gateway when the event is aggregated/flushed. This pins the hashed-vs-raw decision to the configuration active at evaluation time, closing a one-directional leak window where a later Remote Config update could retroactively apply a different environment's consent to already-collected evaluations. Aggregation and flush now read event.observeFullEvaluationData and never consult the gateway; the AND-fold across a bucket's evaluations is unchanged (any no-consent evaluation sinks the bucket to hashed/omitted). Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 --- .../api/openfeature/FlagEvalLoggingHook.java | 8 ++ .../openfeature/FlagEvalLoggingHookTest.java | 54 ++++++++ .../flagevaluation/FlagEvalEvent.java | 49 ++++++- .../flagevaluation/FlagEvalEventTest.java | 20 +++ .../featureflag/FlagEvaluationAggregator.java | 17 ++- .../featureflag/FlagEvaluationWriterImpl.java | 6 +- .../FlagEvaluationAggregatorTest.java | 70 +++++----- .../FlagEvaluationTestSupport.java | 19 +++ .../FlagEvaluationWriterImplTest.java | 121 ++++++++++-------- 9 files changed, 264 insertions(+), 100 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java index 5a3c852a207..e2e5d79e650 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java @@ -109,6 +109,13 @@ public void finallyAfter( ctx != null && ctx.getCtx() != null ? ctx.getCtx().getTargetingKey() : null; final Map attrs = snapshotAttrs(ctx); + // Snapshot the PII consent flag now, on the evaluation thread, so it is pinned to the + // configuration active at evaluation time. The event is drained and flushed later, by which + // point a subsequent RC update may have changed CURRENT_CONFIG; reading consent here (not at + // drain/flush) is what makes the hashed-vs-raw decision faithful to the evaluation. + final boolean observeFullEvaluationData = + FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); + w.enqueue( new FlagEvalEvent( flagKey, @@ -117,6 +124,7 @@ public void finallyAfter( targetingKey, errorMessage, evalTimeMs, + observeFullEvaluationData, () -> extractAttrs(attrs))); } catch (LinkageError | Exception e) { // Never let EVP recording break flag evaluation diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java index b498faf42bc..fe31ceb4d60 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java @@ -16,6 +16,7 @@ import datadog.trace.api.featureflag.FeatureFlaggingGateway; import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; import datadog.trace.api.featureflag.flagevaluation.FlagEvaluationWriter; +import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.FlagEvaluationDetails; import dev.openfeature.sdk.FlagValueType; @@ -49,6 +50,9 @@ void enableFlagEvaluationEnqueue() { @AfterEach void resetFlagEvaluationEnqueue() { FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + // Clear any dispatched UFC so an observeFullEvaluationData value can't leak into other tests + // that share the static gateway. + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); } // ---- helpers ---- @@ -457,4 +461,54 @@ void contextAttributesUseEnqueueTimeSnapshot() { assertFalse(attrs.containsKey("profile.late")); assertFalse(attrs.containsKey("cohorts[1]")); } + + // ---- observeFullEvaluationData is snapshotted from the gateway at evaluation time ---- + + @Test + void snapshotsObserveFullEvaluationDataTrueFromGatewayAtEvaluationTime() { + FeatureFlaggingGateway.dispatch(observeConfig(true)); + try { + assertTrue(enqueuedEvent().observeFullEvaluationData); + } finally { + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + } + } + + @Test + void snapshotsObserveFullEvaluationDataFalseFromGatewayAtEvaluationTime() { + FeatureFlaggingGateway.dispatch(observeConfig(false)); + try { + assertFalse(enqueuedEvent().observeFullEvaluationData); + } finally { + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + } + } + + @Test + void observeFullEvaluationDataDefaultsToFalseWhenNoConfigDispatched() { + // No UFC dispatched: the gateway reports the privacy-preserving default and the hook stamps it. + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + assertFalse(enqueuedEvent().observeFullEvaluationData); + } + + /** Fires the hook once for a simple targeted evaluation and returns the enqueued event. */ + private FlagEvalEvent enqueuedEvent() { + final AtomicReference captured = new AtomicReference<>(); + final FlagEvalLoggingHook hook = hookWithWriter(capturingWriter(captured)); + hook.finallyAfter( + hookCtxWithTargetingKey("obs-flag", "user-1"), + details("obs-flag", "on", "on", Reason.TARGETING_MATCH.name(), null), + Collections.emptyMap()); + assertNotNull(captured.get(), "writer.enqueue must be called once"); + return captured.get(); + } + + private static ServerConfiguration observeConfig(final boolean observeFullEvaluationData) { + return new ServerConfiguration( + "2024-04-17T19:40:53.716Z", + "SERVER", + observeFullEvaluationData, + null, + Collections.emptyMap()); + } } diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java index 83397e5dccd..716b5bd6e3b 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java @@ -50,8 +50,19 @@ public final class FlagEvalEvent { */ public final Map attrs; + /** + * Whether the UFC environment active at evaluation time had {@code + * observeFullEvaluationData} enabled. Snapshotted on the evaluation thread (from {@code + * FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()}) so the consent decision is pinned + * to the instant the flag was evaluated, not to whatever configuration happens to be active when + * the event is later drained and flushed. {@code false} is the privacy-preserving default: when + * off, the targeting key is hashed and the per-evaluation context is omitted on emission. + */ + public final boolean observeFullEvaluationData; + private final Supplier> attrsSupplier; + /** Convenience constructor; consent defaults to the privacy-preserving {@code false}. */ public FlagEvalEvent( final String flagKey, final String variant, @@ -59,9 +70,10 @@ public FlagEvalEvent( final String targetingKey, final long evalTimeMs, final Map attrs) { - this(flagKey, variant, allocationKey, targetingKey, null, evalTimeMs, attrs); + this(flagKey, variant, allocationKey, targetingKey, null, evalTimeMs, false, attrs); } + /** Convenience constructor; consent defaults to the privacy-preserving {@code false}. */ public FlagEvalEvent( final String flagKey, final String variant, @@ -70,16 +82,49 @@ public FlagEvalEvent( final String errorMessage, final long evalTimeMs, final Map attrs) { + this(flagKey, variant, allocationKey, targetingKey, errorMessage, evalTimeMs, false, attrs); + } + + public FlagEvalEvent( + final String flagKey, + final String variant, + final String allocationKey, + final String targetingKey, + final String errorMessage, + final long evalTimeMs, + final boolean observeFullEvaluationData, + final Map attrs) { this.flagKey = flagKey; this.variant = variant; this.allocationKey = allocationKey; this.targetingKey = targetingKey; this.errorMessage = errorMessage; this.evalTimeMs = evalTimeMs; + this.observeFullEvaluationData = observeFullEvaluationData; this.attrs = attrs != null ? attrs : Collections.emptyMap(); this.attrsSupplier = null; } + /** Convenience constructor; consent defaults to the privacy-preserving {@code false}. */ + public FlagEvalEvent( + final String flagKey, + final String variant, + final String allocationKey, + final String targetingKey, + final String errorMessage, + final long evalTimeMs, + final Supplier> attrsSupplier) { + this( + flagKey, + variant, + allocationKey, + targetingKey, + errorMessage, + evalTimeMs, + false, + attrsSupplier); + } + public FlagEvalEvent( final String flagKey, final String variant, @@ -87,6 +132,7 @@ public FlagEvalEvent( final String targetingKey, final String errorMessage, final long evalTimeMs, + final boolean observeFullEvaluationData, final Supplier> attrsSupplier) { this.flagKey = flagKey; this.variant = variant; @@ -94,6 +140,7 @@ public FlagEvalEvent( this.targetingKey = targetingKey; this.errorMessage = errorMessage; this.evalTimeMs = evalTimeMs; + this.observeFullEvaluationData = observeFullEvaluationData; this.attrs = Collections.emptyMap(); this.attrsSupplier = attrsSupplier; } diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEventTest.java b/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEventTest.java index 7faa19c72b3..78b8344502a 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEventTest.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEventTest.java @@ -1,6 +1,7 @@ package datadog.trace.api.featureflag.flagevaluation; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -70,4 +71,23 @@ void defaultsNullLazyContextAttributes() { assertTrue(event.contextAttributes().isEmpty()); } + + @Test + void observeFullEvaluationDataDefaultsToFalseOnConvenienceConstructors() { + final Map attrs = Collections.emptyMap(); + assertFalse(new FlagEvalEvent("f", "on", "a", "t", 1L, attrs).observeFullEvaluationData); + assertFalse(new FlagEvalEvent("f", "on", "a", "t", null, 1L, attrs).observeFullEvaluationData); + assertFalse( + new FlagEvalEvent("f", "on", "a", "t", null, 1L, () -> attrs).observeFullEvaluationData); + } + + @Test + void storesExplicitObserveFullEvaluationData() { + final Map attrs = Collections.emptyMap(); + assertTrue( + new FlagEvalEvent("f", "on", "a", "t", null, 1L, true, attrs).observeFullEvaluationData); + assertTrue( + new FlagEvalEvent("f", "on", "a", "t", null, 1L, true, () -> attrs) + .observeFullEvaluationData); + } } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java index 86b33a4e10c..41e7d10b58f 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java @@ -1,6 +1,5 @@ package com.datadog.featureflag; -import datadog.trace.api.featureflag.FeatureFlaggingGateway; import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; import java.util.HashMap; import java.util.Map; @@ -45,11 +44,12 @@ final class FlagEvaluationAggregator { void aggregate(final FlagEvalEvent event) { final boolean isDefault = event.variant == null; - // Capture consent now, when the evaluation is folded into a bucket, so it reflects the - // configuration active at evaluation time rather than whatever CURRENT_CONFIG happens to be at - // flush. Existing buckets fold with AND: one no-consent evaluation sinks the bucket to hashed. - final boolean observeFullEvaluationData = - FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); + // Consent is read from the event, where it was snapshotted on the evaluation thread at + // evaluation time. We deliberately do NOT read the gateway here: aggregation runs later, on the + // serializer thread, by which point a subsequent RC update may have changed CURRENT_CONFIG, and + // that must not retroactively alter the hashed-vs-raw decision for an already-evaluated flag. + // Existing buckets fold with AND: one no-consent evaluation sinks the bucket to hashed. + final boolean observeFullEvaluationData = event.observeFullEvaluationData; final Map prunedAttrs = pruneContext(event.contextAttributes()); final String ctxKey = canonicalContextKey(prunedAttrs); final FullKey fullKey = buildFullKey(event, ctxKey); @@ -283,9 +283,8 @@ static class EvalBucket { String targetingKey; String errorMessage; Map prunedAttrs; - // Consent to emit raw PII (targeting key + context) captured when this bucket was created, not - // read at flush time. CURRENT_CONFIG can be overwritten by a later RC update between capture - // and flush, so reading the gateway at flush would apply the wrong environment's consent. On + // Consent to emit raw PII (targeting key + context), sourced from each event's evaluation-time + // snapshot (FlagEvalEvent.observeFullEvaluationData), never read from the gateway at flush. On // merge the value is folded with AND (see aggregate()): if any evaluation in the bucket's // lifetime saw consent off, the whole bucket falls back to hashed/omitted — fail-closed. boolean observeFullEvaluationData; diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java index b62c3728cf9..eba631ee955 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java @@ -471,9 +471,9 @@ void flush() { private List buildEventList() { final long flushTimeMs = System.currentTimeMillis(); - // Consent is read per bucket from the value captured at aggregation time, not from the - // gateway here: CURRENT_CONFIG may have been overwritten by a later RC update since these - // evaluations happened, and reading it at flush would apply the wrong environment's consent. + // Consent is read per bucket from the value each event snapshotted at evaluation time, not + // from the gateway here: CURRENT_CONFIG may have been overwritten by a later RC update since + // these evaluations happened, and reading it at flush would apply the wrong config's consent. final List events = new ArrayList<>(aggregator.bucketCount()); for (final FlagEvaluationAggregator.EvalBucket bucket : aggregator.fullBuckets()) { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java index d324ca3012f..2f218eb1e15 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java @@ -6,9 +6,7 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; -import datadog.trace.api.featureflag.FeatureFlaggingGateway; import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; -import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -271,47 +269,49 @@ void degradedKeyEqualityUsesEveryDimension() { @Test void observeFullEvaluationDataFoldsToFalseWhenAnyMergedEvaluationLacksConsent() { final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); - try { - FeatureFlaggingGateway.dispatch(observeConfig(true)); - aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, emptyMap())); - // A later RC update turns consent off; the second evaluation folds into the same bucket. - FeatureFlaggingGateway.dispatch(observeConfig(false)); - aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, emptyMap())); - - final FlagEvaluationAggregator.EvalBucket bucket = - aggregator.snapshot().fullTier.values().iterator().next(); - assertEquals(2, bucket.count); - // Conservative fold: one no-consent evaluation sinks the whole bucket to hashed/omitted. - assertFalse(bucket.observeFullEvaluationData); - } finally { - FeatureFlaggingGateway.dispatch((ServerConfiguration) null); - } + // Consent travels on the event (snapshotted at evaluation time). The first evaluation + // consented; + // a later one - e.g. after an RC update flipped consent off - folds into the same bucket + // carrying consent=false. + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, true, emptyMap())); + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, false, emptyMap())); + + final FlagEvaluationAggregator.EvalBucket bucket = + aggregator.snapshot().fullTier.values().iterator().next(); + assertEquals(2, bucket.count); + // Conservative fold: one no-consent evaluation sinks the whole bucket to hashed/omitted. + assertFalse(bucket.observeFullEvaluationData); } @Test void observeFullEvaluationDataStaysTrueWhenEveryMergedEvaluationConsents() { final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); - try { - FeatureFlaggingGateway.dispatch(observeConfig(true)); - aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, emptyMap())); - aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, emptyMap())); - - final FlagEvaluationAggregator.EvalBucket bucket = - aggregator.snapshot().fullTier.values().iterator().next(); - assertEquals(2, bucket.count); - assertTrue(bucket.observeFullEvaluationData); - } finally { - FeatureFlaggingGateway.dispatch((ServerConfiguration) null); - } + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, true, emptyMap())); + aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, true, emptyMap())); + + final FlagEvaluationAggregator.EvalBucket bucket = + aggregator.snapshot().fullTier.values().iterator().next(); + assertEquals(2, bucket.count); + assertTrue(bucket.observeFullEvaluationData); } - private static ServerConfiguration observeConfig(final boolean observeFullEvaluationData) { - return new ServerConfiguration( - "2024-04-17T19:40:53.716Z", - "SERVER", - observeFullEvaluationData, + private static FlagEvalEvent event( + final String flagKey, + final String variant, + final String allocationKey, + final String targetingKey, + final long evalTimeMs, + final boolean observeFullEvaluationData, + final Map attrs) { + return new FlagEvalEvent( + flagKey, + variant, + allocationKey, + targetingKey, null, - java.util.Collections.emptyMap()); + evalTimeMs, + observeFullEvaluationData, + attrs); } private static FlagEvalEvent event( diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationTestSupport.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationTestSupport.java index 2ec8629dbfc..abb26485c6c 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationTestSupport.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationTestSupport.java @@ -54,6 +54,25 @@ static FlagEvalEvent event( return new FlagEvalEvent(flagKey, variant, allocationKey, targetingKey, evalTimeMs, attrs); } + static FlagEvalEvent event( + final String flagKey, + final String variant, + final String allocationKey, + final String targetingKey, + final long evalTimeMs, + final boolean observeFullEvaluationData, + final Map attrs) { + return new FlagEvalEvent( + flagKey, + variant, + allocationKey, + targetingKey, + null, + evalTimeMs, + observeFullEvaluationData, + attrs); + } + static FlagEvalEvent errorEvent( final String flagKey, final String errorMessage, final long evalTimeMs) { return new FlagEvalEvent( diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index 7d00ec2b3d1..b0fd6a2ce0c 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -543,10 +543,11 @@ void splitPostFailureDoesNotRetryAlreadySentPayloads() throws Exception { @Test void observeFullEvaluationDataTrueEmitsRawTargetingKeyAndContext() throws Exception { - dispatchObserveFullEvaluationData(true); + // Consent travels on the event (snapshotted by the hook at evaluation time); the writer honours + // it verbatim and never consults the gateway. final BackendApi mockEvp = mock(BackendApi.class); final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); - setup.handler.add(piiEvent()); + setup.handler.add(piiEvent(true)); final Map json = flushAndCapture(setup).parsed; @@ -560,64 +561,32 @@ void observeFullEvaluationDataTrueEmitsRawTargetingKeyAndContext() throws Except assertEquals("us-east-1", evalAttrs.get("region")); } - @Test - void observeFullEvaluationDataTrueFromParsedUfcEmitsRawTargetingKey() throws Exception { - // Exercises the config-source -> gateway -> writer seam that the hand-built dispatch skips: - // parse a UFC exactly as the (default) agentless source does, dispatch it, then flush. Guards - // against observeFullEvaluationData being lost between config parsing and the flush-time gate - // read. Field placement mirrors the system-test fixture (after "flags", with format SERVER). - final String attributes = - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":{}," - + "\"observeFullEvaluationData\":true" - + "}"; - final String wrapped = - "{\"data\":{\"type\":\"universal-flag-configuration\",\"attributes\":" + attributes + "}}"; - final ServerConfiguration parsed = - JsonApiUfcResponseParser.INSTANCE.parse( - wrapped.getBytes(java.nio.charset.StandardCharsets.UTF_8)); - assertNotNull(parsed); - assertTrue(parsed.observeFullEvaluationData); - FeatureFlaggingGateway.dispatch(parsed); - - final BackendApi mockEvp = mock(BackendApi.class); - final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); - setup.handler.add(piiEvent()); - - final Map ev = eventForFlag(flushAndCapture(setup).parsed, "pii-flag"); - assertNotNull(ev); - assertEquals("jane.doe@datadoghq.com", ev.get("targeting_key")); - } - @Test void observeFullEvaluationDataFalseHashesTargetingKeyAndOmitsContext() throws Exception { - dispatchObserveFullEvaluationData(false); - assertHashedTargetingKeyAndOmittedContext(); + assertHashedTargetingKeyAndOmittedContext(piiEvent(false)); } @Test - void observeFullEvaluationDataAbsentDefaultsToHashedBehavior() throws Exception { - // No UFC dispatched (default state) — must behave exactly like the explicit "false" case. - assertHashedTargetingKeyAndOmittedContext(); + void flagEvalEventDefaultConsentHashesTargetingKeyAndOmitsContext() throws Exception { + // An event built without an explicit consent value defaults to the privacy-preserving false, so + // it must behave exactly like the explicit "false" case. This is the state the hook produces + // when no UFC has been dispatched (the gateway reports false). + assertHashedTargetingKeyAndOmittedContext(piiEventDefaultConsent()); } @Test - void bucketCapturedUnderFalseStaysHashedEvenIfGatewayLaterReportsTrue() throws Exception { - // Regression guard for the flush-time TOCTOU bug: consent is captured when the evaluation is - // aggregated, not read from the gateway at flush. A bucket aggregated while consent was off - // must stay hashed even if a later RC update turns consent on before the flush drains. - dispatchObserveFullEvaluationData(false); + void eventConsentFalseStaysHashedEvenWhenGatewayLaterReportsTrue() throws Exception { + // Regression guard: consent is decided by the value the event carried at evaluation time, never + // re-read from the gateway at flush. An event evaluated under consent=false must stay hashed + // even if a later RC update turns the gateway's consent on before the flush drains. final BackendApi mockEvp = mock(BackendApi.class); final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); - setup.handler.add(piiEvent()); - setup.handler.drainAndAggregate(); // captures consent=false into the bucket + setup.handler.add(piiEvent(false)); - // Simulate a later RC update flipping consent on; the already-aggregated bucket must not - // follow. + // Flip the gateway's consent on before both aggregation and flush; the event's evaluation-time + // snapshot (false) must win at every downstream step, so neither may consult the gateway. dispatchObserveFullEvaluationData(true); + setup.handler.drainAndAggregate(); final java.util.List captured = new java.util.ArrayList<>(); when(mockEvp.post(eq("flagevaluation"), any(RequestBody.class), any(), any(), eq(false))) @@ -639,10 +608,43 @@ void bucketCapturedUnderFalseStaysHashedEvenIfGatewayLaterReportsTrue() throws E assertFalse(json.raw.contains("jane.doe@datadoghq.com")); } - private void assertHashedTargetingKeyAndOmittedContext() throws Exception { + @Test + void eventConsentTrueStaysRawEvenWhenGatewayLaterReportsFalse() throws Exception { + // Symmetric guard: an event evaluated under consent=true must stay raw even if a later RC + // update + // turns the gateway's consent off before aggregation and flush. Together with the false-stays- + // hashed test this pins that neither aggregation nor flush ever consults the gateway. + final BackendApi mockEvp = mock(BackendApi.class); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); + setup.handler.add(piiEvent(true)); + + dispatchObserveFullEvaluationData(false); + setup.handler.drainAndAggregate(); + + final java.util.List captured = new java.util.ArrayList<>(); + when(mockEvp.post(eq("flagevaluation"), any(RequestBody.class), any(), any(), eq(false))) + .thenAnswer( + inv -> { + captured.add(inv.getArgument(1)); + return null; + }); + setup.handler.flush(); + + assertEquals(1, captured.size()); + final Map ev = + eventForFlag(FlagEvaluationTestSupport.readJson(captured.get(0)).parsed, "pii-flag"); + assertNotNull(ev); + assertEquals("jane.doe@datadoghq.com", ev.get("targeting_key")); + final Map ctx = (Map) ev.get("context"); + assertNotNull(ctx); + assertNotNull(ctx.get("evaluation")); + } + + private void assertHashedTargetingKeyAndOmittedContext(final FlagEvalEvent piiEvent) + throws Exception { final BackendApi mockEvp = mock(BackendApi.class); final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); - setup.handler.add(piiEvent()); + setup.handler.add(piiEvent); final FlagEvaluationTestSupport.CapturedJson captured = flushAndCapture(setup); @@ -658,10 +660,25 @@ private void assertHashedTargetingKeyAndOmittedContext() throws Exception { assertFalse(captured.raw.contains("\"evaluation\":")); } - private static FlagEvalEvent piiEvent() { + private static FlagEvalEvent piiEvent(final boolean observeFullEvaluationData) { + return event( + "pii-flag", + "on", + "alloc1", + "jane.doe@datadoghq.com", + 1000L, + observeFullEvaluationData, + piiAttrs()); + } + + private static FlagEvalEvent piiEventDefaultConsent() { + return event("pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", 1000L, piiAttrs()); + } + + private static Map piiAttrs() { final Map attrs = new HashMap<>(); attrs.put("region", "us-east-1"); - return event("pii-flag", "on", "alloc1", "jane.doe@datadoghq.com", 1000L, attrs); + return attrs; } private static void dispatchObserveFullEvaluationData(final boolean value) { From 933cf3cd7163989fe2f63dc4813a775c69a0a3cf Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Wed, 29 Jul 2026 12:22:06 -0400 Subject: [PATCH 06/19] Bind observeFullEvaluationData consent to the evaluator's configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #12042 review feedback (Codex P1, leoromanovsky, dd-oleksii): the FlagEvalLoggingHook was reading observeFullEvaluationData from FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled() at hook-fire time, which races against a Remote Config swap of CURRENT_CONFIG that happens after DDEvaluator.evaluate() captured its own ServerConfiguration reference. That race can retroactively mark an evaluation performed without consent as consented and leak the raw targeting key / context. DDEvaluator now stamps the boolean directly from the ServerConfiguration it used, onto every ProviderEvaluation via ImmutableMetadata under key "dd.observe_full_evaluation_data". The hook reads consent from that metadata and no longer queries the gateway. Missing metadata (PROVIDER_NOT_READY or a non-DD provider) → false, the privacy-preserving default. The gateway's isObserveFullEvaluationDataEnabled() accessor is removed since its only real caller was the hook and re-adding it would re-open the race. Adds regression tests: hook honours consent metadata (true/false/absent) and ignores a gateway value that disagrees; evaluator stamps the correct boolean on the FLAG_NOT_FOUND path and omits metadata when it holds no config. Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trace/api/openfeature/DDEvaluator.java | 93 +++++++++++++------ .../api/openfeature/FlagEvalLoggingHook.java | 16 ++-- .../api/openfeature/DDEvaluatorTest.java | 43 +++++++++ .../openfeature/FlagEvalLoggingHookTest.java | 54 ++++++----- .../featureflag/FeatureFlaggingGateway.java | 10 -- .../flagevaluation/FlagEvalEvent.java | 13 +-- .../FeatureFlaggingGatewayTest.java | 22 ----- 7 files changed, 160 insertions(+), 91 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java index 9646d1b7579..e853e546107 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java @@ -56,6 +56,12 @@ class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { static final String METADATA_SPLIT_SERIAL_ID = "__dd_split_serial_id"; static final String METADATA_DO_LOG = "__dd_do_log"; + // Consent flag pinned to the ServerConfiguration used by this evaluation. Read by + // FlagEvalLoggingHook so the hashed-vs-raw decision follows the config the evaluator actually + // used, closing the race where CURRENT_CONFIG can be swapped between evaluate() and the hook + // firing. Absent metadata → false (fail-closed toward privacy). + static final String METADATA_OBSERVE_FULL_EVALUATION_DATA = "dd.observe_full_evaluation_data"; + // Read once: when off, the __dd_* span-enrichment metadata is not attached to evaluations, so an // enabled provider pays nothing extra unless span enrichment is also enabled. The gate does not // change at runtime, and this class is loaded lazily (well after startup) so config is ready. @@ -104,30 +110,38 @@ public ProviderEvaluation evaluate( final String key, final T defaultValue, final EvaluationContext context) { + // Snapshot the config once for the entire evaluation and thread its observeFullEvaluationData + // through every ProviderEvaluation this call can return. The hook reads the flag from the + // resulting evaluation metadata rather than from FeatureFlaggingGateway, so the hashed-vs-raw + // decision cannot drift if CURRENT_CONFIG is swapped by a Remote Config update while this + // evaluation is in flight. If config is null (PROVIDER_NOT_READY) we return no consent + // metadata; the hook fails closed to hashed/omitted, which is the privacy-preserving default. + final ServerConfiguration config = configuration.get(); try { - final ServerConfiguration config = configuration.get(); if (config == null) { - return error(defaultValue, ErrorCode.PROVIDER_NOT_READY); + return error(defaultValue, ErrorCode.PROVIDER_NOT_READY, (String) null, null); } if (context == null) { - return error(defaultValue, ErrorCode.INVALID_CONTEXT); + return error(defaultValue, ErrorCode.INVALID_CONTEXT, (String) null, config); } final Flag flag = config.flags.get(key); if (flag == null) { - return error(defaultValue, ErrorCode.FLAG_NOT_FOUND); + return error(defaultValue, ErrorCode.FLAG_NOT_FOUND, (String) null, config); } if (!flag.enabled) { return ProviderEvaluation.builder() .value(defaultValue) .reason(Reason.DISABLED.name()) + .flagMetadata(consentMetadata(config)) .build(); } if (flag.allocations == null) { - return error(defaultValue, ErrorCode.GENERAL, "Missing allocations for flag " + key); + return error( + defaultValue, ErrorCode.GENERAL, "Missing allocations for flag " + key, config); } final Date now = new Date(); @@ -148,10 +162,18 @@ public ProviderEvaluation evaluate( for (final Split split : allocation.splits) { if (isEmpty(split.shards)) { return resolveVariant( - target, key, defaultValue, flag, split.variationKey, allocation, split, context); + target, + key, + defaultValue, + flag, + split.variationKey, + allocation, + split, + context, + config); } else { if (targetingKey == null) { - return error(defaultValue, ErrorCode.TARGETING_KEY_MISSING); + return error(defaultValue, ErrorCode.TARGETING_KEY_MISSING, (String) null, config); } // To match a split, subject must match ALL underlying shards boolean allShardsMatch = true; @@ -170,7 +192,8 @@ public ProviderEvaluation evaluate( split.variationKey, allocation, split, - context); + context, + config); } } } @@ -180,33 +203,46 @@ public ProviderEvaluation evaluate( return ProviderEvaluation.builder() .value(defaultValue) .reason(Reason.DEFAULT.name()) + .flagMetadata(consentMetadata(config)) .build(); } catch (final PatternSyntaxException e) { - return error(defaultValue, ErrorCode.PARSE_ERROR, e); + return error(defaultValue, ErrorCode.PARSE_ERROR, e, config); } catch (final NumberFormatException e) { - return error(defaultValue, ErrorCode.TYPE_MISMATCH, e); + return error(defaultValue, ErrorCode.TYPE_MISMATCH, e, config); } catch (final Exception e) { - return error(defaultValue, ErrorCode.GENERAL, e); + return error(defaultValue, ErrorCode.GENERAL, e, config); } } - private static ProviderEvaluation error(final T defaultValue, final ErrorCode code) { - return error(defaultValue, code, (String) null); + private static ImmutableMetadata consentMetadata(final ServerConfiguration config) { + return ImmutableMetadata.builder() + .addBoolean(METADATA_OBSERVE_FULL_EVALUATION_DATA, config.observeFullEvaluationData) + .build(); } private static ProviderEvaluation error( - final T defaultValue, final ErrorCode code, final Throwable cause) { - return error(defaultValue, code, cause == null ? null : cause.getMessage()); + final T defaultValue, + final ErrorCode code, + final Throwable cause, + final ServerConfiguration config) { + return error(defaultValue, code, cause == null ? null : cause.getMessage(), config); } private static ProviderEvaluation error( - final T defaultValue, final ErrorCode code, final String errorMessage) { - return ProviderEvaluation.builder() - .value(defaultValue) - .reason(Reason.ERROR.name()) - .errorCode(code) - .errorMessage(errorMessage) - .build(); + final T defaultValue, + final ErrorCode code, + final String errorMessage, + final ServerConfiguration config) { + final ProviderEvaluation.ProviderEvaluationBuilder builder = + ProviderEvaluation.builder() + .value(defaultValue) + .reason(Reason.ERROR.name()) + .errorCode(code) + .errorMessage(errorMessage); + if (config != null) { + builder.flagMetadata(consentMetadata(config)); + } + return builder.build(); } private static boolean isEmpty(final List list) { @@ -367,7 +403,8 @@ private static ProviderEvaluation resolveVariant( final String variationKey, final Allocation allocation, final Split split, - final EvaluationContext context) { + final EvaluationContext context, + final ServerConfiguration config) { final Variant variant = flag.variations.get(variationKey); if (variant == null) { return ProviderEvaluation.builder() @@ -375,6 +412,7 @@ private static ProviderEvaluation resolveVariant( .reason(Reason.ERROR.name()) .errorCode(ErrorCode.GENERAL) .errorMessage("Variant not found for: " + variationKey) + .flagMetadata(consentMetadata(config)) .build(); } @@ -385,7 +423,8 @@ private static ProviderEvaluation resolveVariant( "Requested type " + target.getSimpleName() + " does not match flag variationType " - + flag.variationType.name()); + + flag.variationType.name(), + config); } final T mappedValue; @@ -400,7 +439,8 @@ private static ProviderEvaluation resolveVariant( + "' value does not match declared type " + flag.variationType.name() + ": " - + e.getMessage()); + + e.getMessage(), + config); } // Stamp eval-time at the resolution point so first/last_evaluation reflect evaluation time, @@ -411,7 +451,8 @@ private static ProviderEvaluation resolveVariant( .addString("flagKey", flag.key) .addString("variationType", flag.variationType.name()) .addString("allocationKey", allocation.key) - .addLong("dd.eval.timestamp_ms", evalTimestampMs); + .addLong("dd.eval.timestamp_ms", evalTimestampMs) + .addBoolean(METADATA_OBSERVE_FULL_EVALUATION_DATA, config.observeFullEvaluationData); // Surface the UFC split's serial id and the allocation's doLog flag for APM span enrichment — // only when span enrichment is on, so a provider without enrichment pays nothing extra. // __dd_split_serial_id is omitted when the split carries no serial id; __dd_do_log is always diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java index e2e5d79e650..56adda1de9a 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java @@ -109,12 +109,16 @@ public void finallyAfter( ctx != null && ctx.getCtx() != null ? ctx.getCtx().getTargetingKey() : null; final Map attrs = snapshotAttrs(ctx); - // Snapshot the PII consent flag now, on the evaluation thread, so it is pinned to the - // configuration active at evaluation time. The event is drained and flushed later, by which - // point a subsequent RC update may have changed CURRENT_CONFIG; reading consent here (not at - // drain/flush) is what makes the hashed-vs-raw decision faithful to the evaluation. - final boolean observeFullEvaluationData = - FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); + // Read the PII consent flag from evaluation metadata stamped by DDEvaluator against the + // exact ServerConfiguration used for this evaluation. This closes the race where reading + // FeatureFlaggingGateway here could see a *later* RC update than the evaluator did. + // Missing metadata (non-DD provider, or PROVIDER_NOT_READY before any config was accepted) + // → false, the privacy-preserving default. + final Boolean consentFromMetadata = + metadata != null + ? metadata.getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA) + : null; + final boolean observeFullEvaluationData = consentFromMetadata != null && consentFromMetadata; w.enqueue( new FlagEvalEvent( diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index bbe4deec453..d5f86ee6c96 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -213,6 +213,49 @@ public void testNoAllocations() { assertThat(details.getErrorCode(), nullValue()); } + // ---- observeFullEvaluationData metadata is pinned to the config held by evaluate() ---- + // Regression guard for the race the hook used to have with FeatureFlaggingGateway: if the + // gateway's CURRENT_CONFIG is swapped after the evaluator captures its ServerConfiguration but + // before the hook fires, the consent value returned to the hook must still reflect the + // configuration the evaluator actually used. + + @Test + public void observeFullEvaluationDataStampedFromEvaluatorConfigOnSuccess() { + final Map flags = new HashMap<>(); + flags.put("null-allocation", new Flag("target", true, null, null, null)); + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + // Consent on in the config the evaluator accepts. We never swap CURRENT_CONFIG on the gateway + // (nor call the gateway from the evaluator) — this asserts DDEvaluator stamps the boolean + // straight from its captured ServerConfiguration. + evaluator.accept(new ServerConfiguration("", "", true, null, flags)); + + final EvaluationContext ctx = new MutableContext("target").setTargetingKey("k"); + final ProviderEvaluation details = + evaluator.evaluate(Integer.class, "unknown-flag", 23, ctx); + + // FLAG_NOT_FOUND path still carries consent metadata from the config in play. + assertThat(details.getErrorCode(), equalTo(ErrorCode.FLAG_NOT_FOUND)); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(true)); + } + + @Test + public void observeFullEvaluationDataOmittedWhenEvaluatorHasNoConfig() { + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + final ProviderEvaluation details = + evaluator.evaluate(Integer.class, "test", 23, mock(EvaluationContext.class)); + assertThat(details.getErrorCode(), equalTo(ErrorCode.PROVIDER_NOT_READY)); + // No config → no consent metadata → hook fails closed to hashed/omitted. + assertThat( + details.getFlagMetadata() == null + || details + .getFlagMetadata() + .getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA) + == null, + equalTo(true)); + } + private static Arguments[] flatteningTestCases() { final List arguments = new ArrayList<>(); arguments.add(Arguments.of(emptyMap(), emptyMap())); diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java index fe31ceb4d60..de0799911a9 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java @@ -462,42 +462,54 @@ void contextAttributesUseEnqueueTimeSnapshot() { assertFalse(attrs.containsKey("cohorts[1]")); } - // ---- observeFullEvaluationData is snapshotted from the gateway at evaluation time ---- + // ---- observeFullEvaluationData is read from evaluation metadata stamped by DDEvaluator ---- + // The hook must not consult FeatureFlaggingGateway for consent: doing so races against a + // Remote Config update that swaps CURRENT_CONFIG between evaluate() and finallyAfter(). @Test - void snapshotsObserveFullEvaluationDataTrueFromGatewayAtEvaluationTime() { - FeatureFlaggingGateway.dispatch(observeConfig(true)); - try { - assertTrue(enqueuedEvent().observeFullEvaluationData); - } finally { - FeatureFlaggingGateway.dispatch((ServerConfiguration) null); - } + void readsObserveFullEvaluationDataTrueFromEvaluationMetadata() { + assertTrue(enqueuedEventWithConsentMetadata(true).observeFullEvaluationData); + } + + @Test + void readsObserveFullEvaluationDataFalseFromEvaluationMetadata() { + assertFalse(enqueuedEventWithConsentMetadata(false).observeFullEvaluationData); + } + + @Test + void observeFullEvaluationDataDefaultsToFalseWhenMetadataAbsent() { + // No metadata at all: fail-closed toward privacy. + assertFalse(enqueuedEventWithConsentMetadata(null).observeFullEvaluationData); } @Test - void snapshotsObserveFullEvaluationDataFalseFromGatewayAtEvaluationTime() { - FeatureFlaggingGateway.dispatch(observeConfig(false)); + void ignoresGatewayConsentEvenWhenItDisagreesWithMetadata() { + // Gateway says "consent on" (a *later* RC update after the evaluation ran); metadata pins + // the evaluator's original view of "consent off". The hook must trust metadata. + FeatureFlaggingGateway.dispatch(observeConfig(true)); try { - assertFalse(enqueuedEvent().observeFullEvaluationData); + assertFalse(enqueuedEventWithConsentMetadata(false).observeFullEvaluationData); } finally { FeatureFlaggingGateway.dispatch((ServerConfiguration) null); } } - @Test - void observeFullEvaluationDataDefaultsToFalseWhenNoConfigDispatched() { - // No UFC dispatched: the gateway reports the privacy-preserving default and the hook stamps it. - FeatureFlaggingGateway.dispatch((ServerConfiguration) null); - assertFalse(enqueuedEvent().observeFullEvaluationData); - } - - /** Fires the hook once for a simple targeted evaluation and returns the enqueued event. */ - private FlagEvalEvent enqueuedEvent() { + /** + * Fires the hook once for a simple targeted evaluation whose metadata carries the given consent + * value ({@code null} = key absent) and returns the enqueued event. + */ + private FlagEvalEvent enqueuedEventWithConsentMetadata(final Boolean consent) { final AtomicReference captured = new AtomicReference<>(); final FlagEvalLoggingHook hook = hookWithWriter(capturingWriter(captured)); + final ImmutableMetadata metadata = + consent == null + ? null + : ImmutableMetadata.builder() + .addBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA, consent) + .build(); hook.finallyAfter( hookCtxWithTargetingKey("obs-flag", "user-1"), - details("obs-flag", "on", "on", Reason.TARGETING_MATCH.name(), null), + details("obs-flag", "on", "on", Reason.TARGETING_MATCH.name(), metadata), Collections.emptyMap()); assertNotNull(captured.get(), "writer.enqueue must be called once"); return captured.get(); diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java index 623c6d2c1b8..2a823bd32ef 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/FeatureFlaggingGateway.java @@ -117,16 +117,6 @@ public static boolean isFlagEvaluationEnqueueEnabled() { return flagEvalEnqueueEnabled; } - /** - * Returns whether the currently active UFC environment has {@code observeFullEvaluationData} - * enabled. {@code false} (privacy-preserving default) when no UFC has been dispatched yet or when - * the field was absent/false on the last dispatched configuration. - */ - public static boolean isObserveFullEvaluationDataEnabled() { - final ServerConfiguration current = CURRENT_CONFIG.get(); - return current != null && current.observeFullEvaluationData; - } - public static void addSpanEnrichmentListener(final SpanEnrichmentListener listener) { SPAN_ENRICHMENT_LISTENERS.add(listener); } diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java index 716b5bd6e3b..a9209ffe799 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java @@ -51,12 +51,13 @@ public final class FlagEvalEvent { public final Map attrs; /** - * Whether the UFC environment active at evaluation time had {@code - * observeFullEvaluationData} enabled. Snapshotted on the evaluation thread (from {@code - * FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()}) so the consent decision is pinned - * to the instant the flag was evaluated, not to whatever configuration happens to be active when - * the event is later drained and flushed. {@code false} is the privacy-preserving default: when - * off, the targeting key is hashed and the per-evaluation context is omitted on emission. + * Whether the {@code ServerConfiguration} the evaluator actually used for this evaluation had + * {@code observeFullEvaluationData} enabled. Read by the hook from evaluation metadata stamped by + * {@code DDEvaluator} against that exact configuration, so the consent decision follows the + * evaluator rather than whatever configuration happens to be active later (either at hook-fire + * time or when the event is drained and flushed). {@code false} is the privacy-preserving + * default: when off, the targeting key is hashed and the per-evaluation context is omitted on + * emission. */ public final boolean observeFullEvaluationData; diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java b/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java index ecaf7330ef2..887a153f0a1 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/test/java/datadog/trace/api/featureflag/FeatureFlaggingGatewayTest.java @@ -1,14 +1,11 @@ package datadog.trace.api.featureflag; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import datadog.trace.api.featureflag.exposure.ExposureEvent; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; -import java.util.Collections; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -112,25 +109,6 @@ void testAttachingASpanEnrichmentListener() { verifyNoMoreInteractions(spanEnrichmentListener); } - @Test - void isObserveFullEvaluationDataEnabledDefaultsToFalseWithNoConfig() { - FeatureFlaggingGateway.dispatch((ServerConfiguration) null); - assertFalse(FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()); - } - - @Test - void isObserveFullEvaluationDataEnabledReflectsLastDispatchedConfig() { - final ServerConfiguration enabled = - new ServerConfiguration("", "", true, null, Collections.emptyMap()); - FeatureFlaggingGateway.dispatch(enabled); - assertTrue(FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()); - - final ServerConfiguration disabled = - new ServerConfiguration("", "", false, null, Collections.emptyMap()); - FeatureFlaggingGateway.dispatch(disabled); - assertFalse(FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()); - } - private static void clearCurrentServerConfiguration() { FeatureFlaggingGateway.dispatch((ServerConfiguration) null); } From 98df3baa45b321338fd0d8684c01bc27f2c2e3b9 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Wed, 29 Jul 2026 12:49:36 -0400 Subject: [PATCH 07/19] Pass the boolean, not the ServerConfiguration, into error/resolveVariant Follow-up to the previous commit: the private error() and resolveVariant() helpers only ever read one field off the ServerConfiguration (observeFullEvaluationData), so pass the boolean directly instead of the whole config. Keeps the internal API narrow and removes the incidental coupling these helpers had to the UFC. While here, PROVIDER_NOT_READY now stamps consent as the privacy-preserving false rather than omitting the metadata. Same on-the-wire outcome the hook would have produced, but the invariant "every DD-produced evaluation carries dd.observe_full_evaluation_data" is now unconditional, which is easier to reason about. The two error() overloads collapse to one (the (String) null casts at call sites disappear along with them). Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trace/api/openfeature/DDEvaluator.java | 78 +++++++++---------- .../api/openfeature/FlagEvalLoggingHook.java | 4 +- .../api/openfeature/DDEvaluatorTest.java | 12 +-- 3 files changed, 43 insertions(+), 51 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java index e853e546107..3d62952d4e3 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java @@ -59,7 +59,8 @@ class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { // Consent flag pinned to the ServerConfiguration used by this evaluation. Read by // FlagEvalLoggingHook so the hashed-vs-raw decision follows the config the evaluator actually // used, closing the race where CURRENT_CONFIG can be swapped between evaluate() and the hook - // firing. Absent metadata → false (fail-closed toward privacy). + // firing. Stamped on every DD-produced evaluation (including PROVIDER_NOT_READY, with false); + // a missing key indicates a non-DD provider and the hook falls back to false (fail-closed). static final String METADATA_OBSERVE_FULL_EVALUATION_DATA = "dd.observe_full_evaluation_data"; // Read once: when off, the __dd_* span-enrichment metadata is not attached to evaluations, so an @@ -114,34 +115,38 @@ public ProviderEvaluation evaluate( // through every ProviderEvaluation this call can return. The hook reads the flag from the // resulting evaluation metadata rather than from FeatureFlaggingGateway, so the hashed-vs-raw // decision cannot drift if CURRENT_CONFIG is swapped by a Remote Config update while this - // evaluation is in flight. If config is null (PROVIDER_NOT_READY) we return no consent - // metadata; the hook fails closed to hashed/omitted, which is the privacy-preserving default. + // evaluation is in flight. If config is null (PROVIDER_NOT_READY) we default to the + // privacy-preserving false — same wire outcome the hook would produce from missing metadata. final ServerConfiguration config = configuration.get(); + final boolean observeFullEvaluationData = config != null && config.observeFullEvaluationData; try { if (config == null) { - return error(defaultValue, ErrorCode.PROVIDER_NOT_READY, (String) null, null); + return error(defaultValue, ErrorCode.PROVIDER_NOT_READY, null, observeFullEvaluationData); } if (context == null) { - return error(defaultValue, ErrorCode.INVALID_CONTEXT, (String) null, config); + return error(defaultValue, ErrorCode.INVALID_CONTEXT, null, observeFullEvaluationData); } final Flag flag = config.flags.get(key); if (flag == null) { - return error(defaultValue, ErrorCode.FLAG_NOT_FOUND, (String) null, config); + return error(defaultValue, ErrorCode.FLAG_NOT_FOUND, null, observeFullEvaluationData); } if (!flag.enabled) { return ProviderEvaluation.builder() .value(defaultValue) .reason(Reason.DISABLED.name()) - .flagMetadata(consentMetadata(config)) + .flagMetadata(consentMetadata(observeFullEvaluationData)) .build(); } if (flag.allocations == null) { return error( - defaultValue, ErrorCode.GENERAL, "Missing allocations for flag " + key, config); + defaultValue, + ErrorCode.GENERAL, + "Missing allocations for flag " + key, + observeFullEvaluationData); } final Date now = new Date(); @@ -170,10 +175,11 @@ public ProviderEvaluation evaluate( allocation, split, context, - config); + observeFullEvaluationData); } else { if (targetingKey == null) { - return error(defaultValue, ErrorCode.TARGETING_KEY_MISSING, (String) null, config); + return error( + defaultValue, ErrorCode.TARGETING_KEY_MISSING, null, observeFullEvaluationData); } // To match a split, subject must match ALL underlying shards boolean allShardsMatch = true; @@ -193,7 +199,7 @@ public ProviderEvaluation evaluate( allocation, split, context, - config); + observeFullEvaluationData); } } } @@ -203,46 +209,36 @@ public ProviderEvaluation evaluate( return ProviderEvaluation.builder() .value(defaultValue) .reason(Reason.DEFAULT.name()) - .flagMetadata(consentMetadata(config)) + .flagMetadata(consentMetadata(observeFullEvaluationData)) .build(); } catch (final PatternSyntaxException e) { - return error(defaultValue, ErrorCode.PARSE_ERROR, e, config); + return error(defaultValue, ErrorCode.PARSE_ERROR, e.getMessage(), observeFullEvaluationData); } catch (final NumberFormatException e) { - return error(defaultValue, ErrorCode.TYPE_MISMATCH, e, config); + return error( + defaultValue, ErrorCode.TYPE_MISMATCH, e.getMessage(), observeFullEvaluationData); } catch (final Exception e) { - return error(defaultValue, ErrorCode.GENERAL, e, config); + return error(defaultValue, ErrorCode.GENERAL, e.getMessage(), observeFullEvaluationData); } } - private static ImmutableMetadata consentMetadata(final ServerConfiguration config) { + private static ImmutableMetadata consentMetadata(final boolean observeFullEvaluationData) { return ImmutableMetadata.builder() - .addBoolean(METADATA_OBSERVE_FULL_EVALUATION_DATA, config.observeFullEvaluationData) + .addBoolean(METADATA_OBSERVE_FULL_EVALUATION_DATA, observeFullEvaluationData) .build(); } - private static ProviderEvaluation error( - final T defaultValue, - final ErrorCode code, - final Throwable cause, - final ServerConfiguration config) { - return error(defaultValue, code, cause == null ? null : cause.getMessage(), config); - } - private static ProviderEvaluation error( final T defaultValue, final ErrorCode code, final String errorMessage, - final ServerConfiguration config) { - final ProviderEvaluation.ProviderEvaluationBuilder builder = - ProviderEvaluation.builder() - .value(defaultValue) - .reason(Reason.ERROR.name()) - .errorCode(code) - .errorMessage(errorMessage); - if (config != null) { - builder.flagMetadata(consentMetadata(config)); - } - return builder.build(); + final boolean observeFullEvaluationData) { + return ProviderEvaluation.builder() + .value(defaultValue) + .reason(Reason.ERROR.name()) + .errorCode(code) + .errorMessage(errorMessage) + .flagMetadata(consentMetadata(observeFullEvaluationData)) + .build(); } private static boolean isEmpty(final List list) { @@ -404,7 +400,7 @@ private static ProviderEvaluation resolveVariant( final Allocation allocation, final Split split, final EvaluationContext context, - final ServerConfiguration config) { + final boolean observeFullEvaluationData) { final Variant variant = flag.variations.get(variationKey); if (variant == null) { return ProviderEvaluation.builder() @@ -412,7 +408,7 @@ private static ProviderEvaluation resolveVariant( .reason(Reason.ERROR.name()) .errorCode(ErrorCode.GENERAL) .errorMessage("Variant not found for: " + variationKey) - .flagMetadata(consentMetadata(config)) + .flagMetadata(consentMetadata(observeFullEvaluationData)) .build(); } @@ -424,7 +420,7 @@ private static ProviderEvaluation resolveVariant( + target.getSimpleName() + " does not match flag variationType " + flag.variationType.name(), - config); + observeFullEvaluationData); } final T mappedValue; @@ -440,7 +436,7 @@ private static ProviderEvaluation resolveVariant( + flag.variationType.name() + ": " + e.getMessage(), - config); + observeFullEvaluationData); } // Stamp eval-time at the resolution point so first/last_evaluation reflect evaluation time, @@ -452,7 +448,7 @@ private static ProviderEvaluation resolveVariant( .addString("variationType", flag.variationType.name()) .addString("allocationKey", allocation.key) .addLong("dd.eval.timestamp_ms", evalTimestampMs) - .addBoolean(METADATA_OBSERVE_FULL_EVALUATION_DATA, config.observeFullEvaluationData); + .addBoolean(METADATA_OBSERVE_FULL_EVALUATION_DATA, observeFullEvaluationData); // Surface the UFC split's serial id and the allocation's doLog flag for APM span enrichment — // only when span enrichment is on, so a provider without enrichment pays nothing extra. // __dd_split_serial_id is omitted when the split carries no serial id; __dd_do_log is always diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java index 56adda1de9a..ddb2d039969 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java @@ -112,8 +112,8 @@ public void finallyAfter( // Read the PII consent flag from evaluation metadata stamped by DDEvaluator against the // exact ServerConfiguration used for this evaluation. This closes the race where reading // FeatureFlaggingGateway here could see a *later* RC update than the evaluator did. - // Missing metadata (non-DD provider, or PROVIDER_NOT_READY before any config was accepted) - // → false, the privacy-preserving default. + // Missing key (non-DD provider) → false, the privacy-preserving default. DD-produced + // evaluations always stamp the key, including PROVIDER_NOT_READY (with false). final Boolean consentFromMetadata = metadata != null ? metadata.getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA) diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index d5f86ee6c96..92a981aaf6b 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -241,19 +241,15 @@ public void observeFullEvaluationDataStampedFromEvaluatorConfigOnSuccess() { } @Test - public void observeFullEvaluationDataOmittedWhenEvaluatorHasNoConfig() { + public void observeFullEvaluationDataDefaultsToFalseWhenEvaluatorHasNoConfig() { final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); final ProviderEvaluation details = evaluator.evaluate(Integer.class, "test", 23, mock(EvaluationContext.class)); assertThat(details.getErrorCode(), equalTo(ErrorCode.PROVIDER_NOT_READY)); - // No config → no consent metadata → hook fails closed to hashed/omitted. + // No config → consent stamped as the privacy-preserving false. assertThat( - details.getFlagMetadata() == null - || details - .getFlagMetadata() - .getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA) - == null, - equalTo(true)); + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(false)); } private static Arguments[] flatteningTestCases() { From b6d9d60253e46a02a67ec13dd16baaba47b7f3b5 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Wed, 29 Jul 2026 12:50:58 -0400 Subject: [PATCH 08/19] Drop the dd. prefix on the evaluation-metadata consent key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The key is only ever read by FlagEvalLoggingHook one line later — it never lands on the wire, so it doesn't need the "dd." namespacing that "dd.eval.timestamp_ms" has (that key is re-emitted onto spans). Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../main/java/datadog/trace/api/openfeature/DDEvaluator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java index 3d62952d4e3..87bd35faa34 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java @@ -61,7 +61,7 @@ class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { // used, closing the race where CURRENT_CONFIG can be swapped between evaluate() and the hook // firing. Stamped on every DD-produced evaluation (including PROVIDER_NOT_READY, with false); // a missing key indicates a non-DD provider and the hook falls back to false (fail-closed). - static final String METADATA_OBSERVE_FULL_EVALUATION_DATA = "dd.observe_full_evaluation_data"; + static final String METADATA_OBSERVE_FULL_EVALUATION_DATA = "observe_full_evaluation_data"; // Read once: when off, the __dd_* span-enrichment metadata is not attached to evaluations, so an // enabled provider pays nothing extra unless span enrichment is also enabled. The gate does not From cb7609249c4c6ddcaedbda4c3309025b110ba931 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Thu, 30 Jul 2026 12:25:58 -0400 Subject: [PATCH 09/19] Trim verbose comments around the observeFullEvaluationData plumbing The race-vs-CURRENT_CONFIG backstory is captured in the previous commits' messages; the code only needs the forward-looking invariants (metadata is source of truth, missing key = false, DD-produced evaluations always stamp). Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trace/api/openfeature/DDEvaluator.java | 16 +++++----------- .../api/openfeature/FlagEvalLoggingHook.java | 7 ++----- .../trace/api/openfeature/DDEvaluatorTest.java | 12 ++---------- .../api/openfeature/FlagEvalLoggingHookTest.java | 7 ++----- .../flagevaluation/FlagEvalEvent.java | 10 +++------- 5 files changed, 14 insertions(+), 38 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java index 87bd35faa34..c25eff9eae3 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java @@ -56,11 +56,8 @@ class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { static final String METADATA_SPLIT_SERIAL_ID = "__dd_split_serial_id"; static final String METADATA_DO_LOG = "__dd_do_log"; - // Consent flag pinned to the ServerConfiguration used by this evaluation. Read by - // FlagEvalLoggingHook so the hashed-vs-raw decision follows the config the evaluator actually - // used, closing the race where CURRENT_CONFIG can be swapped between evaluate() and the hook - // firing. Stamped on every DD-produced evaluation (including PROVIDER_NOT_READY, with false); - // a missing key indicates a non-DD provider and the hook falls back to false (fail-closed). + // Stamped on every DD-produced evaluation (including PROVIDER_NOT_READY, with false). Missing + // key = non-DD provider; the hook falls back to false (fail-closed). static final String METADATA_OBSERVE_FULL_EVALUATION_DATA = "observe_full_evaluation_data"; // Read once: when off, the __dd_* span-enrichment metadata is not attached to evaluations, so an @@ -111,12 +108,9 @@ public ProviderEvaluation evaluate( final String key, final T defaultValue, final EvaluationContext context) { - // Snapshot the config once for the entire evaluation and thread its observeFullEvaluationData - // through every ProviderEvaluation this call can return. The hook reads the flag from the - // resulting evaluation metadata rather than from FeatureFlaggingGateway, so the hashed-vs-raw - // decision cannot drift if CURRENT_CONFIG is swapped by a Remote Config update while this - // evaluation is in flight. If config is null (PROVIDER_NOT_READY) we default to the - // privacy-preserving false — same wire outcome the hook would produce from missing metadata. + // Snapshot the config once and thread observeFullEvaluationData through every + // ProviderEvaluation returned, so the hook's consent decision is pinned to this evaluation's + // config and cannot drift on a concurrent Remote Config swap. final ServerConfiguration config = configuration.get(); final boolean observeFullEvaluationData = config != null && config.observeFullEvaluationData; try { diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java index ddb2d039969..6595838e770 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java @@ -109,11 +109,8 @@ public void finallyAfter( ctx != null && ctx.getCtx() != null ? ctx.getCtx().getTargetingKey() : null; final Map attrs = snapshotAttrs(ctx); - // Read the PII consent flag from evaluation metadata stamped by DDEvaluator against the - // exact ServerConfiguration used for this evaluation. This closes the race where reading - // FeatureFlaggingGateway here could see a *later* RC update than the evaluator did. - // Missing key (non-DD provider) → false, the privacy-preserving default. DD-produced - // evaluations always stamp the key, including PROVIDER_NOT_READY (with false). + // Consent is read from metadata stamped by DDEvaluator (pinned to its ServerConfiguration). + // Missing key = non-DD provider → false, the privacy-preserving default. final Boolean consentFromMetadata = metadata != null ? metadata.getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA) diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index 92a981aaf6b..0ad3f12cf9c 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -213,27 +213,20 @@ public void testNoAllocations() { assertThat(details.getErrorCode(), nullValue()); } - // ---- observeFullEvaluationData metadata is pinned to the config held by evaluate() ---- - // Regression guard for the race the hook used to have with FeatureFlaggingGateway: if the - // gateway's CURRENT_CONFIG is swapped after the evaluator captures its ServerConfiguration but - // before the hook fires, the consent value returned to the hook must still reflect the - // configuration the evaluator actually used. + // ---- observeFullEvaluationData metadata is stamped from the evaluator's ServerConfiguration + // ---- @Test public void observeFullEvaluationDataStampedFromEvaluatorConfigOnSuccess() { final Map flags = new HashMap<>(); flags.put("null-allocation", new Flag("target", true, null, null, null)); final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); - // Consent on in the config the evaluator accepts. We never swap CURRENT_CONFIG on the gateway - // (nor call the gateway from the evaluator) — this asserts DDEvaluator stamps the boolean - // straight from its captured ServerConfiguration. evaluator.accept(new ServerConfiguration("", "", true, null, flags)); final EvaluationContext ctx = new MutableContext("target").setTargetingKey("k"); final ProviderEvaluation details = evaluator.evaluate(Integer.class, "unknown-flag", 23, ctx); - // FLAG_NOT_FOUND path still carries consent metadata from the config in play. assertThat(details.getErrorCode(), equalTo(ErrorCode.FLAG_NOT_FOUND)); assertThat( details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), @@ -246,7 +239,6 @@ public void observeFullEvaluationDataDefaultsToFalseWhenEvaluatorHasNoConfig() { final ProviderEvaluation details = evaluator.evaluate(Integer.class, "test", 23, mock(EvaluationContext.class)); assertThat(details.getErrorCode(), equalTo(ErrorCode.PROVIDER_NOT_READY)); - // No config → consent stamped as the privacy-preserving false. assertThat( details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), equalTo(false)); diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java index de0799911a9..cc1d7ec64e7 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java @@ -462,9 +462,7 @@ void contextAttributesUseEnqueueTimeSnapshot() { assertFalse(attrs.containsKey("cohorts[1]")); } - // ---- observeFullEvaluationData is read from evaluation metadata stamped by DDEvaluator ---- - // The hook must not consult FeatureFlaggingGateway for consent: doing so races against a - // Remote Config update that swaps CURRENT_CONFIG between evaluate() and finallyAfter(). + // ---- observeFullEvaluationData is read from evaluation metadata, never the gateway ---- @Test void readsObserveFullEvaluationDataTrueFromEvaluationMetadata() { @@ -484,8 +482,7 @@ void observeFullEvaluationDataDefaultsToFalseWhenMetadataAbsent() { @Test void ignoresGatewayConsentEvenWhenItDisagreesWithMetadata() { - // Gateway says "consent on" (a *later* RC update after the evaluation ran); metadata pins - // the evaluator's original view of "consent off". The hook must trust metadata. + // Gateway says on, metadata says off; hook must trust metadata. FeatureFlaggingGateway.dispatch(observeConfig(true)); try { assertFalse(enqueuedEventWithConsentMetadata(false).observeFullEvaluationData); diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java index a9209ffe799..68a7b2c7ace 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/flagevaluation/FlagEvalEvent.java @@ -51,13 +51,9 @@ public final class FlagEvalEvent { public final Map attrs; /** - * Whether the {@code ServerConfiguration} the evaluator actually used for this evaluation had - * {@code observeFullEvaluationData} enabled. Read by the hook from evaluation metadata stamped by - * {@code DDEvaluator} against that exact configuration, so the consent decision follows the - * evaluator rather than whatever configuration happens to be active later (either at hook-fire - * time or when the event is drained and flushed). {@code false} is the privacy-preserving - * default: when off, the targeting key is hashed and the per-evaluation context is omitted on - * emission. + * PII consent from the {@code ServerConfiguration} used by the evaluation. When {@code false} + * (privacy-preserving default), the targeting key is hashed and the per-evaluation context is + * omitted on emission. */ public final boolean observeFullEvaluationData; From 9e5253b9bf0c223eb79ee958d43e974969cb344a Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Thu, 30 Jul 2026 12:35:00 -0400 Subject: [PATCH 10/19] Skip evaluation context when aggregating consent-off evaluations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #12042 review from leoromanovsky (escalated Codex P2 → P1): on the protected path (observeFullEvaluationData=false) the serializer drops the evaluation context, but the aggregator was still running it through pruneContext + canonicalContextKey and keying every full-tier bucket on it. A high-cardinality field on the evaluation context (request_id, timestamp, correlation id) would fragment buckets that emit byte-identical wire rows, blow out PER_FLAG_CAP (10k) inside one flush window, and force subsequent evaluations into the degraded tier — which drops the targeting key entirely. On the protected path aggregate() now uses ctxKey="" and stores prunedAttrs=null, so different contexts for the same subject collapse into one bucket. The targeting key stays in the aggregation identity, so different subjects still hash to different buckets. The consent-on path is unchanged. Regression tests: protected path collapses differing contexts for one subject; protected path still separates distinct subjects; full path still splits on context. Existing tests that exercise pruneContext / context-differentiation were updated to use consent=on (that's the code path they actually cover). Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../featureflag/FlagEvaluationAggregator.java | 19 +++--- .../FlagEvaluationAggregatorTest.java | 67 ++++++++++++++++--- .../FlagEvaluationWriterImplTest.java | 1 + 3 files changed, 67 insertions(+), 20 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java index 41e7d10b58f..855704dca9b 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java @@ -44,14 +44,13 @@ final class FlagEvaluationAggregator { void aggregate(final FlagEvalEvent event) { final boolean isDefault = event.variant == null; - // Consent is read from the event, where it was snapshotted on the evaluation thread at - // evaluation time. We deliberately do NOT read the gateway here: aggregation runs later, on the - // serializer thread, by which point a subsequent RC update may have changed CURRENT_CONFIG, and - // that must not retroactively alter the hashed-vs-raw decision for an already-evaluated flag. - // Existing buckets fold with AND: one no-consent evaluation sinks the bucket to hashed. final boolean observeFullEvaluationData = event.observeFullEvaluationData; - final Map prunedAttrs = pruneContext(event.contextAttributes()); - final String ctxKey = canonicalContextKey(prunedAttrs); + // On the protected path the context is dropped on emit, so it must not fragment buckets or be + // stored — otherwise a high-cardinality field (request_id, timestamp) blows out PER_FLAG_CAP + // and spills every subsequent evaluation into the degraded tier. + final Map prunedAttrs = + observeFullEvaluationData ? pruneContext(event.contextAttributes()) : null; + final String ctxKey = observeFullEvaluationData ? canonicalContextKey(prunedAttrs) : ""; final FullKey fullKey = buildFullKey(event, ctxKey); EvalBucket bucket = fullTier.get(fullKey); @@ -283,10 +282,8 @@ static class EvalBucket { String targetingKey; String errorMessage; Map prunedAttrs; - // Consent to emit raw PII (targeting key + context), sourced from each event's evaluation-time - // snapshot (FlagEvalEvent.observeFullEvaluationData), never read from the gateway at flush. On - // merge the value is folded with AND (see aggregate()): if any evaluation in the bucket's - // lifetime saw consent off, the whole bucket falls back to hashed/omitted — fail-closed. + // Consent to emit raw PII, taken from each event's evaluation-time snapshot. Folded with AND + // on merge: any consent-off evaluation sinks the bucket to hashed/omitted (fail-closed). boolean observeFullEvaluationData; EvalBucket( diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java index 2f218eb1e15..07d4eb8996c 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java @@ -38,8 +38,8 @@ void differentValueTypesProduceDifferentBuckets() { final Map attrsStr = new HashMap<>(); attrsStr.put("score", "1"); - aggregator.aggregate(event("flag-b", "on", "alloc1", "user-1", 1000L, attrsInt)); - aggregator.aggregate(event("flag-b", "on", "alloc1", "user-1", 1000L, attrsStr)); + aggregator.aggregate(event("flag-b", "on", "alloc1", "user-1", 1000L, true, attrsInt)); + aggregator.aggregate(event("flag-b", "on", "alloc1", "user-1", 1000L, true, attrsStr)); final FlagEvaluationAggregator.AggregatedState state = aggregator.snapshot(); assertEquals(2, state.fullTier.size()); @@ -118,7 +118,7 @@ void contextExceeding256FieldsIsPrunedToStoredPrunedAttrs() { hugeAttrs.put("key" + i, "v" + i); } - aggregator.aggregate(event("flag-d", "on", "alloc1", "user-1", 1000L, hugeAttrs)); + aggregator.aggregate(event("flag-d", "on", "alloc1", "user-1", 1000L, true, hugeAttrs)); final FlagEvaluationAggregator.AggregatedState state = aggregator.snapshot(); final FlagEvaluationAggregator.EvalBucket bucket = state.fullTier.values().iterator().next(); @@ -179,7 +179,7 @@ void contextValueExceeding256CharsIsSkippedFromPrunedAttrs() { attrs.put("long-val", repeat('x', 300)); attrs.put("short-val", "ok"); - aggregator.aggregate(event("flag-e", "on", "alloc1", "user-1", 1000L, attrs)); + aggregator.aggregate(event("flag-e", "on", "alloc1", "user-1", 1000L, true, attrs)); final FlagEvaluationAggregator.AggregatedState state = aggregator.snapshot(); final FlagEvaluationAggregator.EvalBucket bucket = state.fullTier.values().iterator().next(); @@ -269,17 +269,13 @@ void degradedKeyEqualityUsesEveryDimension() { @Test void observeFullEvaluationDataFoldsToFalseWhenAnyMergedEvaluationLacksConsent() { final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); - // Consent travels on the event (snapshotted at evaluation time). The first evaluation - // consented; - // a later one - e.g. after an RC update flipped consent off - folds into the same bucket - // carrying consent=false. + // First event consents; a later one (e.g. after RC flipped consent off) folds into the bucket. aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, true, emptyMap())); aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, false, emptyMap())); final FlagEvaluationAggregator.EvalBucket bucket = aggregator.snapshot().fullTier.values().iterator().next(); assertEquals(2, bucket.count); - // Conservative fold: one no-consent evaluation sinks the whole bucket to hashed/omitted. assertFalse(bucket.observeFullEvaluationData); } @@ -295,6 +291,59 @@ void observeFullEvaluationDataStaysTrueWhenEveryMergedEvaluationConsents() { assertTrue(bucket.observeFullEvaluationData); } + @Test + void protectedPathCollapsesDifferingContextIntoOneBucket() { + // Same subject, different request-id contexts, consent off: the context is dropped on emit so + // it must not fragment full-tier buckets or the per-flag cap blows out under real traffic. + final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); + final Map ctx1 = new HashMap<>(); + ctx1.put("request_id", "req-1"); + final Map ctx2 = new HashMap<>(); + ctx2.put("request_id", "req-2"); + final Map ctx3 = new HashMap<>(); + ctx3.put("request_id", "req-3"); + + aggregator.aggregate(event("checkout", "on", "alloc1", "alice", 1000L, false, ctx1)); + aggregator.aggregate(event("checkout", "on", "alloc1", "alice", 2000L, false, ctx2)); + aggregator.aggregate(event("checkout", "on", "alloc1", "alice", 3000L, false, ctx3)); + + assertEquals(1, aggregator.fullTierSize()); + final FlagEvaluationAggregator.EvalBucket bucket = + aggregator.snapshot().fullTier.values().iterator().next(); + assertEquals(3, bucket.count); + assertFalse(bucket.observeFullEvaluationData); + assertEquals(0, bucket.prunedContextFieldCount()); + } + + @Test + void protectedPathSeparatesDifferentSubjects() { + // Different targeting keys must still fall into distinct buckets on the protected path — the + // (hashed) targeting key stays part of the aggregation identity. + final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); + aggregator.aggregate(event("checkout", "on", "alloc1", "alice", 1000L, false, emptyMap())); + aggregator.aggregate(event("checkout", "on", "alloc1", "bob", 2000L, false, emptyMap())); + + assertEquals(2, aggregator.fullTierSize()); + } + + @Test + void fullPathStillSplitsBucketsOnDifferingContext() { + // Consent-on preserves the previous behaviour: distinct contexts remain distinct buckets so + // each raw context is emitted verbatim. + final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); + final Map ctx1 = new HashMap<>(); + ctx1.put("plan", "pro"); + ctx1.put("request_id", "req-1"); + final Map ctx2 = new HashMap<>(); + ctx2.put("plan", "pro"); + ctx2.put("request_id", "req-2"); + + aggregator.aggregate(event("checkout", "on", "alloc1", "alice", 1000L, true, ctx1)); + aggregator.aggregate(event("checkout", "on", "alloc1", "alice", 2000L, true, ctx2)); + + assertEquals(2, aggregator.fullTierSize()); + } + private static FlagEvalEvent event( final String flagKey, final String variant, diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index b0fd6a2ce0c..915ef9946f8 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -301,6 +301,7 @@ void contextMaterializationFailureDropsSingleEvent() { "user-1", null, 1000L, + true, () -> { throw new IllegalArgumentException("bad context"); })); From 79c8009a451906b6c4cd54241a3b860a925455e0 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Thu, 30 Jul 2026 13:13:55 -0400 Subject: [PATCH 11/19] Skip evaluation-context capture on the hook hot path when consent is off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Companion to the aggregator fix: with observeFullEvaluationData=false the evaluation context is dropped on emit and no longer influences aggregation, so there is no reason to snapshot it on the evaluation thread. The hook now branches on consent up front — the protected path enqueues an event with an empty materialized attrs map (no map copy of the OpenFeature context, no Supplier allocation, no lambda instance), while the consent-on path is unchanged. Grep confirms the only production consumer of FlagEvalEvent.contextAttributes / FlagEvalEvent.attrs is FlagEvaluationAggregator.aggregate, which already skips them on the protected path. Regression test: mutating the EvaluationContext after finallyAfter returns still yields empty attrs on the enqueued event — proves the hook never snapshotted it. Two existing tests that exercise the snapshot mechanism were switched to pass consent-on metadata (that's the code path they cover). Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../api/openfeature/FlagEvalLoggingHook.java | 37 +++++++++++----- .../openfeature/FlagEvalLoggingHookTest.java | 43 ++++++++++++++++++- 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java index 6595838e770..0fe5ba5e39e 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java @@ -107,7 +107,6 @@ public void finallyAfter( // targetingKey from evaluation context final String targetingKey = ctx != null && ctx.getCtx() != null ? ctx.getCtx().getTargetingKey() : null; - final Map attrs = snapshotAttrs(ctx); // Consent is read from metadata stamped by DDEvaluator (pinned to its ServerConfiguration). // Missing key = non-DD provider → false, the privacy-preserving default. @@ -117,16 +116,32 @@ public void finallyAfter( : null; final boolean observeFullEvaluationData = consentFromMetadata != null && consentFromMetadata; - w.enqueue( - new FlagEvalEvent( - flagKey, - variant, - allocationKey, - targetingKey, - errorMessage, - evalTimeMs, - observeFullEvaluationData, - () -> extractAttrs(attrs))); + // On the protected path the evaluation context is dropped on emit and never consulted by + // the aggregator, so skip the snapshot + supplier allocation entirely. + if (observeFullEvaluationData) { + final Map attrs = snapshotAttrs(ctx); + w.enqueue( + new FlagEvalEvent( + flagKey, + variant, + allocationKey, + targetingKey, + errorMessage, + evalTimeMs, + true, + () -> extractAttrs(attrs))); + } else { + w.enqueue( + new FlagEvalEvent( + flagKey, + variant, + allocationKey, + targetingKey, + errorMessage, + evalTimeMs, + false, + Collections.emptyMap())); + } } catch (LinkageError | Exception e) { // Never let EVP recording break flag evaluation } diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java index cc1d7ec64e7..e73f307aced 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java @@ -405,7 +405,7 @@ void contextAttributesAreFlattenedAndConvertedAfterEnqueue() { .ctx(context) .build(); final FlagEvaluationDetails det = - details("ctx-flag", "v", "v", Reason.TARGETING_MATCH.name(), null); + details("ctx-flag", "v", "v", Reason.TARGETING_MATCH.name(), consentOnMetadata()); hook.finallyAfter(hookCtx, det, Collections.emptyMap()); @@ -442,7 +442,7 @@ void contextAttributesUseEnqueueTimeSnapshot() { .ctx(context) .build(); final FlagEvaluationDetails det = - details("ctx-flag", "v", "v", Reason.TARGETING_MATCH.name(), null); + details("ctx-flag", "v", "v", Reason.TARGETING_MATCH.name(), consentOnMetadata()); hook.finallyAfter(hookCtx, det, Collections.emptyMap()); context.add("region", "eu-west-1"); @@ -480,6 +480,39 @@ void observeFullEvaluationDataDefaultsToFalseWhenMetadataAbsent() { assertFalse(enqueuedEventWithConsentMetadata(null).observeFullEvaluationData); } + @Test + void protectedPathSkipsEvaluationContextCapture() { + // Consent off → the hook must not snapshot the evaluation context at all. Verified by mutating + // the context after finallyAfter returns and asserting the enqueued event still sees nothing. + final AtomicReference captured = new AtomicReference<>(); + final FlagEvalLoggingHook hook = hookWithWriter(capturingWriter(captured)); + + final MutableContext context = new MutableContext("user-1"); + context.add("region", "us-east-1"); + + final HookContext hookCtx = + HookContext.builder() + .flagKey("ctx-flag") + .type(FlagValueType.STRING) + .defaultValue("default") + .ctx(context) + .build(); + final ImmutableMetadata consentOff = + ImmutableMetadata.builder() + .addBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA, false) + .build(); + + hook.finallyAfter( + hookCtx, + details("ctx-flag", "v", "v", Reason.TARGETING_MATCH.name(), consentOff), + Collections.emptyMap()); + context.add("region", "eu-west-1"); + + assertNotNull(captured.get()); + assertTrue(captured.get().attrs.isEmpty()); + assertTrue(captured.get().contextAttributes().isEmpty()); + } + @Test void ignoresGatewayConsentEvenWhenItDisagreesWithMetadata() { // Gateway says on, metadata says off; hook must trust metadata. @@ -512,6 +545,12 @@ private FlagEvalEvent enqueuedEventWithConsentMetadata(final Boolean consent) { return captured.get(); } + private static ImmutableMetadata consentOnMetadata() { + return ImmutableMetadata.builder() + .addBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA, true) + .build(); + } + private static ServerConfiguration observeConfig(final boolean observeFullEvaluationData) { return new ServerConfiguration( "2024-04-17T19:40:53.716Z", From c4b9a82efb59206ec4a6e069ee0a24ad138e8640 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Thu, 30 Jul 2026 13:33:32 -0400 Subject: [PATCH 12/19] Include observeFullEvaluationData in the aggregation bucket key Bucket keys should cover every dimension the emitter will branch on. The serializer branches on observeFullEvaluationData (hashes the targeting key and drops the context when off), so two evaluations that differ only in consent produce different wire rows and must not share a bucket. Before this change they could: same subject, same flag, same empty context would land under the same FullKey regardless of consent, and the AND-fold would silently downgrade a consent-on evaluation to the protected wire shape because a nearby consent-off event merged into its bucket first. No PII leak (fail-closed direction), but arrival-order-dependent semantics and a lost raw-context row. Add observeFullEvaluationData to FullKey / DegradedKey (equals + hashCode). The AND-fold on bucket.observeFullEvaluationData stays as defensive belt- and-suspenders; every event merging into a bucket now carries the matching consent value by construction. Regression test: two events identical except for consent land in two full- tier buckets, one consent-on and one consent-off. Updated the previous "fold to false on mixed consent" test to reflect the new invariant. Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../featureflag/FlagEvaluationAggregator.java | 43 ++++++++++++++----- .../FlagEvaluationAggregatorTest.java | 30 +++++++++---- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java index 855704dca9b..bb950f49975 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java @@ -150,7 +150,7 @@ void simulateFullTierAtCap() { for (int i = globalFullCount.get(); i < GLOBAL_CAP; i++) { final String key = "synthetic-full-" + i; fullTier.put( - new FullKey(key, "on", "alloc", false, null, null, ""), + new FullKey(key, "on", "alloc", false, null, null, "", false), new EvalBucket(key, "on", "alloc", null, null, 1L, false, null, false)); globalFullCount.incrementAndGet(); perFlagCount.merge(key, 1, Integer::sum); @@ -161,7 +161,7 @@ void simulateDegradedTierAtCap() { for (int i = degradedTier.size(); i < DEGRADED_CAP; i++) { final String key = "synthetic-dg-" + i; degradedTier.put( - new DegradedKey(key, "on", "alloc", false, null), + new DegradedKey(key, "on", "alloc", false, null, false), new EvalBucket(key, "on", "alloc", null, null, 1L, false, null, false)); } } @@ -173,7 +173,7 @@ void addDegradedBucketForTest( final String errorMessage, final long evalTimeMs) { degradedTier.put( - new DegradedKey(flagKey, variant, allocationKey, variant == null, errorMessage), + new DegradedKey(flagKey, variant, allocationKey, variant == null, errorMessage, false), new EvalBucket( flagKey, variant, @@ -194,7 +194,8 @@ private static FullKey buildFullKey(final FlagEvalEvent event, final String ctxK event.variant == null, event.errorMessage, event.targetingKey, - ctxKey); + ctxKey, + event.observeFullEvaluationData); } private static DegradedKey buildDegradedKey(final FlagEvalEvent event) { @@ -203,7 +204,8 @@ private static DegradedKey buildDegradedKey(final FlagEvalEvent event) { event.variant, event.allocationKey, event.variant == null, - event.errorMessage); + event.errorMessage, + event.observeFullEvaluationData); } static Map pruneContext(final Map attrs) { @@ -282,8 +284,9 @@ static class EvalBucket { String targetingKey; String errorMessage; Map prunedAttrs; - // Consent to emit raw PII, taken from each event's evaluation-time snapshot. Folded with AND - // on merge: any consent-off evaluation sinks the bucket to hashed/omitted (fail-closed). + // Consent to emit raw PII, uniform per bucket (part of FullKey / DegradedKey). The AND-fold + // on merge is now defensive belt-and-suspenders — every event merging into this bucket already + // carries the matching consent value by construction. boolean observeFullEvaluationData; EvalBucket( @@ -335,6 +338,10 @@ static final class FullKey { private final String errorMessage; private final String targetingKey; private final String contextKey; + // Part of the key so consent-on and consent-off evaluations never share a bucket. The + // serializer branches on this to hash the targeting key and drop the context, so events with + // different consent produce different wire rows and belong in different buckets. + private final boolean observeFullEvaluationData; FullKey( final String flagKey, @@ -343,7 +350,8 @@ static final class FullKey { final boolean runtimeDefaultUsed, final String errorMessage, final String targetingKey, - final String contextKey) { + final String contextKey, + final boolean observeFullEvaluationData) { this.flagKey = flagKey; this.variant = variant; this.allocationKey = allocationKey; @@ -351,6 +359,7 @@ static final class FullKey { this.errorMessage = errorMessage; this.targetingKey = targetingKey; this.contextKey = contextKey; + this.observeFullEvaluationData = observeFullEvaluationData; } @Override @@ -363,6 +372,7 @@ public boolean equals(final Object o) { } final FullKey fullKey = (FullKey) o; return runtimeDefaultUsed == fullKey.runtimeDefaultUsed + && observeFullEvaluationData == fullKey.observeFullEvaluationData && Objects.equals(flagKey, fullKey.flagKey) && Objects.equals(variant, fullKey.variant) && Objects.equals(allocationKey, fullKey.allocationKey) @@ -380,7 +390,8 @@ public int hashCode() { runtimeDefaultUsed, errorMessage, targetingKey, - contextKey); + contextKey, + observeFullEvaluationData); } } @@ -390,18 +401,21 @@ static final class DegradedKey { private final String allocationKey; private final boolean runtimeDefaultUsed; private final String errorMessage; + private final boolean observeFullEvaluationData; DegradedKey( final String flagKey, final String variant, final String allocationKey, final boolean runtimeDefaultUsed, - final String errorMessage) { + final String errorMessage, + final boolean observeFullEvaluationData) { this.flagKey = flagKey; this.variant = variant; this.allocationKey = allocationKey; this.runtimeDefaultUsed = runtimeDefaultUsed; this.errorMessage = errorMessage; + this.observeFullEvaluationData = observeFullEvaluationData; } @Override @@ -414,6 +428,7 @@ public boolean equals(final Object o) { } final DegradedKey that = (DegradedKey) o; return runtimeDefaultUsed == that.runtimeDefaultUsed + && observeFullEvaluationData == that.observeFullEvaluationData && Objects.equals(flagKey, that.flagKey) && Objects.equals(variant, that.variant) && Objects.equals(allocationKey, that.allocationKey) @@ -422,7 +437,13 @@ public boolean equals(final Object o) { @Override public int hashCode() { - return Objects.hash(flagKey, variant, allocationKey, runtimeDefaultUsed, errorMessage); + return Objects.hash( + flagKey, + variant, + allocationKey, + runtimeDefaultUsed, + errorMessage, + observeFullEvaluationData); } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java index 07d4eb8996c..23cff44a6b8 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java @@ -267,20 +267,31 @@ void degradedKeyEqualityUsesEveryDimension() { } @Test - void observeFullEvaluationDataFoldsToFalseWhenAnyMergedEvaluationLacksConsent() { + void mixedConsentEvaluationsForSameSubjectLandInDistinctBuckets() { + // Consent is part of FullKey: two evaluations that differ only in consent produce different + // wire rows (raw vs hashed targeting key, context vs no context) and belong in different + // buckets. Merging them would silently downgrade the consent-on row to the protected shape. final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); - // First event consents; a later one (e.g. after RC flipped consent off) folds into the bucket. aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, true, emptyMap())); aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, false, emptyMap())); - final FlagEvaluationAggregator.EvalBucket bucket = - aggregator.snapshot().fullTier.values().iterator().next(); - assertEquals(2, bucket.count); - assertFalse(bucket.observeFullEvaluationData); + assertEquals(2, aggregator.fullTierSize()); + int onCount = 0; + int offCount = 0; + for (final FlagEvaluationAggregator.EvalBucket bucket : + aggregator.snapshot().fullTier.values()) { + if (bucket.observeFullEvaluationData) { + onCount++; + } else { + offCount++; + } + } + assertEquals(1, onCount); + assertEquals(1, offCount); } @Test - void observeFullEvaluationDataStaysTrueWhenEveryMergedEvaluationConsents() { + void sameConsentEvaluationsForSameSubjectMergeIntoOneBucket() { final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 1000L, true, emptyMap())); aggregator.aggregate(event("fold-flag", "on", "alloc1", "user-1", 2000L, true, emptyMap())); @@ -392,7 +403,8 @@ private static FlagEvaluationAggregator.FullKey fullKey( runtimeDefaultUsed, errorMessage, targetingKey, - contextKey); + contextKey, + false); } private static FlagEvaluationAggregator.DegradedKey degradedKey( @@ -402,7 +414,7 @@ private static FlagEvaluationAggregator.DegradedKey degradedKey( final boolean runtimeDefaultUsed, final String errorMessage) { return new FlagEvaluationAggregator.DegradedKey( - flagKey, variant, allocationKey, runtimeDefaultUsed, errorMessage); + flagKey, variant, allocationKey, runtimeDefaultUsed, errorMessage, false); } private static String repeat(final char c, final int count) { From 1b034ccaa9ac5ec34507679d88d05e8fbaa23c55 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Thu, 30 Jul 2026 16:39:39 -0400 Subject: [PATCH 13/19] Add consent metadata to ProviderTest flag-eval-logging hook route test The test asserts that context attributes flow through the logging hook, but the mock metadata omitted the observe-full-evaluation-data flag, so the hook took the privacy-preserving path and dropped context. Co-Authored-By: Claude Sonnet 4.6 --- .../test/java/datadog/trace/api/openfeature/ProviderTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/ProviderTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/ProviderTest.java index ef37cdb330b..d35b78335b8 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/ProviderTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/ProviderTest.java @@ -376,6 +376,7 @@ public void testClientEvaluationRoutesThroughFlagEvalLoggingHook() throws Except ImmutableMetadata.builder() .addString("allocationKey", "allocation-1") .addLong("dd.eval.timestamp_ms", 1_700_000_000_000L) + .addBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA, true) .build()) .build()); final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); From 857d65b95d4044db6990ac0b903aed753cef7555 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Tue, 4 Aug 2026 08:55:11 -0400 Subject: [PATCH 14/19] Redact error messages when observeFullEvaluationData is off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exception messages from the evaluator's outer catch blocks (NumberFormatException, generic Exception) can echo raw evaluation-context values verbatim — for example a GT rule on "id" with a PII-shaped targeting key produced error.message="For input string: \"jane.doe@...\"" on the wire regardless of consent, defeating the PR's own PII guard. Drop the message at DDEvaluator.error() when consent is off, and add a hook-layer fallback that substitutes ErrorCode.name() so operators keep a stable signal (e.g. "TYPE_MISMATCH") even when a third-party provider hands us a raw message. Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trace/api/openfeature/DDEvaluator.java | 19 +++--- .../api/openfeature/FlagEvalLoggingHook.java | 23 ++++--- .../api/openfeature/DDEvaluatorTest.java | 68 +++++++++++++++++++ .../openfeature/FlagEvalLoggingHookTest.java | 67 +++++++++++++++++- .../FlagEvaluationWriterImplTest.java | 35 ++++++++++ 5 files changed, 192 insertions(+), 20 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java index c25eff9eae3..88a12908196 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java @@ -226,11 +226,16 @@ private static ProviderEvaluation error( final ErrorCode code, final String errorMessage, final boolean observeFullEvaluationData) { + // Under consent-off the errorMessage is dropped: exception messages from the outer catch blocks + // (NumberFormatException, generic Exception) can echo raw evaluation-context values, so they + // must never reach any consumer of ProviderEvaluation.getErrorMessage() — not just our own + // wire hook. Downstream (FlagEvalLoggingHook) falls back to ErrorCode.name(), so operators + // still get a stable signal like "TYPE_MISMATCH". return ProviderEvaluation.builder() .value(defaultValue) .reason(Reason.ERROR.name()) .errorCode(code) - .errorMessage(errorMessage) + .errorMessage(observeFullEvaluationData ? errorMessage : null) .flagMetadata(consentMetadata(observeFullEvaluationData)) .build(); } @@ -397,13 +402,11 @@ private static ProviderEvaluation resolveVariant( final boolean observeFullEvaluationData) { final Variant variant = flag.variations.get(variationKey); if (variant == null) { - return ProviderEvaluation.builder() - .value(defaultValue) - .reason(Reason.ERROR.name()) - .errorCode(ErrorCode.GENERAL) - .errorMessage("Variant not found for: " + variationKey) - .flagMetadata(consentMetadata(observeFullEvaluationData)) - .build(); + return error( + defaultValue, + ErrorCode.GENERAL, + "Variant not found for: " + variationKey, + observeFullEvaluationData); } if (!isTypeCompatible(target, flag.variationType)) { diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java index 0fe5ba5e39e..391ff44afb1 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/FlagEvalLoggingHook.java @@ -94,16 +94,6 @@ public void finallyAfter( // evaluated value. A null variant means no variant was selected (runtime default). final String variant = details.getVariant(); - // error message: prefer the human-readable message; fall back to the error code name when - // the message is empty (some providers populate only the code). null on success. - String errorMessage = details.getErrorMessage(); - if ((errorMessage == null || errorMessage.isEmpty()) && details.getErrorCode() != null) { - errorMessage = details.getErrorCode().name(); - } - if (errorMessage != null && errorMessage.isEmpty()) { - errorMessage = null; - } - // targetingKey from evaluation context final String targetingKey = ctx != null && ctx.getCtx() != null ? ctx.getCtx().getTargetingKey() : null; @@ -116,6 +106,19 @@ public void finallyAfter( : null; final boolean observeFullEvaluationData = consentFromMetadata != null && consentFromMetadata; + // Error message: prefer the human-readable message under consent-on; under consent-off the + // provider's raw message can echo evaluation-context values (e.g. NumberFormatException: + // "For input string: \"jane.doe@...\""), so replace it with the ErrorCode name — a stable, + // PII-free signal. Same substitution path is used when the message is absent regardless of + // consent (some providers populate only the code). Null on success. + String errorMessage = observeFullEvaluationData ? details.getErrorMessage() : null; + if ((errorMessage == null || errorMessage.isEmpty()) && details.getErrorCode() != null) { + errorMessage = details.getErrorCode().name(); + } + if (errorMessage != null && errorMessage.isEmpty()) { + errorMessage = null; + } + // On the protected path the evaluation context is dropped on emit and never consulted by // the aggregator, so skip the snapshot + supplier allocation entirely. if (observeFullEvaluationData) { diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index 0ad3f12cf9c..a73a26a166c 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -12,6 +12,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasEntry; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -24,8 +26,13 @@ import com.squareup.moshi.Moshi; import com.squareup.moshi.Types; import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.ufc.v1.Allocation; +import datadog.trace.api.featureflag.ufc.v1.ConditionConfiguration; +import datadog.trace.api.featureflag.ufc.v1.ConditionOperator; import datadog.trace.api.featureflag.ufc.v1.Flag; +import datadog.trace.api.featureflag.ufc.v1.Rule; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; +import datadog.trace.api.featureflag.ufc.v1.ValueType; import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.MutableContext; @@ -244,6 +251,67 @@ public void observeFullEvaluationDataDefaultsToFalseWhenEvaluatorHasNoConfig() { equalTo(false)); } + // ---- error message redaction respects observeFullEvaluationData ---- + + @Test + public void numericConditionOnTargetingKeyDropsExceptionMessageUnderConsentOff() { + // Rule {attribute:"id", operator:GT, value:0} + "id" not in context → + // DDEvaluator.resolveAttribute + // falls back to the targeting key, so Double.parseDouble("jane.doe@datadoghq.com") throws + // NumberFormatException. The exception message echoes the raw context value verbatim, so it + // must be dropped when observeFullEvaluationData=false. + final ProviderEvaluation details = + evaluateWithNumericRuleOnId("jane.doe@datadoghq.com", false); + + assertThat(details.getErrorCode(), equalTo(ErrorCode.TYPE_MISMATCH)); + assertNull(details.getErrorMessage(), "consent-off must not surface the raw exception message"); + } + + @Test + public void numericConditionOnTargetingKeyPreservesExceptionMessageUnderConsentOn() { + // Symmetric case: with consent on, the raw exception message flows through unchanged so + // operators keep the diagnostic detail they opted in to. + final ProviderEvaluation details = + evaluateWithNumericRuleOnId("jane.doe@datadoghq.com", true); + + assertThat(details.getErrorCode(), equalTo(ErrorCode.TYPE_MISMATCH)); + assertThat(details.getErrorMessage(), equalTo("For input string: \"jane.doe@datadoghq.com\"")); + } + + @Test + public void numericConditionOnTargetingKeyErrorMessageNeverContainsPiiUnderConsentOff() { + // Belt-and-suspenders: independent of the exact null/empty form, the raw PII value must never + // appear in the message under consent-off. Guards against future changes that might replace + // null with a redacted string or a code-name suffix. + final ProviderEvaluation details = + evaluateWithNumericRuleOnId("jane.doe@datadoghq.com", false); + + final String message = details.getErrorMessage(); + assertFalse( + message != null && message.contains("jane.doe@datadoghq.com"), + "consent-off errorMessage must not contain raw context values"); + } + + private static ProviderEvaluation evaluateWithNumericRuleOnId( + final String targetingKey, final boolean observeFullEvaluationData) { + final Map flags = new HashMap<>(); + final List rules = + singletonList( + new Rule(singletonList(new ConditionConfiguration(ConditionOperator.GT, "id", 0)))); + // Split must be non-empty so the allocation is considered a match target; its contents don't + // matter because the rule throws before a split is picked. + final Allocation allocation = + new Allocation("alloc", rules, null, null, emptyList(), Boolean.FALSE); + flags.put( + "num-rule", + new Flag("num-rule", true, ValueType.INTEGER, emptyMap(), singletonList(allocation))); + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + evaluator.accept(new ServerConfiguration("", "", observeFullEvaluationData, null, flags)); + + final EvaluationContext ctx = new MutableContext(targetingKey); + return evaluator.evaluate(Integer.class, "num-rule", 23, ctx); + } + private static Arguments[] flatteningTestCases() { final List arguments = new ArrayList<>(); arguments.add(Arguments.of(emptyMap(), emptyMap())); diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java index e73f307aced..f96baa93742 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/FlagEvalLoggingHookTest.java @@ -226,7 +226,9 @@ void absentVariantProducesNullVariant() { // ---- test: error message captured from details (error object support) ---- @Test - void errorMessageCapturedFromDetails() { + void errorMessageCapturedFromDetailsUnderConsentOn() { + // With observeFullEvaluationData=true the provider's raw message is preserved verbatim so + // operators keep the diagnostic detail they opted in to. final AtomicReference captured = new AtomicReference<>(); final FlagEvalLoggingHook hook = hookWithWriter(capturingWriter(captured)); @@ -237,6 +239,7 @@ void errorMessageCapturedFromDetails() { .reason(Reason.ERROR.name()) .errorCode(ErrorCode.TYPE_MISMATCH) .errorMessage("value does not match declared type") + .flagMetadata(consentOnMetadata()) .build(); hook.finallyAfter(null, det, Collections.emptyMap()); @@ -245,7 +248,61 @@ void errorMessageCapturedFromDetails() { assertEquals( "value does not match declared type", captured.get().errorMessage, - "errorMessage must be captured from the evaluation details"); + "errorMessage must be captured from the evaluation details under consent-on"); + } + + @Test + void errorMessageReplacedByErrorCodeUnderConsentOff() { + // Defense-in-depth: even if a provider hands us a raw message under consent-off (a bug in the + // provider, or a third-party provider that doesn't distinguish consent tiers), the hook must + // substitute the ErrorCode name so raw PII from exception messages never reaches the wire. + // Uses a PII-looking marker exactly like the wire-level guards in FlagEvaluationWriterImplTest. + final AtomicReference captured = new AtomicReference<>(); + final FlagEvalLoggingHook hook = hookWithWriter(capturingWriter(captured)); + + final FlagEvaluationDetails det = + FlagEvaluationDetails.builder() + .flagKey("err-flag") + .value("default") + .reason(Reason.ERROR.name()) + .errorCode(ErrorCode.TYPE_MISMATCH) + .errorMessage("For input string: \"jane.doe@datadoghq.com\"") + .flagMetadata(consentOffMetadata()) + .build(); + + hook.finallyAfter(null, det, Collections.emptyMap()); + + assertNotNull(captured.get()); + assertEquals( + "TYPE_MISMATCH", + captured.get().errorMessage, + "consent-off must replace the raw message with the ErrorCode name"); + assertFalse( + captured.get().errorMessage.contains("jane.doe@datadoghq.com"), + "raw PII must never survive into the enqueued event under consent-off"); + } + + @Test + void errorMessageDroppedWhenConsentOffAndNoErrorCode() { + // Edge case: no ErrorCode available (unusual — providers should set one for ERROR reason). + // Consent-off drops the message and there's nothing to substitute, so the enqueued event has + // no error message at all. This is the strictest privacy-preserving outcome. + final AtomicReference captured = new AtomicReference<>(); + final FlagEvalLoggingHook hook = hookWithWriter(capturingWriter(captured)); + + final FlagEvaluationDetails det = + FlagEvaluationDetails.builder() + .flagKey("err-flag") + .value("default") + .reason(Reason.ERROR.name()) + .errorMessage("For input string: \"jane.doe@datadoghq.com\"") + .flagMetadata(consentOffMetadata()) + .build(); + + hook.finallyAfter(null, det, Collections.emptyMap()); + + assertNotNull(captured.get()); + assertNull(captured.get().errorMessage); } // ---- test: error code used as fallback message when error message is empty ---- @@ -551,6 +608,12 @@ private static ImmutableMetadata consentOnMetadata() { .build(); } + private static ImmutableMetadata consentOffMetadata() { + return ImmutableMetadata.builder() + .addBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA, false) + .build(); + } + private static ServerConfiguration observeConfig(final boolean observeFullEvaluationData) { return new ServerConfiguration( "2024-04-17T19:40:53.716Z", diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index 915ef9946f8..3e0958973a6 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -641,6 +641,41 @@ void eventConsentTrueStaysRawEvenWhenGatewayLaterReportsFalse() throws Exception assertNotNull(ctx.get("evaluation")); } + @Test + void consentOffPreservesErrorCodeSignalAndNeverLeaksPiiInErrorMessage() throws Exception { + // Upstream contract: the hook substitutes the ErrorCode name for the raw exception message + // under consent-off (see + // FlagEvalLoggingHookTest#errorMessageReplacedByErrorCodeUnderConsentOff). + // This wire-level guard pins that a properly-formed consent-off event (a) still surfaces the + // stable ErrorCode signal for operators and (b) never lets a PII-shaped string escape onto the + // wire. Mirrors the existing PII guards on the targeting_key axis. + final BackendApi mockEvp = mock(BackendApi.class); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(mockEvp); + setup.handler.add( + new FlagEvalEvent( + "err-flag", + null, + "alloc1", + "jane.doe@datadoghq.com", + "TYPE_MISMATCH", + 1000L, + false, + emptyMap())); + + final FlagEvaluationTestSupport.CapturedJson captured = flushAndCapture(setup); + + final Map ev = eventForFlag(captured.parsed, "err-flag"); + assertNotNull(ev); + final Map error = (Map) ev.get("error"); + assertNotNull(error, "error object must be present so operators keep the ErrorCode signal"); + assertEquals("TYPE_MISMATCH", error.get("message")); + assertEquals(HASHED_JANE_DOE, ev.get("targeting_key")); + assertFalse(captured.raw.contains("jane.doe@datadoghq.com")); + assertFalse( + captured.raw.contains("For input string"), + "no exception-message-shaped text may reach the wire under consent-off"); + } + private void assertHashedTargetingKeyAndOmittedContext(final FlagEvalEvent piiEvent) throws Exception { final BackendApi mockEvp = mock(BackendApi.class); From b1a7a2bd56e853eac24d30de977610e22de49853 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Tue, 4 Aug 2026 09:02:56 -0400 Subject: [PATCH 15/19] Exercise every consent-stamp code path in DDEvaluatorTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only observeFullEvaluationData assertions were on error paths (FLAG_NOT_FOUND, PROVIDER_NOT_READY), leaving the success-path stamp in resolveVariant and the DISABLED/DEFAULT stamps in consentMetadata uncovered — line 448 could be deleted or hardcoded to either value and every existing test would still pass. Add symmetric consent-on/consent-off tests for each of resolveVariant, DISABLED, and DEFAULT so any mutation (delete / hardcode true / hardcode false) flips at least one assertion. Rename the previously misleading …OnSuccess test to reflect what it actually exercises (FLAG_NOT_FOUND error via error()). Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../api/openfeature/DDEvaluatorTest.java | 133 +++++++++++++++++- 1 file changed, 129 insertions(+), 4 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index a73a26a166c..6ed54882b95 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -32,7 +32,9 @@ import datadog.trace.api.featureflag.ufc.v1.Flag; import datadog.trace.api.featureflag.ufc.v1.Rule; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; +import datadog.trace.api.featureflag.ufc.v1.Split; import datadog.trace.api.featureflag.ufc.v1.ValueType; +import datadog.trace.api.featureflag.ufc.v1.Variant; import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.MutableContext; @@ -222,13 +224,93 @@ public void testNoAllocations() { // ---- observeFullEvaluationData metadata is stamped from the evaluator's ServerConfiguration // ---- + // + // Every code path that returns a ProviderEvaluation must stamp the consent boolean so downstream + // hooks can honour it. These tests exercise each stamp site with both consent values (on/off) so + // a mutation to any stamp — deleting the line, hardcoding the value — flips at least one + // assertion. + + // -- success path: resolveVariant (variant metadata builder) -- @Test - public void observeFullEvaluationDataStampedFromEvaluatorConfigOnSuccess() { - final Map flags = new HashMap<>(); - flags.put("null-allocation", new Flag("target", true, null, null, null)); + public void observeFullEvaluationDataStampedTrueOnResolvedVariant() { + final ProviderEvaluation details = evaluateMatchingFlag(true); + + assertThat(details.getReason(), equalTo("STATIC")); + assertThat(details.getVariant(), equalTo("on")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(true)); + } + + @Test + public void observeFullEvaluationDataStampedFalseOnResolvedVariant() { + // Symmetric consent-off assertion. Paired with the consent-on test above this pins the + // resolveVariant metadata line (DDEvaluator.java: METADATA_OBSERVE_FULL_EVALUATION_DATA) so + // deleting it or hardcoding either value would fail at least one assertion. + final ProviderEvaluation details = evaluateMatchingFlag(false); + + assertThat(details.getReason(), equalTo("STATIC")); + assertThat(details.getVariant(), equalTo("on")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(false)); + } + + // -- DISABLED path: flag.enabled=false -- + + @Test + public void observeFullEvaluationDataStampedTrueOnDisabledFlag() { + final ProviderEvaluation details = evaluateDisabledFlag(true); + + assertThat(details.getReason(), equalTo("DISABLED")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(true)); + } + + @Test + public void observeFullEvaluationDataStampedFalseOnDisabledFlag() { + final ProviderEvaluation details = evaluateDisabledFlag(false); + + assertThat(details.getReason(), equalTo("DISABLED")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(false)); + } + + // -- DEFAULT path: no allocation matches -- + + @Test + public void observeFullEvaluationDataStampedTrueOnDefault() { + // Allocation exists but has empty splits, so the loop finishes without returning and we fall + // through to the DEFAULT branch. + final ProviderEvaluation details = evaluateWithEmptySplits(true); + + assertThat(details.getReason(), equalTo("DEFAULT")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(true)); + } + + @Test + public void observeFullEvaluationDataStampedFalseOnDefault() { + final ProviderEvaluation details = evaluateWithEmptySplits(false); + + assertThat(details.getReason(), equalTo("DEFAULT")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(false)); + } + + // -- error paths: FLAG_NOT_FOUND / PROVIDER_NOT_READY (via consentMetadata in error()) -- + + @Test + public void observeFullEvaluationDataStampedOnFlagNotFoundError() { + // Was previously named "…OnSuccess" but actually exercises the error() helper's stamp via + // FLAG_NOT_FOUND — kept for that stamp site, correctly named. final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); - evaluator.accept(new ServerConfiguration("", "", true, null, flags)); + evaluator.accept(new ServerConfiguration("", "", true, null, new HashMap<>())); final EvaluationContext ctx = new MutableContext("target").setTargetingKey("k"); final ProviderEvaluation details = @@ -251,6 +333,49 @@ public void observeFullEvaluationDataDefaultsToFalseWhenEvaluatorHasNoConfig() { equalTo(false)); } + // Builds a flag that reaches resolveVariant: enabled, one allocation with no rules, one split + // with empty shards (so the shard-match branch is skipped and the split is picked immediately), + // and a single "on" variant whose value maps to the requested Integer type. + private static ProviderEvaluation evaluateMatchingFlag( + final boolean observeFullEvaluationData) { + final Map variations = new HashMap<>(); + variations.put("on", new Variant("on", 1)); + final Split split = new Split(emptyList(), "on", emptyMap(), null); + final Allocation allocation = + new Allocation("alloc-1", null, null, null, singletonList(split), Boolean.FALSE); + return evaluateFlag( + new Flag("target", true, ValueType.INTEGER, variations, singletonList(allocation)), + observeFullEvaluationData); + } + + private static ProviderEvaluation evaluateDisabledFlag( + final boolean observeFullEvaluationData) { + return evaluateFlag( + new Flag("target", false, ValueType.INTEGER, emptyMap(), null), observeFullEvaluationData); + } + + private static ProviderEvaluation evaluateWithEmptySplits( + final boolean observeFullEvaluationData) { + // Enabled, allocations present, allocation active, no rules, empty splits → falls through the + // for-loop to the DEFAULT return. + final Allocation allocation = + new Allocation("alloc-1", null, null, null, emptyList(), Boolean.FALSE); + return evaluateFlag( + new Flag("target", true, ValueType.INTEGER, emptyMap(), singletonList(allocation)), + observeFullEvaluationData); + } + + private static ProviderEvaluation evaluateFlag( + final Flag flag, final boolean observeFullEvaluationData) { + final Map flags = new HashMap<>(); + flags.put("target", flag); + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + evaluator.accept(new ServerConfiguration("", "", observeFullEvaluationData, null, flags)); + + final EvaluationContext ctx = new MutableContext("target").setTargetingKey("user-1"); + return evaluator.evaluate(Integer.class, "target", 23, ctx); + } + // ---- error message redaction respects observeFullEvaluationData ---- @Test From 52bd9a199a7a83aa84f1f4abac3a0d8e0be1c458 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Tue, 4 Aug 2026 09:19:51 -0400 Subject: [PATCH 16/19] Drop consent from DegradedKey to reclaim effective DEGRADED_CAP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two degraded buckets differing only in observeFullEvaluationData emit byte-identical wire JSON — the degraded serializer (fromBucket with isFullTier=false) drops the targeting key and context regardless of consent — so the consent dimension in DegradedKey halved effective DEGRADED_CAP for zero wire fidelity gain. FullKey correctly keeps consent (the full-tier serializer branches on it for raw-vs-hashed targeting key and context inclusion). Mixed-consent events now merge into one degraded bucket. The AND-fold on EvalBucket.observeFullEvaluationData still runs and collapses to false whenever any consent-off event lands in a mixed bucket; benign because the value has no downstream effect for degraded rows. Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../featureflag/FlagEvaluationAggregator.java | 37 +++++++++---------- .../FlagEvaluationAggregatorTest.java | 26 ++++++++++++- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java index bb950f49975..143bf2de70d 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationAggregator.java @@ -161,7 +161,7 @@ void simulateDegradedTierAtCap() { for (int i = degradedTier.size(); i < DEGRADED_CAP; i++) { final String key = "synthetic-dg-" + i; degradedTier.put( - new DegradedKey(key, "on", "alloc", false, null, false), + new DegradedKey(key, "on", "alloc", false, null), new EvalBucket(key, "on", "alloc", null, null, 1L, false, null, false)); } } @@ -173,7 +173,7 @@ void addDegradedBucketForTest( final String errorMessage, final long evalTimeMs) { degradedTier.put( - new DegradedKey(flagKey, variant, allocationKey, variant == null, errorMessage, false), + new DegradedKey(flagKey, variant, allocationKey, variant == null, errorMessage), new EvalBucket( flagKey, variant, @@ -204,8 +204,7 @@ private static DegradedKey buildDegradedKey(final FlagEvalEvent event) { event.variant, event.allocationKey, event.variant == null, - event.errorMessage, - event.observeFullEvaluationData); + event.errorMessage); } static Map pruneContext(final Map attrs) { @@ -284,9 +283,12 @@ static class EvalBucket { String targetingKey; String errorMessage; Map prunedAttrs; - // Consent to emit raw PII, uniform per bucket (part of FullKey / DegradedKey). The AND-fold - // on merge is now defensive belt-and-suspenders — every event merging into this bucket already - // carries the matching consent value by construction. + // Consent to emit raw PII. For full-tier buckets this is uniform (consent is a FullKey + // dimension) and the AND-fold on merge is defensive. For degraded-tier buckets consent is NOT + // a key dimension — mixed-consent events merge here — so the AND-fold produces false whenever + // any consent-off event lands in the bucket. That's benign because the degraded wire path + // drops the targeting key and context regardless of consent, so this field has no downstream + // effect for degraded rows. boolean observeFullEvaluationData; EvalBucket( @@ -396,26 +398,30 @@ public int hashCode() { } static final class DegradedKey { + // Unlike FullKey, consent is NOT a bucket dimension here: the wire serializer for degraded rows + // (FlagEvaluationPayloads.FlagEvaluationEvent.fromBucket with isFullTier=false) drops the + // targeting key and context unconditionally, so two degraded buckets differing only in consent + // would emit byte-identical JSON with evaluation_count split — halving effective DEGRADED_CAP + // for zero wire fidelity. Mixed-consent events merge into one bucket; the AND-fold on + // EvalBucket.observeFullEvaluationData still runs but has no downstream effect for degraded + // rows. private final String flagKey; private final String variant; private final String allocationKey; private final boolean runtimeDefaultUsed; private final String errorMessage; - private final boolean observeFullEvaluationData; DegradedKey( final String flagKey, final String variant, final String allocationKey, final boolean runtimeDefaultUsed, - final String errorMessage, - final boolean observeFullEvaluationData) { + final String errorMessage) { this.flagKey = flagKey; this.variant = variant; this.allocationKey = allocationKey; this.runtimeDefaultUsed = runtimeDefaultUsed; this.errorMessage = errorMessage; - this.observeFullEvaluationData = observeFullEvaluationData; } @Override @@ -428,7 +434,6 @@ public boolean equals(final Object o) { } final DegradedKey that = (DegradedKey) o; return runtimeDefaultUsed == that.runtimeDefaultUsed - && observeFullEvaluationData == that.observeFullEvaluationData && Objects.equals(flagKey, that.flagKey) && Objects.equals(variant, that.variant) && Objects.equals(allocationKey, that.allocationKey) @@ -437,13 +442,7 @@ public boolean equals(final Object o) { @Override public int hashCode() { - return Objects.hash( - flagKey, - variant, - allocationKey, - runtimeDefaultUsed, - errorMessage, - observeFullEvaluationData); + return Objects.hash(flagKey, variant, allocationKey, runtimeDefaultUsed, errorMessage); } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java index 23cff44a6b8..81f0c3c5775 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationAggregatorTest.java @@ -98,6 +98,30 @@ void perFlagCapOverflowRoutesToDegradedTierAndMergesSameDegradedKey() { assertEquals(2000L, bucket.lastEvalMs); } + @Test + void mixedConsentDegradedEvaluationsMergeIntoOneBucket() { + // Mirror of mixedConsentEvaluationsForSameSubjectLandInDistinctBuckets, but for the degraded + // tier: the wire serializer for degraded rows drops the targeting key and context regardless + // of consent, so two events differing only in consent emit byte-identical JSON. They must + // share a bucket, otherwise DEGRADED_CAP is effectively halved for zero wire fidelity gain. + // The AND-fold on EvalBucket.observeFullEvaluationData still runs but the value has no + // downstream effect for degraded rows. + final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); + aggregator.perFlagCount.put("hot-flag", FlagEvaluationAggregator.PER_FLAG_CAP); + + aggregator.aggregate(event("hot-flag", "on", "alloc1", "user-1", 1000L, true, emptyMap())); + aggregator.aggregate(event("hot-flag", "on", "alloc1", "user-2", 2000L, false, emptyMap())); + + final FlagEvaluationAggregator.AggregatedState state = aggregator.snapshot(); + assertEquals(0, state.fullTier.size()); + assertEquals(1, state.degradedTier.size()); + final FlagEvaluationAggregator.EvalBucket bucket = + state.degradedTier.values().iterator().next(); + assertEquals(2, bucket.count); + // AND-fold collapses to consent-off; benign for degraded rows but a documented invariant. + assertFalse(bucket.observeFullEvaluationData); + } + @Test void absentVariantSetsRuntimeDefaultUsed() { final FlagEvaluationAggregator aggregator = new FlagEvaluationAggregator(); @@ -414,7 +438,7 @@ private static FlagEvaluationAggregator.DegradedKey degradedKey( final boolean runtimeDefaultUsed, final String errorMessage) { return new FlagEvaluationAggregator.DegradedKey( - flagKey, variant, allocationKey, runtimeDefaultUsed, errorMessage, false); + flagKey, variant, allocationKey, runtimeDefaultUsed, errorMessage); } private static String repeat(final char c, final int count) { From a27aa082888f46d8a2f43c889b6c07b9bd95f671 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Tue, 4 Aug 2026 09:31:31 -0400 Subject: [PATCH 17/19] Tolerate malformed observeFullEvaluationData in UFC parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this change ServerConfiguration.observeFullEvaluationData was a primitive boolean — Moshi's reflective adapter rejected the entire UFC whenever the JSON value was null or wrong-typed. Agentless swallows the IOException at DEBUG, so a pod starting after a malformed message had no last-known-good, stranded every flag on PROVIDER_NOT_READY, and served defaults forever. Fail-closed on privacy shouldn't cascade into fail-closed on availability. Box the field to Boolean so null tolerates naturally, register a LenientBooleanAdapter that maps wrong-typed values to null as well, and read via Boolean.TRUE.equals(...) at the DDEvaluator so null falls to the privacy-preserving default. The lenient adapter only intercepts Boolean (not primitive boolean), so mandatory fields like Flag.enabled keep their strict parse; the only other Boolean it touches is Allocation.doLog, which is already read as `!= null && doLog`. Reversed the earlier RejectsExplicitNull test — it had locked in the buggy behaviour — into a family of tolerance tests for null / stringified / numeric. Added a DDEvaluator test that a config with a null consent field evaluates without NPE and stamps the privacy-preserving default. Generated with Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trace/api/openfeature/DDEvaluator.java | 5 +- .../api/openfeature/DDEvaluatorTest.java | 20 +++++++ .../ufc/v1/ServerConfiguration.java | 9 ++- .../UniversalFlagConfigParser.java | 50 +++++++++++++++- .../JsonApiUfcResponseParserTest.java | 60 +++++++++++++++---- 5 files changed, 129 insertions(+), 15 deletions(-) diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java index 88a12908196..6c68ae8c612 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java @@ -112,7 +112,10 @@ public ProviderEvaluation evaluate( // ProviderEvaluation returned, so the hook's consent decision is pinned to this evaluation's // config and cannot drift on a concurrent Remote Config swap. final ServerConfiguration config = configuration.get(); - final boolean observeFullEvaluationData = config != null && config.observeFullEvaluationData; + // Boolean.TRUE.equals covers both null (privacy-preserving default) and Boolean.FALSE without + // an NPE — the field is boxed so a malformed UFC message doesn't abort the whole parse. + final boolean observeFullEvaluationData = + config != null && Boolean.TRUE.equals(config.observeFullEvaluationData); try { if (config == null) { return error(defaultValue, ErrorCode.PROVIDER_NOT_READY, null, observeFullEvaluationData); diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java index 6ed54882b95..82ade47ec50 100644 --- a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java @@ -333,6 +333,26 @@ public void observeFullEvaluationDataDefaultsToFalseWhenEvaluatorHasNoConfig() { equalTo(false)); } + @Test + public void observeFullEvaluationDataNullConfigFieldTreatedAsFalse() { + // The field is boxed so Moshi tolerates a malformed consent value in the UFC JSON without + // aborting the whole parse. The evaluator must then interpret null as the privacy-preserving + // default. An auto-unbox at the read site (config.observeFullEvaluationData) would NPE here. + final Map flags = new HashMap<>(); + flags.put("target", new Flag("target", true, ValueType.INTEGER, emptyMap(), emptyList())); + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + evaluator.accept(new ServerConfiguration("", "", null, null, flags)); + + final EvaluationContext ctx = new MutableContext("target").setTargetingKey("k"); + final ProviderEvaluation details = evaluator.evaluate(Integer.class, "target", 23, ctx); + + // Flags still evaluate — availability preserved despite the malformed consent field. + assertThat(details.getReason(), equalTo("DEFAULT")); + assertThat( + details.getFlagMetadata().getBoolean(DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA), + equalTo(false)); + } + // Builds a flag that reaches resolveVariant: enabled, one allocation with no rules, one split // with empty shards (so the shard-match branch is skipped and the split is picked immediately), // and a single "on" variant whose value maps to the requested Integer type. diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java index 8128eca7b32..caaa85a611f 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ServerConfiguration.java @@ -5,14 +5,19 @@ public class ServerConfiguration { public final String createdAt; public final String format; - public final boolean observeFullEvaluationData; + // Boxed on purpose. Moshi's reflective adapter for a primitive boolean field aborts the whole + // UFC parse when the JSON value is null or not a boolean; with a Boolean field it tolerates + // null (and other malformed values are still caught locally) so a malformed consent field + // doesn't strand a fresh pod on PROVIDER_NOT_READY. Read sites must use + // Boolean.TRUE.equals(...) so null falls to the privacy-preserving default. + public final Boolean observeFullEvaluationData; public final Environment environment; public final Map flags; public ServerConfiguration( final String createdAt, final String format, - final boolean observeFullEvaluationData, + final Boolean observeFullEvaluationData, final Environment environment, final Map flags) { this.createdAt = createdAt; diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/UniversalFlagConfigParser.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/UniversalFlagConfigParser.java index ba5536601f7..5e626e1aea4 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/UniversalFlagConfigParser.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/UniversalFlagConfigParser.java @@ -29,7 +29,11 @@ final class UniversalFlagConfigParser implements ConfigurationDeserializer V1_ADAPTER = MOSHI.adapter(ServerConfiguration.class); @@ -112,6 +116,50 @@ public void toJson(@Nonnull final JsonWriter writer, @Nullable final MapOnly applies to {@code Boolean.class} (not primitive {@code boolean}), so mandatory + * primitive-boolean fields (e.g. {@code Flag.enabled}) keep their strict parse. + */ + static final class LenientBooleanAdapter extends JsonAdapter { + + static final Factory FACTORY = + new Factory() { + @Nullable + @Override + public JsonAdapter create( + @Nonnull final Type type, + @Nonnull final Set annotations, + @Nonnull final Moshi moshi) { + if (!annotations.isEmpty() || type != Boolean.class) { + return null; + } + return new LenientBooleanAdapter(); + } + }; + + @Nullable + @Override + public Boolean fromJson(@Nonnull final JsonReader reader) throws IOException { + if (reader.peek() == JsonReader.Token.BOOLEAN) { + return reader.nextBoolean(); + } + // null and every wrong-typed value collapse to null so the caller falls back to its default + // rather than the enclosing config being rejected wholesale. + reader.skipValue(); + return null; + } + + @Override + public void toJson(@Nonnull final JsonWriter writer, @Nullable final Boolean value) + throws IOException { + throw new UnsupportedOperationException("Reading only adapter"); + } + } + static final class DateAdapter extends JsonAdapter { @Nullable diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java index 3db21f3c8dc..104adef4cbe 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/JsonApiUfcResponseParserTest.java @@ -78,9 +78,12 @@ void rejectsTrailingJson() { @Test void observeFullEvaluationDataDefaultsToFalseWhenAbsent() throws Exception { + // Absent → Moshi leaves the boxed field null; the read site's Boolean.TRUE.equals(...) then + // resolves to the privacy-preserving default (consent-off). Either null-or-false is the + // documented invariant; assert the field never reads as true. final ServerConfiguration configuration = parse(wrap(emptyConfig())); assertNotNull(configuration); - assertFalse(configuration.observeFullEvaluationData); + assertFalse(Boolean.TRUE.equals(configuration.observeFullEvaluationData)); } @ParameterizedTest @@ -93,14 +96,44 @@ void observeFullEvaluationDataParsesExplicitValue(final boolean value) throws Ex } @Test - void observeFullEvaluationDataRejectsExplicitNull() { - // An explicit null for this boolean is malformed input. Parsing rejects the whole - // configuration, which is the fail-closed outcome we want: full evaluation data is never - // observed off the back of a malformed config. Callers (AgentlessConfigurationSource and the - // remote-config poller) swallow the failure and keep the last-known-good config, and the - // gateway defaults to the privacy-preserving behaviour when no valid config was dispatched. - // Servers send true/false or omit the field; null is not a value they emit. - assertThrows(Exception.class, () -> parse(wrap(configWithNullObserveFullEvaluationData()))); + void observeFullEvaluationDataExplicitNullDefaultsToFalseWithoutRejectingConfig() + throws Exception { + // An explicit null (or a wrong-typed value) for this field must not abort the whole UFC parse. + // A pod that starts after a malformed UFC has no last-known-good, so aborting would strand + // every flag on PROVIDER_NOT_READY (its default value). We fail closed on privacy (consent + // stays false) but preserve availability: flags parse and evaluate. + final ServerConfiguration configuration = + parse(wrap(configWithRawObserveFullEvaluationData("null"))); + + assertNotNull(configuration); + assertFalse( + configuration.observeFullEvaluationData != null && configuration.observeFullEvaluationData); + assertNotNull(configuration.flags); + } + + @Test + void observeFullEvaluationDataWrongTypedStringDefaultsToFalse() throws Exception { + // Moshi tolerates a stringified boolean like "true" via nullSafe/boxed handling: it either + // parses as null or throws locally and leaves the field null. Either way, downstream reads + // via Boolean.TRUE.equals(...) treat it as consent-off. The rest of the config must parse. + final ServerConfiguration configuration = + parse(wrap(configWithRawObserveFullEvaluationData("\"true\""))); + + assertNotNull(configuration); + assertFalse( + configuration.observeFullEvaluationData != null && configuration.observeFullEvaluationData); + assertNotNull(configuration.flags); + } + + @Test + void observeFullEvaluationDataWrongTypedNumberDefaultsToFalse() throws Exception { + final ServerConfiguration configuration = + parse(wrap(configWithRawObserveFullEvaluationData("1"))); + + assertNotNull(configuration); + assertFalse( + configuration.observeFullEvaluationData != null && configuration.observeFullEvaluationData); + assertNotNull(configuration.flags); } private static ServerConfiguration parse(final String json) throws Exception { @@ -124,10 +157,15 @@ private static String configWithObserveFullEvaluationData(final boolean value) { + "}"; } - private static String configWithNullObserveFullEvaluationData() { + private static String configWithRawObserveFullEvaluationData(final String rawJsonValue) { + // Emit the field with a caller-controlled raw JSON value (null / "true" / 1 / ...) so we can + // assert the parser's tolerance of malformed shapes without going through configWith's + // boolean-typed helper. return "{" + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"observeFullEvaluationData\":null," + + "\"observeFullEvaluationData\":" + + rawJsonValue + + "," + "\"environment\":{\"name\":\"Test\"}," + "\"flags\":{}" + "}"; From 4d94921d0d3df3d411ae5945166ecb4e8e1585c1 Mon Sep 17 00:00:00 2001 From: Vickie Boettcher Date: Wed, 5 Aug 2026 11:30:47 -0400 Subject: [PATCH 18/19] Set observeFullEvaluationData=true for the NaN-poison flush test The consent-off short-circuit in FlagEvaluationEvent.fromBucket drops the raw context before Moshi encodes it, so a NaN in the attrs never reaches the encoder and the flush succeeds. That defeated the intent of encodeFailureClearsAggregatorSoLaterFlushesRecover, which must observe a real encode failure to prove the aggregator is cleared. Co-Authored-By: Claude --- .../com/datadog/featureflag/FlagEvaluationWriterImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index b872560009e..c5b39f13a36 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -676,7 +676,7 @@ void encodeFailureClearsAggregatorSoLaterFlushesRecover() throws Exception { // bucket. Before the fix, the aggregator kept the bucket and every later flush re-threw. final Map poison = new HashMap<>(); poison.put("bad-number", Double.NaN); - setup.handler.add(event("poison-flag", "on", "alloc1", "user-1", 1000L, poison)); + setup.handler.add(event("poison-flag", "on", "alloc1", "user-1", 1000L, true, poison)); setup.handler.drainAndAggregate(); setup.handler.flush(); verify(mockEvp, org.mockito.Mockito.never()) From d387c3bbcf3114bba034885ad8d39207d00f6480 Mon Sep 17 00:00:00 2001 From: "vickie.fridge" Date: Wed, 5 Aug 2026 16:25:50 +0000 Subject: [PATCH 19/19] Cover LenientBooleanAdapter read-only and qualifier paths The per-class JaCoCo gate (0.9 minimum, gradle/jacoco.gradle) failed on the new adapter: toJson was never invoked (20/25 instructions) and the factory's !annotations.isEmpty() short-circuit never evaluated true (3/4 branches). Neither path is reachable through the parse-driven tests in JsonApiUfcResponseParserTest. Mirror the tests the sibling FlagMapAdapter and DateAdapter already have. The primitive-boolean assertion documents the guard that keeps this leniency off mandatory fields like Flag.enabled. Environment: Datadog workspace Co-Authored-By: Claude Opus 5 --- .../RemoteConfigServiceImplTest.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/RemoteConfigServiceImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/RemoteConfigServiceImplTest.java index c1f5ef17b87..7f13091cd2f 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/RemoteConfigServiceImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/RemoteConfigServiceImplTest.java @@ -206,6 +206,35 @@ void flagMapAdapterFactoryOnlyCreatesFlagMapAdapterForFlagMapType() { flagsType, singleton(mock(Annotation.class)), moshi)); } + @Test + void lenientBooleanAdapterFactoryOnlyCreatesAdapterForUnannotatedBoxedBoolean() { + final Moshi moshi = moshi(); + + final JsonAdapter adapter = + UniversalFlagConfigParser.LenientBooleanAdapter.FACTORY.create( + Boolean.class, emptySet(), moshi); + + assertNotNull(adapter); + assertTrue(adapter instanceof UniversalFlagConfigParser.LenientBooleanAdapter); + // Primitive boolean keeps Moshi's strict adapter so mandatory fields still reject bad values. + assertNull( + UniversalFlagConfigParser.LenientBooleanAdapter.FACTORY.create( + boolean.class, emptySet(), moshi)); + // A qualified Boolean belongs to whichever adapter declared the qualifier, not to this one. + assertNull( + UniversalFlagConfigParser.LenientBooleanAdapter.FACTORY.create( + Boolean.class, singleton(mock(Annotation.class)), moshi)); + } + + @Test + void lenientBooleanAdapterIsReadOnly() { + final UniversalFlagConfigParser.LenientBooleanAdapter adapter = + new UniversalFlagConfigParser.LenientBooleanAdapter(); + + assertThrows( + UnsupportedOperationException.class, () -> adapter.toJson(mock(JsonWriter.class), true)); + } + @Test void allowsNullFlagMap() throws Exception { final ServerConfiguration config =