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 dcfe62db147..91c0aafdc7a 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 @@ -58,7 +58,12 @@ public DDEvaluator(final Runnable configCallback) { public boolean initialize( final long timeout, final TimeUnit unit, final EvaluationContext context) throws Exception { FeatureFlaggingGateway.addConfigListener(this); - return initializationLatch.await(timeout, unit); // await for initialization + return initializationLatch.await(timeout, unit) || hasConfiguration(); + } + + @Override + public boolean hasConfiguration() { + return configuration.get() != null; } @Override @@ -69,8 +74,12 @@ public void shutdown() { @Override public void accept(final ServerConfiguration config) { configuration.set(config); - initializationLatch.countDown(); - configCallback.run(); + if (config != null) { + initializationLatch.countDown(); + configCallback.run(); + } else if (initializationLatch.getCount() == 0) { + configCallback.run(); + } } @Override diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Evaluator.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Evaluator.java index 6ce3bac7b93..f4c9cacffdb 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Evaluator.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Evaluator.java @@ -8,6 +8,8 @@ interface Evaluator { boolean initialize(long timeout, TimeUnit timeUnit, EvaluationContext context) throws Exception; + boolean hasConfiguration(); + void shutdown(); ProviderEvaluation evaluate( diff --git a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Provider.java b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Provider.java index f16b6e582a8..c492ef49c69 100644 --- a/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Provider.java +++ b/products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Provider.java @@ -3,6 +3,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; import de.thetaphi.forbiddenapis.SuppressForbidden; +import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.EventProvider; import dev.openfeature.sdk.Hook; @@ -18,7 +19,7 @@ import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -30,7 +31,8 @@ public class Provider extends EventProvider implements Metadata { private static final Options DEFAULT_OPTIONS = new Options().initTimeout(30, SECONDS); private volatile Evaluator evaluator; private final Options options; - private final AtomicBoolean initialized = new AtomicBoolean(false); + private final AtomicReference initializationState = + new AtomicReference<>(InitializationState.NOT_STARTED); private final FlagEvalMetrics flagEvalMetrics; private final FlagEvalHook flagEvalHook; @@ -61,30 +63,97 @@ public Provider(final Options options) { @Override public void initialize(final EvaluationContext context) throws Exception { + initializationState.set(InitializationState.INITIALIZING); try { evaluator = buildEvaluator(); - final boolean init = evaluator.initialize(options.getTimeout(), options.getUnit(), context); - initialized.set(init); - if (!init) { + if (!evaluator.initialize(options.getTimeout(), options.getUnit(), context)) { + if (markInitialConfigReceivedReady()) { + return; + } + markInitializationError(); + throw new ProviderNotReadyError( + "Provider timed-out while waiting for initial configuration"); + } + if (!evaluator.hasConfiguration() || !markSuccessfulInitializationReady()) { + markInitializationError(); throw new ProviderNotReadyError( "Provider timed-out while waiting for initial configuration"); } } catch (final OpenFeatureError e) { + markInitializationError(); throw e; } catch (final Throwable e) { + markInitializationError(); throw new FatalError("Failed to initialize provider, is the tracer configured?", e); } } - private void onConfigurationChange() { - if (initialized.getAndSet(true)) { - emit( - ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, - ProviderEventDetails.builder().message("New configuration received").build()); - } else { + void onConfigurationChange() { + if (evaluator == null || !evaluator.hasConfiguration()) { + onConfigurationUnavailable(); + return; + } + + final InitializationState state = initializationState.get(); + if (state == InitializationState.INITIALIZING) { + initializationState.compareAndSet( + InitializationState.INITIALIZING, InitializationState.INITIAL_CONFIG_RECEIVED); + return; + } + if (state == InitializationState.INITIAL_CONFIG_RECEIVED) { + return; + } + if (state == InitializationState.ERROR + && initializationState.compareAndSet( + InitializationState.ERROR, InitializationState.READY)) { emit( ProviderEvent.PROVIDER_READY, ProviderEventDetails.builder().message("Provider ready").build()); + return; + } + if (initializationState.get() != InitializationState.READY) { + return; + } + emit( + ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, + ProviderEventDetails.builder().message("New configuration received").build()); + } + + private void onConfigurationUnavailable() { + if (initializationState.compareAndSet( + InitializationState.INITIAL_CONFIG_RECEIVED, InitializationState.ERROR)) { + return; + } + if (!initializationState.compareAndSet(InitializationState.READY, InitializationState.ERROR)) { + return; + } + emit( + ProviderEvent.PROVIDER_ERROR, + ProviderEventDetails.builder() + .message("Configuration unavailable") + .errorCode(ErrorCode.PROVIDER_NOT_READY) + .build()); + } + + private boolean markInitialConfigReceivedReady() { + return initializationState.get() == InitializationState.READY + || initializationState.compareAndSet( + InitializationState.INITIAL_CONFIG_RECEIVED, InitializationState.READY); + } + + private boolean markSuccessfulInitializationReady() { + return markInitialConfigReceivedReady() + || initializationState.compareAndSet( + InitializationState.INITIALIZING, InitializationState.READY); + } + + private void markInitializationError() { + InitializationState state = initializationState.get(); + while (state != InitializationState.READY && state != InitializationState.ERROR) { + if (initializationState.compareAndSet(state, InitializationState.ERROR)) { + return; + } + state = initializationState.get(); } } @@ -160,6 +229,14 @@ protected Class loadEvaluatorClass() throws ClassNotFoundException { return Class.forName(EVALUATOR_IMPL); } + private enum InitializationState { + NOT_STARTED, + INITIALIZING, + INITIAL_CONFIG_RECEIVED, + READY, + ERROR + } + public static class Options { private long timeout; 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 f13f8d2a2ba..bb86c409bad 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 java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; @@ -52,6 +54,9 @@ import java.util.List; import java.util.Map; import java.util.TimeZone; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -79,6 +84,7 @@ public void setup() { @AfterEach public void tearDown() { FeatureFlaggingGateway.removeExposureListener(exposureListener); + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); } private static Arguments[] valueMappingTestCases() { @@ -148,6 +154,39 @@ public void testEvaluateNoConfig() { assertThat(details.getErrorCode(), equalTo(ErrorCode.PROVIDER_NOT_READY)); } + @Test + public void testInitializeTimesOutWithoutConfig() throws Exception { + final Runnable configCallback = mock(Runnable.class); + final DDEvaluator evaluator = new DDEvaluator(configCallback); + evaluator.accept(null); + try { + assertThat( + evaluator.initialize(10, MILLISECONDS, mock(EvaluationContext.class)), equalTo(false)); + verify(configCallback, times(0)).run(); + } finally { + evaluator.shutdown(); + } + } + + @Test + public void testInitializeWaitsForNonNullConfig() throws Exception { + final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); + final ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + final Future initialized = + executor.submit(() -> evaluator.initialize(1, SECONDS, mock(EvaluationContext.class))); + + evaluator.accept(null); + assertThat(initialized.isDone(), equalTo(false)); + + evaluator.accept(mock(ServerConfiguration.class)); + assertThat(initialized.get(1, SECONDS), equalTo(true)); + } finally { + executor.shutdownNow(); + evaluator.shutdown(); + } + } + @Test public void testEvaluateNoContext() { final DDEvaluator evaluator = new DDEvaluator(mock(Runnable.class)); 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 87a80f59e20..27d4dd5d2b5 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 @@ -9,7 +9,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -20,6 +19,7 @@ import datadog.trace.api.featureflag.ufc.v1.ServerConfiguration; import datadog.trace.api.openfeature.Provider.Options; import dev.openfeature.sdk.Client; +import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.EventDetails; import dev.openfeature.sdk.Features; @@ -32,9 +32,12 @@ import dev.openfeature.sdk.Value; import dev.openfeature.sdk.exceptions.FatalError; import dev.openfeature.sdk.exceptions.ProviderNotReadyError; +import java.lang.reflect.Field; import java.util.List; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -79,46 +82,228 @@ public void testSetProvider() { } @Test - public void testSetProviderAndWait() { + public void testSetProviderAndWait() throws Exception { final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); - executor.submit(() -> api.setProviderAndWait(new Provider())); + final Future provider = executor.submit(() -> api.setProviderAndWait(new Provider())); final Client client = api.getClient(); assertThat(client.getProviderState(), equalTo(ProviderState.NOT_READY)); FeatureFlaggingGateway.dispatch(mock(ServerConfiguration.class)); await().atMost(ofSeconds(1)).until(() -> client.getProviderState() == ProviderState.READY); + provider.get(1, SECONDS); } @Test - public void testSetProviderAndWaitTimeout() { + public void testSetProviderAndWaitTimeoutRecoversWhenConfigurationArrives() { final Consumer readyEvent = mock(Consumer.class); final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); final Client client = api.getClient(); client.on(ProviderEvent.PROVIDER_READY, readyEvent); - // we time out after 10 millis without receiving the initial config assertThrows( ProviderNotReadyError.class, () -> api.setProviderAndWait(new Provider(new Options().initTimeout(10, MILLISECONDS)))); - // ready has not yet been called + assertThat(client.getProviderState(), equalTo(ProviderState.ERROR)); verify(readyEvent, times(0)).accept(any()); - // dispatch an initial configuration FeatureFlaggingGateway.dispatch(mock(ServerConfiguration.class)); - // ready is called after receiving the configuration await() .atMost(ofSeconds(1)) .untilAsserted( () -> { + assertThat(client.getProviderState(), equalTo(ProviderState.READY)); verify(readyEvent, times(1)).accept(eventDetailsCaptor.capture()); - final EventDetails details = eventDetailsCaptor.getValue(); - assertThat(details.getProviderName(), equalTo(METADATA)); + final EventDetails eventDetails = eventDetailsCaptor.getValue(); + assertThat(eventDetails.getProviderName(), equalTo(METADATA)); }); } + @Test + public void testSetProviderAndWaitCompletesWhenConfigurationArrivesAtTimeoutBoundary() + throws Exception { + final Provider[] providerRef = new Provider[1]; + final Evaluator evaluator = + new Evaluator() { + private boolean hasConfiguration; + + @Override + public boolean initialize( + final long timeout, + final java.util.concurrent.TimeUnit timeUnit, + final EvaluationContext context) { + hasConfiguration = true; + providerRef[0].onConfigurationChange(); + return false; + } + + @Override + public boolean hasConfiguration() { + return hasConfiguration; + } + + @Override + public void shutdown() {} + + @Override + public ProviderEvaluation evaluate( + final Class target, + final String key, + final T defaultValue, + final EvaluationContext context) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + }; + + final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + providerRef[0] = new Provider(new Options().initTimeout(10, MILLISECONDS), evaluator); + api.setProviderAndWait(providerRef[0]); + + final Client client = api.getClient(); + assertThat(client.getProviderState(), equalTo(ProviderState.READY)); + } + + @Test + public void testSetProviderAndWaitFailsWhenConfigurationIsRemovedBeforeInitializationCompletes() { + final Provider[] providerRef = new Provider[1]; + final Evaluator evaluator = + new Evaluator() { + private boolean hasConfiguration; + + @Override + public boolean initialize( + final long timeout, + final java.util.concurrent.TimeUnit timeUnit, + final EvaluationContext context) { + hasConfiguration = true; + providerRef[0].onConfigurationChange(); + hasConfiguration = false; + providerRef[0].onConfigurationChange(); + return true; + } + + @Override + public boolean hasConfiguration() { + return hasConfiguration; + } + + @Override + public void shutdown() {} + + @Override + public ProviderEvaluation evaluate( + final Class target, + final String key, + final T defaultValue, + final EvaluationContext context) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + }; + + final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + providerRef[0] = new Provider(new Options().initTimeout(10, MILLISECONDS), evaluator); + + assertThrows(ProviderNotReadyError.class, () -> api.setProviderAndWait(providerRef[0])); + + final Client client = api.getClient(); + assertThat(client.getProviderState(), equalTo(ProviderState.ERROR)); + } + + @Test + public void testInitializationErrorDoesNotOverwriteRecoveredReadyState() throws Exception { + final Provider[] providerRef = new Provider[1]; + final Evaluator evaluator = + new Evaluator() { + private boolean hasConfiguration; + + @Override + public boolean initialize( + final long timeout, + final java.util.concurrent.TimeUnit timeUnit, + final EvaluationContext context) { + hasConfiguration = true; + providerRef[0].onConfigurationChange(); + hasConfiguration = false; + providerRef[0].onConfigurationChange(); + hasConfiguration = true; + providerRef[0].onConfigurationChange(); + throw new ProviderNotReadyError( + "Provider timed-out while waiting for initial configuration"); + } + + @Override + public boolean hasConfiguration() { + return hasConfiguration; + } + + @Override + public void shutdown() {} + + @Override + public ProviderEvaluation evaluate( + final Class target, + final String key, + final T defaultValue, + final EvaluationContext context) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + }; + + providerRef[0] = new Provider(new Options().initTimeout(10, MILLISECONDS), evaluator); + + assertThrows(ProviderNotReadyError.class, () -> providerRef[0].initialize(null)); + + assertThat(initializationState(providerRef[0]), equalTo("READY")); + } + + @Test + public void testNullConfigurationAfterReadyTransitionsToErrorAndRecovers() { + final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + api.setProvider(new Provider()); + final Client client = api.getClient(); + + FeatureFlaggingGateway.dispatch(mock(ServerConfiguration.class)); + await().atMost(ofSeconds(1)).until(() -> client.getProviderState() == ProviderState.READY); + + final Consumer errorEvent = mock(Consumer.class); + final Consumer readyEvent = mock(Consumer.class); + final Consumer configChangedEvent = mock(Consumer.class); + client.on(ProviderEvent.PROVIDER_ERROR, errorEvent); + client.on(ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, configChangedEvent); + + FeatureFlaggingGateway.dispatch((ServerConfiguration) null); + await() + .atMost(ofSeconds(1)) + .untilAsserted( + () -> { + assertThat(client.getProviderState(), equalTo(ProviderState.ERROR)); + verify(errorEvent, times(1)).accept(eventDetailsCaptor.capture()); + final EventDetails eventDetails = eventDetailsCaptor.getValue(); + assertThat(eventDetails.getProviderName(), equalTo(METADATA)); + }); + + final FlagEvaluationDetails evalDetails = client.getStringDetails("missing", "default"); + assertThat(evalDetails.getValue(), equalTo("default")); + assertThat(evalDetails.getErrorCode(), equalTo(ErrorCode.PROVIDER_NOT_READY)); + + client.on(ProviderEvent.PROVIDER_READY, readyEvent); + FeatureFlaggingGateway.dispatch(mock(ServerConfiguration.class)); + await() + .atMost(ofSeconds(1)) + .untilAsserted( + () -> { + assertThat(client.getProviderState(), equalTo(ProviderState.READY)); + verify(readyEvent, times(1)).accept(any()); + }); + + FeatureFlaggingGateway.dispatch(mock(ServerConfiguration.class)); + await() + .atMost(ofSeconds(1)) + .untilAsserted(() -> verify(configChangedEvent, times(1)).accept(any())); + } + @Test public void testFailureToLoadInternalApi() { @SuppressWarnings("unchecked") @@ -152,7 +337,8 @@ public void testGetProviderHooksReturnsFlagEvalHook() { @Test public void testShutdownCleansUpMetrics() throws Exception { Evaluator evaluator = mock(Evaluator.class); - when(evaluator.initialize(anyLong(), any(), any())).thenReturn(true); + when(evaluator.initialize(eq(10L), eq(MILLISECONDS), any())).thenReturn(true); + when(evaluator.hasConfiguration()).thenReturn(true); Provider provider = new Provider(new Options().initTimeout(10, MILLISECONDS), evaluator); provider.initialize(null); provider.shutdown(); @@ -182,7 +368,8 @@ public void testProviderEvaluation( final String flag, final E defaultValue, final EvaluateMethod method) throws Exception { FeatureFlaggingGateway.dispatch(mock(ServerConfiguration.class)); final Evaluator evaluator = mock(Evaluator.class); - when(evaluator.initialize(anyLong(), any(), any())).thenReturn(true); + when(evaluator.initialize(eq(10L), eq(SECONDS), any())).thenReturn(true); + when(evaluator.hasConfiguration()).thenReturn(true); when(evaluator.evaluate(any(), any(), any(), any())) .thenAnswer( invocation -> @@ -200,4 +387,11 @@ public void testProviderEvaluation( verify(evaluator, times(1)) .evaluate(any(), eq(flag), eq(defaultValue), any(EvaluationContext.class)); } + + private static String initializationState(final Provider provider) throws Exception { + final Field stateField = Provider.class.getDeclaredField("initializationState"); + stateField.setAccessible(true); + final AtomicReference state = (AtomicReference) stateField.get(provider); + return state.get().toString(); + } }