diff --git a/products/feature-flagging/feature-flagging-api/build.gradle.kts b/products/feature-flagging/feature-flagging-api/build.gradle.kts index 298b39d4092..a787e7b3fd7 100644 --- a/products/feature-flagging/feature-flagging-api/build.gradle.kts +++ b/products/feature-flagging/feature-flagging-api/build.gradle.kts @@ -47,6 +47,10 @@ dependencies { compileOnly("io.opentelemetry:opentelemetry-api:1.47.0") testImplementation(project(":products:feature-flagging:feature-flagging-bootstrap")) + // SpanEnrichmentGate resolves FeatureFlaggingConfig at runtime. Without it on the test + // classpath the gate swallows a NoClassDefFoundError and reads as off, so the enrichment + // branch cannot be driven. + testImplementation(project(":products:feature-flagging:feature-flagging-config")) testImplementation(project(":utils:config-utils")) testImplementation("io.opentelemetry:opentelemetry-api:1.47.0") testImplementation(libs.bundles.junit5) 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 a9f51b4a660..9bc55fe63e9 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 @@ -43,15 +43,60 @@ import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { + private static final Logger log = LoggerFactory.getLogger(DDEvaluator.class); private static final Set> SUPPORTED_RESOLUTION_TYPES = new HashSet<>(asList(String.class, Boolean.class, Integer.class, Double.class, Value.class)); + static final AtomicBoolean SPLIT_SERIAL_ID_SUPPORTED = + new AtomicBoolean(splitSerialIdSupported(Split.class)); + + static final AtomicBoolean USE_LEGACY_EXPOSURE_API = + new AtomicBoolean( + !(SPLIT_SERIAL_ID_SUPPORTED.get() && exposureSerialIdSupported(ExposureEvent.class))); + + static boolean splitSerialIdSupported(final Class splitClass) { + try { + return splitClass.getField("serialId").getType() == Integer.class; + } catch (final NoSuchFieldException | LinkageError | RuntimeException e) { + log.warn( + "Feature flag serial ID reporting is unavailable with the installed Datadog Java " + + "agent, which does not carry a serial id on the flag configuration. Upgrade " + + "dd-java-agent to enable holdout attribution."); + log.debug("Unable to access the flag configuration serial ID", e); + return false; + } + } + + static boolean exposureSerialIdSupported(final Class eventClass) { + try { + eventClass.getConstructor( + long.class, + datadog.trace.api.featureflag.exposure.Allocation.class, + datadog.trace.api.featureflag.exposure.Flag.class, + datadog.trace.api.featureflag.exposure.Variant.class, + Subject.class, + Integer.class); + return true; + } catch (final NoSuchMethodException | LinkageError | RuntimeException e) { + log.warn( + "Feature flag exposure serial ID reporting is unavailable with the installed " + + "Datadog Java agent. Exposures are still reported, without the serial id, and " + + "span enrichment is unaffected. Upgrade dd-java-agent to enable holdout " + + "attribution on exposures."); + log.debug("Unable to access the exposure serial ID constructor", e); + return false; + } + } + /** * Maximum evaluation-context nesting depth captured on the hot path. Recursion runs on the * caller's evaluation thread over a caller-owned Value tree, so an arbitrarily deep @@ -557,7 +602,7 @@ private static ProviderEvaluation resolveVariant( // present (when enrichment is on) so the span-enrichment hook can decide whether to record the // subject. if (SPAN_ENRICHMENT_ENABLED) { - if (split.serialId != null) { + if (SPLIT_SERIAL_ID_SUPPORTED.get() && split.serialId != null) { metadataBuilder.addInteger(METADATA_SPLIT_SERIAL_ID, split.serialId); } metadataBuilder.addBoolean(METADATA_DO_LOG, allocation.doLog != null && allocation.doLog); @@ -576,7 +621,7 @@ private static ProviderEvaluation resolveVariant( .build(); final boolean doLog = allocation.doLog != null && allocation.doLog; if (doLog) { - dispatchExposure(key, result, context); + dispatchExposure(key, result, context, split); } return result; } @@ -649,20 +694,30 @@ private static Double parseDouble(final Object value) { } private static void dispatchExposure( - final String flag, final ProviderEvaluation evaluation, final EvaluationContext context) { + final String flag, + final ProviderEvaluation evaluation, + final EvaluationContext context, + final Split split) { final String allocationKey = allocationKey(evaluation); final String variantKey = evaluation.getVariant(); if (allocationKey == null || variantKey == null) { return; } - final ExposureEvent event = - new ExposureEvent( - System.currentTimeMillis(), - new datadog.trace.api.featureflag.exposure.Allocation(allocationKey), - new datadog.trace.api.featureflag.exposure.Flag(flag), - new datadog.trace.api.featureflag.exposure.Variant(variantKey), - new Subject(context.getTargetingKey(), flattenContext(context))); + final long timestamp = System.currentTimeMillis(); + // Exposure types share names with the imported UFC Allocation, Flag, and Variant types. + final datadog.trace.api.featureflag.exposure.Allocation allocation = + new datadog.trace.api.featureflag.exposure.Allocation(allocationKey); + final datadog.trace.api.featureflag.exposure.Flag exposureFlag = + new datadog.trace.api.featureflag.exposure.Flag(flag); + final datadog.trace.api.featureflag.exposure.Variant variant = + new datadog.trace.api.featureflag.exposure.Variant(variantKey); + final Subject subject = new Subject(context.getTargetingKey(), flattenContext(context)); + final ExposureEvent event = + USE_LEGACY_EXPOSURE_API.get() + ? new ExposureEvent(timestamp, allocation, exposureFlag, variant, subject) + : new ExposureEvent( + timestamp, allocation, exposureFlag, variant, subject, split.serialId); FeatureFlaggingGateway.dispatch(event); } diff --git a/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorSpanEnrichmentForkedTest.java b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorSpanEnrichmentForkedTest.java new file mode 100644 index 00000000000..2e5a4c483bf --- /dev/null +++ b/products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorSpanEnrichmentForkedTest.java @@ -0,0 +1,98 @@ +package datadog.trace.api.openfeature; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.mock; + +import datadog.trace.api.featureflag.ufc.v1.Allocation; +import datadog.trace.api.featureflag.ufc.v1.Flag; +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.EvaluationContext; +import dev.openfeature.sdk.MutableContext; +import dev.openfeature.sdk.ProviderEvaluation; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.Test; + +/** + * Drives the span-enrichment branch of {@link DDEvaluator}, which the ordinary test task cannot + * reach: the gate is read once into a static final field at class load. The property is set here + * rather than through {@code @WithConfig} because that extension rewrites {@code Config.INSTANCE}, + * which this module does not use, and the forked task gives this class its own JVM where nothing + * has loaded the evaluator yet. + */ +class DDEvaluatorSpanEnrichmentForkedTest { + + static { + System.setProperty("dd.experimental.flagging.provider.span.enrichment.enabled", "true"); + } + + @Test + void enrichmentMetadataCarriesTheSplitSerialId() { + final ProviderEvaluation result = evaluate(340132); + + assertNotNull( + result.getFlagMetadata().getBoolean(DDEvaluator.METADATA_DO_LOG), + "span enrichment must be on, or the assertions below pass vacuously"); + assertEquals( + Integer.valueOf(340132), + result.getFlagMetadata().getInteger(DDEvaluator.METADATA_SPLIT_SERIAL_ID)); + } + + /** + * Simulates legacy exposure support while Split.serialId remains available. Falling back to the + * five-argument exposure constructor must not suppress the serial id in enrichment metadata. + */ + @Test + void enrichmentMetadataSurvivesAnAgentWithoutTheExposureConstructor() { + final boolean previous = DDEvaluator.USE_LEGACY_EXPOSURE_API.getAndSet(true); + try { + assertEquals( + Integer.valueOf(340132), + evaluate(340132).getFlagMetadata().getInteger(DDEvaluator.METADATA_SPLIT_SERIAL_ID)); + } finally { + DDEvaluator.USE_LEGACY_EXPOSURE_API.set(previous); + } + } + + @Test + void enrichmentMetadataOmitsTheSerialIdWhenTheAgentSplitHasNoField() { + final boolean previous = DDEvaluator.SPLIT_SERIAL_ID_SUPPORTED.getAndSet(false); + try { + assertNull( + evaluate(340132).getFlagMetadata().getInteger(DDEvaluator.METADATA_SPLIT_SERIAL_ID)); + } finally { + DDEvaluator.SPLIT_SERIAL_ID_SUPPORTED.set(previous); + } + } + + @Test + void enrichmentMetadataOmitsTheSerialIdWhenTheSplitHasNone() { + assertNull(evaluate(null).getFlagMetadata().getInteger(DDEvaluator.METADATA_SPLIT_SERIAL_ID)); + } + + private static ProviderEvaluation evaluate(final Integer serialId) { + final Map variations = new HashMap<>(); + variations.put("on", new Variant("on", 1)); + final Split split = new Split(emptyList(), "on", emptyMap(), serialId); + final Allocation allocation = + new Allocation("alloc-1", null, null, null, singletonList(split), Boolean.FALSE); + final Map flags = new HashMap<>(); + flags.put( + "target", + new Flag("target", true, ValueType.INTEGER, variations, singletonList(allocation))); + + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + evaluator.accept(new ServerConfiguration("", "", true, null, flags)); + + final EvaluationContext ctx = new MutableContext("target").setTargetingKey("user-1"); + return evaluator.evaluate(Integer.class, "target", 23, ctx); + } +} 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 aa4fe133c0d..940c84998d4 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,9 +12,11 @@ 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.assertEquals; 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.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -26,6 +28,8 @@ import com.squareup.moshi.Moshi; import com.squareup.moshi.Types; import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +import datadog.trace.api.featureflag.exposure.Subject; import datadog.trace.api.featureflag.ufc.v1.Allocation; import datadog.trace.api.featureflag.ufc.v1.ConditionConfiguration; import datadog.trace.api.featureflag.ufc.v1.ConditionOperator; @@ -394,6 +398,116 @@ public void observeFullEvaluationDataNullConfigFieldTreatedAsFalse() { equalTo(false)); } + // ---- exposure events carry the split's serial id ---- + + @Test + public void exposureCarriesTheSplitSerialId() { + assertEquals(Integer.valueOf(340132), exposureFor(340132).serial_id); + } + + @Test + public void exposureCarriesSerialIdZero() { + assertEquals(Integer.valueOf(0), exposureFor(0).serial_id); + } + + @Test + public void exposureOmitsSerialIdWhenTheSplitHasNone() { + assertNull(exposureFor(null).serial_id); + } + + @Test + public void legacyExposureApiDispatchesAnExposureWithoutSerialId() { + final boolean previous = DDEvaluator.USE_LEGACY_EXPOSURE_API.getAndSet(true); + try { + assertNull(exposureFor(7).serial_id); + } finally { + DDEvaluator.USE_LEGACY_EXPOSURE_API.set(previous); + } + } + + // ---- old-agent bootstrap probe ---- + + /** A Split from an agent that predates the serial id: the field does not exist. */ + static final class SplitWithoutSerialId {} + + /** A Split whose serialId is not the Integer the dispatch site reads. */ + static final class SplitWithWrongSerialIdType { + public long serialId; + } + + /** An ExposureEvent from an agent that predates the serial id: only the five-arg constructor. */ + static final class LegacyExposureEvent { + // Qualify exposure types that share names with the imported UFC types. + LegacyExposureEvent( + final long timestamp, + final datadog.trace.api.featureflag.exposure.Allocation allocation, + final datadog.trace.api.featureflag.exposure.Flag flag, + final datadog.trace.api.featureflag.exposure.Variant variant, + final Subject subject) {} + } + + /** + * Positive control. The probe must agree with the bootstrap actually on the classpath, or the + * negative cases below would pass for the wrong reason and the feature would ship switched off. + */ + @Test + public void probeAcceptsTheBootstrapOnTheClasspath() { + assertTrue(DDEvaluator.splitSerialIdSupported(Split.class)); + assertTrue(DDEvaluator.exposureSerialIdSupported(ExposureEvent.class)); + assertTrue(DDEvaluator.SPLIT_SERIAL_ID_SUPPORTED.get()); + assertFalse(DDEvaluator.USE_LEGACY_EXPOSURE_API.get()); + } + + @Test + public void probeRejectsAnAgentWhoseSplitHasNoSerialId() { + assertFalse(DDEvaluator.splitSerialIdSupported(SplitWithoutSerialId.class)); + } + + @Test + public void probeRejectsAnAgentWhoseSerialIdIsNotAnInteger() { + assertFalse(DDEvaluator.splitSerialIdSupported(SplitWithWrongSerialIdType.class)); + } + + @Test + public void probeRejectsAnAgentWithoutTheSerialIdConstructor() { + assertFalse(DDEvaluator.exposureSerialIdSupported(LegacyExposureEvent.class)); + } + + /** + * Tests legacy bootstrap behavior when Split.serialId exists but the exposure event has only the + * five-argument constructor. Split support must remain independent of exposure constructor + * support. + */ + @Test + public void probeKeepsSplitSupportWhenOnlyTheEventConstructorIsMissing() { + assertTrue(DDEvaluator.splitSerialIdSupported(Split.class)); + assertFalse(DDEvaluator.exposureSerialIdSupported(LegacyExposureEvent.class)); + } + + /** + * Evaluates a logging allocation whose split carries the given serial id and returns the single + * dispatched exposure. Span enrichment is off here, as it is by default, so this also pins that + * the serial id does not travel via the enrichment-gated evaluation metadata. + */ + private static ExposureEvent exposureFor(final Integer serialId) { + final List dispatched = new ArrayList<>(); + final FeatureFlaggingGateway.ExposureListener listener = dispatched::add; + FeatureFlaggingGateway.addExposureListener(listener); + try { + final Map variations = new HashMap<>(); + variations.put("on", new Variant("on", 1)); + final Split split = new Split(emptyList(), "on", emptyMap(), serialId); + final Allocation allocation = + new Allocation("alloc-1", null, null, null, singletonList(split), Boolean.TRUE); + evaluateFlag( + new Flag("target", true, ValueType.INTEGER, variations, singletonList(allocation)), true); + } finally { + FeatureFlaggingGateway.removeExposureListener(listener); + } + assertEquals(1, dispatched.size()); + return dispatched.get(0); + } + // 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/exposure/ExposureEvent.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/exposure/ExposureEvent.java index 38b04a7f406..4101b767760 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/exposure/ExposureEvent.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/exposure/ExposureEvent.java @@ -8,16 +8,29 @@ public class ExposureEvent { public final Variant variant; public final Subject subject; + public final Integer serial_id; + public ExposureEvent( final long timestamp, final Allocation allocation, final Flag flag, final Variant variant, final Subject subject) { + this(timestamp, allocation, flag, variant, subject, null); + } + + public ExposureEvent( + final long timestamp, + final Allocation allocation, + final Flag flag, + final Variant variant, + final Subject subject, + final Integer serialId) { this.timestamp = timestamp; this.allocation = allocation; this.flag = flag; this.variant = variant; this.subject = subject; + this.serial_id = serialId; } } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureCache.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureCache.java index 6fdd06bece7..7125d009a69 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureCache.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureCache.java @@ -38,10 +38,12 @@ public int hashCode() { final class Value { public final String variant; public final String allocation; + public final Integer serialId; public Value(final ExposureEvent event) { this.variant = event.variant == null ? null : event.variant.key; this.allocation = event.allocation == null ? null : event.allocation.key; + this.serialId = event.serial_id; } @Override @@ -50,12 +52,14 @@ public boolean equals(final Object o) { return false; } final Value value = (Value) o; - return Objects.equals(variant, value.variant) && Objects.equals(allocation, value.allocation); + return Objects.equals(variant, value.variant) + && Objects.equals(allocation, value.allocation) + && Objects.equals(serialId, value.serialId); } @Override public int hashCode() { - return Objects.hash(variant, allocation); + return Objects.hash(variant, allocation, serialId); } } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagEvpPublisherTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagEvpPublisherTest.java index 379dd49e444..711e1b90e29 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagEvpPublisherTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagEvpPublisherTest.java @@ -1,7 +1,10 @@ package com.datadog.featureflag; +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; @@ -12,7 +15,14 @@ import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +import datadog.trace.api.featureflag.exposure.ExposuresRequest; +import datadog.trace.api.featureflag.exposure.Flag; +import datadog.trace.api.featureflag.exposure.Subject; +import datadog.trace.api.featureflag.exposure.Variant; import datadog.trace.api.intake.Intake; +import java.nio.charset.StandardCharsets; import okhttp3.RequestBody; import org.junit.jupiter.api.Test; @@ -61,6 +71,53 @@ void postThrowsWhenEvpBackendApiCannotBeCreated() { () -> publisher.post("flagevaluation", FeatureFlagEvpPublisher.utf8Bytes("{}"))); } + @Test + void serializesSerialIdUnderTheIntakeWireKey() { + assertTrue(exposureJson(340132).contains("\"serial_id\":340132")); + } + + @Test + void serializesSerialIdZeroRatherThanOmittingIt() { + assertTrue(exposureJson(0).contains("\"serial_id\":0")); + } + + @Test + void omitsSerialIdKeyWhenAbsent() { + assertFalse(exposureJson(null).contains("serial_id")); + } + + @Test + void omitsSerialIdKeyForAnEventBuiltWithoutOne() { + final ExposureEvent event = + new ExposureEvent( + 1234L, + new Allocation("allocation"), + new Flag("flag"), + new Variant("variant"), + new Subject("subject", emptyMap())); + + assertFalse(exposureJsonOf(event).contains("serial_id")); + } + + private static String exposureJson(final Integer serialId) { + return exposureJsonOf( + new ExposureEvent( + 1234L, + new Allocation("allocation"), + new Flag("flag"), + new Variant("variant"), + new Subject("subject", emptyMap()), + serialId)); + } + + private static String exposureJsonOf(final ExposureEvent event) { + final FeatureFlagEvpPublisher publisher = + new FeatureFlagEvpPublisher<>(mock(BackendApiFactory.class), ExposuresRequest.class); + return new String( + publisher.serialize(new ExposuresRequest(emptyMap(), singletonList(event))), + StandardCharsets.UTF_8); + } + static class TestRequest { public final String value; diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/LRUExposureCacheTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/LRUExposureCacheTest.java index 43c4c2d40f0..7d43c7a8b51 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/LRUExposureCacheTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/LRUExposureCacheTest.java @@ -59,6 +59,52 @@ void testAddingEventsWithSameKeyButDifferentDetailsUpdatesCache() { assertEquals("allocation2", retrieved.allocation); } + @Test + void testSerialIdAppearingEmitsANewExposure() { + assertEquals(2, emissions(null, 7)); + } + + @Test + void testSerialIdDisappearingEmitsANewExposure() { + assertEquals(2, emissions(7, null)); + } + + @Test + void testSerialIdChangingEmitsANewExposure() { + assertEquals(2, emissions(7, 8)); + } + + @Test + void testSerialIdCycleEmitsAnExposurePerTransition() { + assertEquals(3, emissions(7, 8, 7)); + } + + @Test + void testUnchangedSerialIdIsDeduplicated() { + assertEquals(1, emissions(7, 7)); + } + + @Test + void testUnchangedAbsentSerialIdIsDeduplicated() { + assertEquals(1, emissions(null, null)); + } + + @Test + void testSerialIdZeroIsDistinguishedFromAbsent() { + assertEquals(2, emissions(null, 0)); + assertEquals(2, emissions(0, null)); + } + + @Test + void testSerialIdIsRetainedOnTheCachedValue() { + LRUExposureCache cache = new LRUExposureCache(5); + ExposureEvent event = createEvent("flag", "subject", "variant", "allocation", 0); + + cache.add(event); + + assertEquals(Integer.valueOf(0), cache.get(new ExposureCache.Key(event)).serialId); + } + @Test void testLruEvictionWhenCapacityExceeded() { LRUExposureCache cache = new LRUExposureCache(2); @@ -234,11 +280,32 @@ void testDuplicateExposureKeepsSubjectHotInLruOrder() { private static ExposureEvent createEvent( String flag, String subject, String variant, String allocation) { + return createEvent(flag, subject, variant, allocation, null); + } + + private static ExposureEvent createEvent( + String flag, String subject, String variant, String allocation, Integer serialId) { return new ExposureEvent( System.currentTimeMillis(), new Allocation(allocation), new Flag(flag), new Variant(variant), - new Subject(subject, emptyMap())); + new Subject(subject, emptyMap()), + serialId); + } + + /** + * Number of exposures a run of same-flag, same-subject evaluations emits: the cache reports true + * only for the ones it does not suppress as duplicates. + */ + private static int emissions(final Integer... serialIds) { + LRUExposureCache cache = new LRUExposureCache(5); + int emitted = 0; + for (Integer serialId : serialIds) { + if (cache.add(createEvent("flag", "subject", "variant", "allocation", serialId))) { + emitted++; + } + } + return emitted; } } 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 c1da0a11dcc..f37599feeb0 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 @@ -33,14 +33,18 @@ import datadog.trace.api.featureflag.ufc.v1.Flag; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import java.io.IOException; +import java.io.InputStream; import java.lang.annotation.Annotation; import java.lang.reflect.Type; import java.time.Instant; import java.util.Date; import java.util.Map; +import okio.Buffer; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.Mock; @@ -84,34 +88,7 @@ void testNewConfigReceived() throws Exception { @Test void skipsMalformedFlagAllocationsAndKeepsValidFlag() throws Exception { - final ServerConfiguration config = - deserialize( - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":{" - + "\"malformed-flag\":{" - + "\"key\":\"malformed-flag\"," - + "\"enabled\":true," - + "\"variationType\":\"STRING\"," - + "\"variations\":{\"on\":{\"key\":\"on\",\"value\":\"on\"}}," - + "\"allocations\":\"this-is-not-a-list\"" - + "}," - + "\"valid-flag\":{" - + "\"key\":\"valid-flag\"," - + "\"enabled\":true," - + "\"variationType\":\"STRING\"," - + "\"variations\":{\"expected\":{\"key\":\"expected\",\"value\":\"expected\"}}," - + "\"allocations\":[{" - + "\"key\":\"default-allocation\"," - + "\"rules\":[]," - + "\"splits\":[{\"variationKey\":\"expected\",\"shards\":[]}]," - + "\"doLog\":true" - + "}]" - + "}" - + "}" - + "}"); + final ServerConfiguration config = deserialize(resource("malformed-allocations.json")); assertNotNull(config); assertFalse(config.flags.containsKey("malformed-flag")); @@ -119,17 +96,88 @@ void skipsMalformedFlagAllocationsAndKeepsValidFlag() throws Exception { assertEquals("expected", config.flags.get("valid-flag").variations.get("expected").value); } + @Test + void parsesSplitSerialId() throws Exception { + final ServerConfiguration config = deserialize(configWithSerialId("340132")); + + assertNotNull(config); + assertEquals(Integer.valueOf(340132), serialIdOf(config)); + } + + @Test + void parsesSplitSerialIdZero() throws Exception { + final ServerConfiguration config = deserialize(configWithSerialId("0")); + + assertNotNull(config); + assertEquals(Integer.valueOf(0), serialIdOf(config)); + } + + @Test + void parsesAbsentSplitSerialIdAsNull() throws Exception { + final ServerConfiguration config = deserialize(configWithSerialId(null)); + + assertNotNull(config); + assertNull(serialIdOf(config)); + } + + @Test + void parsesNullSplitSerialIdAsNull() throws Exception { + final ServerConfiguration config = deserialize(configWithSerialId("null")); + + assertNotNull(config); + assertNull(serialIdOf(config)); + } + + @Test + void skipsFlagWithUncoercibleSerialIdAndKeepsSiblingFlag() throws Exception { + final ServerConfiguration config = deserialize(configWithSiblingSerialIds("true")); + + assertNotNull(config); + assertFalse(config.flags.containsKey("malformed-flag")); + assertEquals("invalid_flag", config.invalidFlags.get("malformed-flag")); + assertTrue(config.flags.containsKey("valid-flag")); + assertEquals(Integer.valueOf(7), serialIdOf(config)); + } + + /** + * Records how leniently the per-flag value reader coerces a serial id, so a future change to the + * parse path is visible here. The values come from the compiler-validated UFC, so the SDK adds no + * validation of its own; what matters is that a bad one never rejects the sibling flag. + */ + @ParameterizedTest + @CsvSource({"\"340132\", 340132", "1.5, 1", "-1, -1", "2147483648, 2147483647"}) + void coercesSerialIdWithoutRejectingTheFlag(final String wireValue, final int expected) + throws Exception { + final ServerConfiguration config = deserialize(configWithSerialId(wireValue)); + + assertNotNull(config); + assertEquals(Integer.valueOf(expected), serialIdOf(config)); + } + + private static Integer serialIdOf(final ServerConfiguration config) { + return config.flags.get("valid-flag").allocations.get(0).splits.get(0).serialId; + } + + private static String configWithSerialId(final String serialIdJson) throws IOException { + return withSerialId(resource("serial-id.json"), serialIdJson); + } + + /** A malformed serial id must bind to its own flag and leave the sibling flag intact. */ + private static String configWithSiblingSerialIds(final String malformedSerialIdJson) + throws IOException { + return withSerialId(resource("sibling-serial-ids.json"), malformedSerialIdJson); + } + + /** Inserts the raw wire value so the parser sees its original JSON type; null omits the key. */ + private static String withSerialId(final String json, final String serialIdJson) { + return serialIdJson == null + ? json.replace("\"serialId\": \"${serialId}\",", "") + : json.replace("\"${serialId}\"", serialIdJson); + } + @Test void ignoresUnknownTopLevelFields() throws Exception { - final ServerConfiguration config = - deserialize( - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"segments\":{\"new-schema-key\":{\"ignored\":true}}," - + "\"flags\":{}" - + "}"); + final ServerConfiguration config = deserialize(resource("unknown-top-level-fields.json")); assertNotNull(config); assertEquals("2024-04-17T19:40:53.716Z", config.createdAt); @@ -141,29 +189,7 @@ void ignoresUnknownTopLevelFields() throws Exception { @Test void parsesAllocationWindowDatesAsDateFieldsWithInstantAccessors() throws Exception { - final ServerConfiguration config = - deserialize( - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":{" - + "\"dated-flag\":{" - + "\"key\":\"dated-flag\"," - + "\"enabled\":true," - + "\"variationType\":\"STRING\"," - + "\"variations\":{\"expected\":{\"key\":\"expected\",\"value\":\"expected\"}}," - + "\"allocations\":[{" - + "\"key\":\"dated-allocation\"," - + "\"rules\":[]," - + "\"startAt\":\"2023-01-01T01:00:00.123456+01:00\"," - + "\"endAt\":\"2023-01-02T00:00:00.987654Z\"," - + "\"splits\":[{\"variationKey\":\"expected\",\"shards\":[]}]," - + "\"doLog\":true" - + "}]" - + "}" - + "}" - + "}"); + final ServerConfiguration config = deserialize(resource("allocation-window-dates.json")); final Allocation allocation = config.flags.get("dated-flag").allocations.get(0); assertEquals(Date.class, Allocation.class.getField("startAt").getType()); @@ -181,43 +207,7 @@ void rejectsTrailingJson() { @Test void skipsUnknownOperatorFlagAndKeepsValidFlag() throws Exception { - final ServerConfiguration config = - deserialize( - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":{" - + "\"operator-grease-flag\":{" - + "\"key\":\"operator-grease-flag\"," - + "\"enabled\":true," - + "\"variationType\":\"STRING\"," - + "\"variations\":{\"trap\":{\"key\":\"trap\",\"value\":\"trap\"}}," - + "\"allocations\":[{" - + "\"key\":\"grease-allocation\"," - + "\"rules\":[{\"conditions\":[{" - + "\"attribute\":\"country\"," - + "\"operator\":\"not-a-real-operator\"," - + "\"value\":\"anything\"" - + "}]}]," - + "\"splits\":[{\"variationKey\":\"trap\",\"shards\":[]}]," - + "\"doLog\":true" - + "}]" - + "}," - + "\"valid-flag\":{" - + "\"key\":\"valid-flag\"," - + "\"enabled\":true," - + "\"variationType\":\"STRING\"," - + "\"variations\":{\"expected\":{\"key\":\"expected\",\"value\":\"expected\"}}," - + "\"allocations\":[{" - + "\"key\":\"default-allocation\"," - + "\"rules\":[]," - + "\"splits\":[{\"variationKey\":\"expected\",\"shards\":[]}]," - + "\"doLog\":true" - + "}]" - + "}" - + "}" - + "}"); + final ServerConfiguration config = deserialize(resource("unknown-operator.json")); assertNotNull(config); assertFalse(config.flags.containsKey("operator-grease-flag")); @@ -273,14 +263,7 @@ void lenientBooleanAdapterIsReadOnly() { @Test void allowsNullFlagMap() throws Exception { - final ServerConfiguration config = - deserialize( - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":null" - + "}"); + final ServerConfiguration config = deserialize(resource("null-flags.json")); assertNotNull(config); assertNull(config.flags); @@ -288,28 +271,7 @@ void allowsNullFlagMap() throws Exception { @Test void skipsNullFlagAndKeepsValidFlag() throws Exception { - final ServerConfiguration config = - deserialize( - "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":{" - + "\"null-flag\":null," - + "\"valid-flag\":{" - + "\"key\":\"valid-flag\"," - + "\"enabled\":true," - + "\"variationType\":\"STRING\"," - + "\"variations\":{\"expected\":{\"key\":\"expected\",\"value\":\"expected\"}}," - + "\"allocations\":[{" - + "\"key\":\"default-allocation\"," - + "\"rules\":[]," - + "\"splits\":[{\"variationKey\":\"expected\",\"shards\":[]}]," - + "\"doLog\":true" - + "}]" - + "}" - + "}" - + "}"); + final ServerConfiguration config = deserialize(resource("null-flag.json")); assertNotNull(config); assertFalse(config.flags.containsKey("null-flag")); @@ -427,12 +389,15 @@ private static Moshi moshi() { .build(); } - private static String emptyConfig() { - return "{" - + "\"createdAt\":\"2024-04-17T19:40:53.716Z\"," - + "\"format\":\"SERVER\"," - + "\"environment\":{\"name\":\"Test\"}," - + "\"flags\":{}" - + "}"; + private static String emptyConfig() throws IOException { + return resource("empty-config.json"); + } + + private static String resource(final String name) throws IOException { + try (final InputStream stream = + RemoteConfigServiceImplTest.class.getResourceAsStream("/remote-config/" + name)) { + assertNotNull(stream, "Missing remote-config fixture: " + name); + return new Buffer().readFrom(stream).readUtf8(); + } } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/allocation-window-dates.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/allocation-window-dates.json new file mode 100644 index 00000000000..26d0a90da2a --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/allocation-window-dates.json @@ -0,0 +1,35 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": { + "dated-flag": { + "key": "dated-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "expected": { + "key": "expected", + "value": "expected" + } + }, + "allocations": [ + { + "key": "dated-allocation", + "rules": [], + "startAt": "2023-01-01T01:00:00.123456+01:00", + "endAt": "2023-01-02T00:00:00.987654Z", + "splits": [ + { + "variationKey": "expected", + "shards": [] + } + ], + "doLog": true + } + ] + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/empty-config.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/empty-config.json new file mode 100644 index 00000000000..51b021e5de7 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/empty-config.json @@ -0,0 +1,8 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": {} +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/malformed-allocations.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/malformed-allocations.json new file mode 100644 index 00000000000..93314b69e7d --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/malformed-allocations.json @@ -0,0 +1,45 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": { + "malformed-flag": { + "key": "malformed-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "on": { + "key": "on", + "value": "on" + } + }, + "allocations": "this-is-not-a-list" + }, + "valid-flag": { + "key": "valid-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "expected": { + "key": "expected", + "value": "expected" + } + }, + "allocations": [ + { + "key": "default-allocation", + "rules": [], + "splits": [ + { + "variationKey": "expected", + "shards": [] + } + ], + "doLog": true + } + ] + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/null-flag.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/null-flag.json new file mode 100644 index 00000000000..a4f3a6c2502 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/null-flag.json @@ -0,0 +1,34 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": { + "null-flag": null, + "valid-flag": { + "key": "valid-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "expected": { + "key": "expected", + "value": "expected" + } + }, + "allocations": [ + { + "key": "default-allocation", + "rules": [], + "splits": [ + { + "variationKey": "expected", + "shards": [] + } + ], + "doLog": true + } + ] + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/null-flags.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/null-flags.json new file mode 100644 index 00000000000..a083526e553 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/null-flags.json @@ -0,0 +1,8 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": null +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/serial-id.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/serial-id.json new file mode 100644 index 00000000000..6b7dc926854 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/serial-id.json @@ -0,0 +1,34 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": { + "valid-flag": { + "key": "valid-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "expected": { + "key": "expected", + "value": "expected" + } + }, + "allocations": [ + { + "key": "default-allocation", + "rules": [], + "splits": [ + { + "serialId": "${serialId}", + "variationKey": "expected", + "shards": [] + } + ], + "doLog": true + } + ] + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/sibling-serial-ids.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/sibling-serial-ids.json new file mode 100644 index 00000000000..aa791903f66 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/sibling-serial-ids.json @@ -0,0 +1,59 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": { + "malformed-flag": { + "key": "malformed-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "on": { + "key": "on", + "value": "on" + } + }, + "allocations": [ + { + "key": "default-allocation", + "rules": [], + "splits": [ + { + "serialId": "${serialId}", + "variationKey": "on", + "shards": [] + } + ], + "doLog": true + } + ] + }, + "valid-flag": { + "key": "valid-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "expected": { + "key": "expected", + "value": "expected" + } + }, + "allocations": [ + { + "key": "default-allocation", + "rules": [], + "splits": [ + { + "serialId": 7, + "variationKey": "expected", + "shards": [] + } + ], + "doLog": true + } + ] + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/unknown-operator.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/unknown-operator.json new file mode 100644 index 00000000000..a380463b1d5 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/unknown-operator.json @@ -0,0 +1,67 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "flags": { + "operator-grease-flag": { + "key": "operator-grease-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "trap": { + "key": "trap", + "value": "trap" + } + }, + "allocations": [ + { + "key": "grease-allocation", + "rules": [ + { + "conditions": [ + { + "attribute": "country", + "operator": "not-a-real-operator", + "value": "anything" + } + ] + } + ], + "splits": [ + { + "variationKey": "trap", + "shards": [] + } + ], + "doLog": true + } + ] + }, + "valid-flag": { + "key": "valid-flag", + "enabled": true, + "variationType": "STRING", + "variations": { + "expected": { + "key": "expected", + "value": "expected" + } + }, + "allocations": [ + { + "key": "default-allocation", + "rules": [], + "splits": [ + { + "variationKey": "expected", + "shards": [] + } + ], + "doLog": true + } + ] + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/unknown-top-level-fields.json b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/unknown-top-level-fields.json new file mode 100644 index 00000000000..fe23d4a46b1 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/resources/remote-config/unknown-top-level-fields.json @@ -0,0 +1,13 @@ +{ + "createdAt": "2024-04-17T19:40:53.716Z", + "format": "SERVER", + "environment": { + "name": "Test" + }, + "segments": { + "new-schema-key": { + "ignored": true + } + }, + "flags": {} +}