diff --git a/products/feature-flagging/feature-flagging-api/src/jmh/java/datadog/trace/api/openfeature/FlagEvaluationRuleBenchmark.java b/products/feature-flagging/feature-flagging-api/src/jmh/java/datadog/trace/api/openfeature/FlagEvaluationRuleBenchmark.java new file mode 100644 index 00000000000..ce5d1de8aad --- /dev/null +++ b/products/feature-flagging/feature-flagging-api/src/jmh/java/datadog/trace/api/openfeature/FlagEvaluationRuleBenchmark.java @@ -0,0 +1,120 @@ +package datadog.trace.api.openfeature; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; + +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.Shard; +import datadog.trace.api.featureflag.ufc.v1.ShardRange; +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.MutableContext; +import java.util.List; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +/** + * Measures evaluator rule costs that run on the application thread. + * + *

The static case is the control. The regex and shard cases differ only in the rule work that + * selects the same boolean variation. + * + *

Run: {@code ./gradlew :products:feature-flagging:feature-flagging-api:jmh + * -PjmhIncludes=FlagEvaluationRuleBenchmark -PjmhProf=gc}. + */ +@State(Scope.Benchmark) +@Warmup(iterations = 3, time = 2, timeUnit = SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = SECONDS) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(NANOSECONDS) +@Fork(1) +public class FlagEvaluationRuleBenchmark { + + private DDEvaluator staticEvaluator; + private DDEvaluator regexEvaluator; + private DDEvaluator shardEvaluator; + private MutableContext context; + + @Setup + public void setUp() { + context = new MutableContext("benchmark-subject-0123456789"); + context.add("email", "benchmark@datadoghq.com"); + + staticEvaluator = evaluator(staticAllocation()); + regexEvaluator = evaluator(regexAllocation()); + shardEvaluator = evaluator(shardAllocation()); + } + + @Benchmark + public void staticRule(final Blackhole blackhole) { + blackhole.consume(staticEvaluator.evaluate(Boolean.class, "bench", false, context)); + } + + @Benchmark + public void regexRule(final Blackhole blackhole) { + blackhole.consume(regexEvaluator.evaluate(Boolean.class, "bench", false, context)); + } + + @Benchmark + public void shardRule(final Blackhole blackhole) { + blackhole.consume(shardEvaluator.evaluate(Boolean.class, "bench", false, context)); + } + + private static DDEvaluator evaluator(final Allocation allocation) { + final Flag flag = + new Flag( + "bench", + true, + ValueType.BOOLEAN, + singletonMap("on", new Variant("on", true)), + singletonList(allocation)); + final DDEvaluator evaluator = new DDEvaluator(() -> {}); + evaluator.accept(new ServerConfiguration("", "", false, null, singletonMap("bench", flag))); + return evaluator; + } + + private static Allocation staticAllocation() { + return allocation(emptyList(), new Split(emptyList(), "on", null, null)); + } + + private static Allocation regexAllocation() { + final ConditionConfiguration condition = + new ConditionConfiguration( + ConditionOperator.MATCHES, "email", "^[[:alnum:]._%+-]+@datadoghq[.]com$"); + condition.cacheRegexPattern(); + return allocation(singletonList(new Rule(singletonList(condition))), staticSplit()); + } + + private static Allocation shardAllocation() { + final Shard shard = + new Shard("benchmark-allocation-salt", singletonList(new ShardRange(0, 100_000)), 100_000); + return allocation(emptyList(), new Split(singletonList(shard), "on", null, null)); + } + + private static Split staticSplit() { + return new Split(emptyList(), "on", null, null); + } + + private static Allocation allocation(final List rules, final Split split) { + return new Allocation( + "benchmark-allocation", rules, null, null, singletonList(split), Boolean.FALSE); + } +} 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 e74074a640a..c1f7d749fb2 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 @@ -44,7 +44,6 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; -import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { @@ -110,6 +109,18 @@ class DDEvaluator implements Evaluator, FeatureFlaggingGateway.ConfigListener { // change at runtime, and this class is loaded lazily (well after startup) so config is ready. private static final boolean SPAN_ENRICHMENT_ENABLED = SpanEnrichmentGate.isEnabled(); + // MessageDigest is mutable. One instance per evaluation thread avoids shared mutation and the + // provider lookup and digest allocation on every shard evaluation. + private static final ThreadLocal MD5 = + ThreadLocal.withInitial( + () -> { + try { + return MessageDigest.getInstance("MD5"); + } catch (final NoSuchAlgorithmException e) { + throw new IllegalStateException("MD5 algorithm not available", e); + } + }); + private final Runnable configCallback; private final AtomicReference configuration = new AtomicReference<>(); private final CountDownLatch initializationLatch = new CountDownLatch(1); @@ -361,9 +372,9 @@ private static boolean evaluateCondition( switch (condition.operator) { case MATCHES: - return matchesRegex(attributeValue, condition.value); + return matchesRegex(attributeValue, condition); case NOT_MATCHES: - return !matchesRegex(attributeValue, condition.value); + return !matchesRegex(attributeValue, condition); case ONE_OF: return isOneOf(attributeValue, condition.value); case NOT_ONE_OF: @@ -393,21 +404,11 @@ private static boolean evaluateCondition( } } - private static boolean matchesRegex(final Object attributeValue, final Object conditionValue) { + private static boolean matchesRegex( + final Object attributeValue, final ConditionConfiguration condition) { // PatternSyntaxException is intentionally not caught here so it propagates to evaluate(), // which maps it to ErrorCode.PARSE_ERROR. - final Pattern pattern = Pattern.compile(normalizeRegex(String.valueOf(conditionValue))); - return pattern.matcher(String.valueOf(attributeValue)).find(); - } - - private static String normalizeRegex(final String regex) { - return regex - .replace("[:alnum:]", "\\p{Alnum}") - .replace("[:alpha:]", "\\p{Alpha}") - .replace("[:digit:]", "\\p{Digit}") - .replace("[:lower:]", "\\p{Lower}") - .replace("[:upper:]", "\\p{Upper}") - .replace("[:space:]", "\\p{Space}"); + return condition.regexPattern().matcher(String.valueOf(attributeValue)).find(); } private static boolean isOneOf(final Object attributeValue, final Object conditionValue) { @@ -461,7 +462,7 @@ private static boolean evaluateSemverCondition( } private static boolean matchesShard(final Shard shard, final String targetingKey) { - final int assignedShard = getShard(shard.salt, targetingKey, shard.totalShards); + final int assignedShard = getShard(shard, targetingKey); for (final ShardRange range : shard.ranges) { if (assignedShard >= range.start && assignedShard < range.end) { return true; @@ -470,30 +471,17 @@ private static boolean matchesShard(final Shard shard, final String targetingKey return false; } - private static int getShard(final String salt, final String targetingKey, final int totalShards) { - final String hashKey = salt + "-" + targetingKey; - final String md5Hash = getMD5Hash(hashKey); - final String first8Chars = md5Hash.substring(0, Math.min(8, md5Hash.length())); - final long intFromHash = Long.parseLong(first8Chars, 16); - return (int) (intFromHash % totalShards); - } - - private static String getMD5Hash(final String input) { - try { - final MessageDigest md = MessageDigest.getInstance("MD5"); - final byte[] hashBytes = md.digest(input.getBytes(StandardCharsets.UTF_8)); - final StringBuilder hexString = new StringBuilder(); - for (byte b : hashBytes) { - final String hex = Integer.toHexString(0xff & b); - if (hex.length() == 1) { - hexString.append('0'); - } - hexString.append(hex); - } - return hexString.toString(); - } catch (NoSuchAlgorithmException e) { - throw new RuntimeException("MD5 algorithm not available", e); - } + static int getShard(final Shard shard, final String targetingKey) { + final MessageDigest digest = MD5.get(); + digest.reset(); + shard.updateDigest(digest); + final byte[] hash = digest.digest(targetingKey.getBytes(StandardCharsets.UTF_8)); + final long firstFourBytes = + ((hash[0] & 0xffL) << 24) + | ((hash[1] & 0xffL) << 16) + | ((hash[2] & 0xffL) << 8) + | (hash[3] & 0xffL); + return (int) (firstFourBytes % shard.totalShards); } private static ProviderEvaluation resolveVariant( 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 aeddda0bfd2..d78bd211304 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 @@ -33,6 +33,7 @@ import datadog.trace.api.featureflag.ufc.v1.ParsedSemver; import datadog.trace.api.featureflag.ufc.v1.Rule; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; +import datadog.trace.api.featureflag.ufc.v1.Shard; import datadog.trace.api.featureflag.ufc.v1.Split; import datadog.trace.api.featureflag.ufc.v1.ValueType; import datadog.trace.api.featureflag.ufc.v1.Variant; @@ -62,6 +63,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; public class DDEvaluatorTest { @@ -616,6 +618,15 @@ public void testEvaluateSemverConditionInvalidComparandReturnsParseError() { assertThat(details.getErrorCode(), equalTo(ErrorCode.PARSE_ERROR)); } + @ParameterizedTest + @CsvSource({"eve,732", "user-1,2895", "alice,9136", "bob,8956"}) + public void testShardCalculationMatchesGoAndEppoFixtures( + final String targetingKey, final int expectedShard) { + final Shard shard = new Shard("split-numeric-flag-some-allocation", emptyList(), 10_000); + + assertThat(DDEvaluator.getShard(shard, targetingKey), equalTo(expectedShard)); + } + private static Arguments[] flatteningTestCases() { final List arguments = new ArrayList<>(); arguments.add(Arguments.of(emptyMap(), emptyMap())); diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ConditionConfiguration.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ConditionConfiguration.java index ae693674acc..efcdaef3d07 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ConditionConfiguration.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/ConditionConfiguration.java @@ -1,5 +1,7 @@ package datadog.trace.api.featureflag.ufc.v1; +import java.util.regex.Pattern; + public class ConditionConfiguration { public final ConditionOperator operator; public final String attribute; @@ -9,10 +11,44 @@ public class ConditionConfiguration { // (not from JSON) when the operator is a SEMVER_* operator. public transient ParsedSemver semverComparand; + // The compiled MATCHES or NOT_MATCHES value. Set during configuration preprocessing. Pattern is + // immutable and safe to share between concurrent evaluation threads. + private transient Pattern regexPattern; + public ConditionConfiguration( final ConditionOperator operator, final String attribute, final Object value) { this.operator = operator; this.attribute = attribute; this.value = value; } + + /** Compiles and caches this condition's normalized regular expression. */ + public void cacheRegexPattern() { + regexPattern = compileRegex(); + } + + /** Returns the cached pattern, or compiles one for a condition created outside the UFC parser. */ + public Pattern regexPattern() { + final Pattern cached = regexPattern; + return cached != null ? cached : compileRegex(); + } + + /** Returns true when configuration preprocessing cached this condition's pattern. */ + public boolean hasCachedRegexPattern() { + return regexPattern != null; + } + + private Pattern compileRegex() { + return Pattern.compile(normalizeRegex(String.valueOf(value))); + } + + private static String normalizeRegex(final String regex) { + return regex + .replace("[:alnum:]", "\\p{Alnum}") + .replace("[:alpha:]", "\\p{Alpha}") + .replace("[:digit:]", "\\p{Digit}") + .replace("[:lower:]", "\\p{Lower}") + .replace("[:upper:]", "\\p{Upper}") + .replace("[:space:]", "\\p{Space}"); + } } diff --git a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/Shard.java b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/Shard.java index b94fead2581..bd8c37d2f8d 100644 --- a/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/Shard.java +++ b/products/feature-flagging/feature-flagging-bootstrap/src/main/java/datadog/trace/api/featureflag/ufc/v1/Shard.java @@ -1,5 +1,7 @@ package datadog.trace.api.featureflag.ufc.v1; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; import java.util.List; public class Shard { @@ -7,9 +9,26 @@ public class Shard { public final List ranges; public final int totalShards; + // The immutable UTF-8 bytes before the targeting key in the shard hash input. Set during + // configuration preprocessing. The array is private and is never exposed or mutated. + private transient byte[] saltPrefix; + public Shard(final String salt, final List ranges, final int totalShards) { this.salt = salt; this.ranges = ranges; this.totalShards = totalShards; + cacheSaltPrefix(); + } + + /** Caches the UTF-8 bytes for {@code salt + "-"}. */ + public void cacheSaltPrefix() { + saltPrefix = (String.valueOf(salt) + "-").getBytes(StandardCharsets.UTF_8); + } + + /** Adds the cached salt prefix to a digest without exposing the mutable byte array. */ + public void updateDigest(final MessageDigest digest) { + final byte[] cached = saltPrefix; + digest.update( + cached != null ? cached : (String.valueOf(salt) + "-").getBytes(StandardCharsets.UTF_8)); } } 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 8044160e459..bb9054b5a2f 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 @@ -13,6 +13,7 @@ import datadog.trace.api.featureflag.ufc.v1.ParsedSemver; import datadog.trace.api.featureflag.ufc.v1.Rule; import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; +import datadog.trace.api.featureflag.ufc.v1.Shard; import datadog.trace.api.featureflag.ufc.v1.Split; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -24,6 +25,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.PatternSyntaxException; import javax.annotation.Nonnull; import javax.annotation.Nullable; import okio.BufferedSource; @@ -105,9 +107,55 @@ private static void validateFlag(final String flagKey, final Flag flag) { } } } + cacheEvaluationData(flag); validateAndCacheSemverComparands(flagKey, flag); } + /** Caches immutable rule data used by each evaluation. */ + private static void cacheEvaluationData(final Flag flag) { + for (final Allocation allocation : flag.allocations) { + if (allocation == null) { + continue; + } + if (allocation.rules != null) { + for (final Rule rule : allocation.rules) { + if (rule == null || rule.conditions == null) { + continue; + } + for (final ConditionConfiguration condition : rule.conditions) { + if (condition == null || condition.operator == null) { + continue; + } + switch (condition.operator) { + case MATCHES: + case NOT_MATCHES: + try { + condition.cacheRegexPattern(); + } catch (final PatternSyntaxException ignored) { + // Keep the flag. Evaluation maps this same invalid expression to PARSE_ERROR. + } + break; + default: + break; + } + } + } + } + if (allocation.splits != null) { + for (final Split split : allocation.splits) { + if (split == null || split.shards == null) { + continue; + } + for (final Shard shard : split.shards) { + if (shard != null) { + shard.cacheSaltPrefix(); + } + } + } + } + } + } + /** * Validates and caches SemVer comparands for all SEMVER_* conditions in a flag. Throws {@link * InvalidSemverComparandException} if any condition has an invalid or non-string comparand value. 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 61332a13520..20524d1775d 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 @@ -97,6 +97,11 @@ void preprocessesSemverComparandsAndDropsMalformedFlags() throws Exception { allocation( "non-semver", "[{\"conditions\":[{\"attribute\":\"version\",\"operator\":\"MATCHES\",\"value\":\"1\"}]}]")), + booleanFlag( + "invalid-regex", + allocation( + "invalid-regex", + "[{\"conditions\":[{\"attribute\":\"version\",\"operator\":\"MATCHES\",\"value\":\"[\"}]}]")), booleanFlag( "valid-semver", allocation( @@ -122,6 +127,7 @@ void preprocessesSemverComparandsAndDropsMalformedFlags() throws Exception { assertTrue(configuration.flags.containsKey("no-conditions")); assertTrue(configuration.flags.containsKey("no-operator")); assertTrue(configuration.flags.containsKey("non-semver")); + assertTrue(configuration.flags.containsKey("invalid-regex")); assertTrue(configuration.flags.containsKey("valid-semver")); assertFalse(configuration.flags.containsKey("invalid-semver")); assertFalse(configuration.flags.containsKey("non-string-semver")); @@ -130,6 +136,29 @@ void preprocessesSemverComparandsAndDropsMalformedFlags() throws Exception { assertEquals("invalid_semver_comparand", configuration.invalidFlags.get("invalid-semver")); assertEquals("invalid_semver_comparand", configuration.invalidFlags.get("non-string-semver")); + assertTrue( + configuration + .flags + .get("non-semver") + .allocations + .get(0) + .rules + .get(0) + .conditions + .get(0) + .hasCachedRegexPattern()); + assertFalse( + configuration + .flags + .get("invalid-regex") + .allocations + .get(0) + .rules + .get(0) + .conditions + .get(0) + .hasCachedRegexPattern()); + assertNotNull( configuration .flags @@ -160,6 +189,20 @@ void dropsFlagWithMissingSplitShards() throws Exception { assertEquals("invalid_flag", configuration.invalidFlags.get("missing-shards")); } + @Test + void preprocessesValidShardData() throws Exception { + final ServerConfiguration configuration = + parse( + wrap( + configWithFlags( + booleanFlag( + "valid-shard", + ",\"allocations\":[{\"key\":\"valid-shard\",\"rules\":[],\"splits\":[{\"variationKey\":\"on\",\"shards\":[{\"salt\":\"test-salt\",\"ranges\":[],\"totalShards\":10000}]}]}]")))); + + assertNotNull(configuration); + assertTrue(configuration.flags.containsKey("valid-shard")); + } + @Test void nullAttributesAreRejectedWithoutInvokingTheFlagParser() throws Exception { assertNull(parse("{\"data\":{\"type\":\"universal-flag-configuration\",\"attributes\":null}}"));