From 612ef527e1c060eee5f0b84b17b5f4603285cfa4 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Sat, 12 Sep 2026 03:50:17 +0000 Subject: [PATCH 1/8] Harden agentless Feature Flags EVP delivery Add capability-gated local discovery, safe sticky direct fallback, shared route state, send-once semantics, and bounded lifecycle handling for Java Feature Flags telemetry. Environment: Datadog workspace --- .../communication/BackendApiFactory.java | 163 +++++++- .../java/datadog/communication/EvpProxy.java | 9 + .../ddagent/DDAgentFeaturesDiscovery.java | 37 +- .../communication/BackendApiFactoryTest.java | 394 +++++++++++++++++- .../java/lang/ShutdownInstrumentation.java | 10 +- ...ceDisabledFeatureFlaggingShutdownTest.java | 45 ++ .../featureflag/FeatureFlaggingSystem.java | 6 +- .../AgentlessFeatureFlagBackendApi.java | 155 +++++-- .../featureflag/ExposureWriterImpl.java | 172 ++++++-- .../FeatureFlagBackendApiFactory.java | 103 +++-- .../featureflag/FeatureFlagRouteSelector.java | 79 ++++ .../featureflag/FlagEvaluationWriterImpl.java | 14 + .../AgentlessFeatureFlagBackendApiTest.java | 345 ++++++++++++++- .../featureflag/ExposureWriterTests.java | 217 +++++++++- .../FeatureFlagBackendApiFactoryTest.java | 90 +--- 15 files changed, 1611 insertions(+), 228 deletions(-) create mode 100644 dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java create mode 100644 products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java diff --git a/communication/src/main/java/datadog/communication/BackendApiFactory.java b/communication/src/main/java/datadog/communication/BackendApiFactory.java index c1b471ebfc8..08c2b08d897 100644 --- a/communication/src/main/java/datadog/communication/BackendApiFactory.java +++ b/communication/src/main/java/datadog/communication/BackendApiFactory.java @@ -1,27 +1,61 @@ package datadog.communication; +import static java.util.Collections.emptyMap; +import static java.util.Collections.unmodifiableMap; + import datadog.communication.ddagent.DDAgentFeaturesDiscovery; import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; import datadog.trace.util.throwable.FatalAgentMisconfigurationError; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; +import okhttp3.Request; import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class BackendApiFactory { private static final Logger log = LoggerFactory.getLogger(BackendApiFactory.class); + private static final int MAX_DNS_LABEL_LENGTH = 63; + private static final int MAX_DNS_HOST_LENGTH = 253; private final Config config; private final SharedCommunicationObjects sharedCommunicationObjects; + private final Map requestHeaders; + private final boolean sendOnce; public BackendApiFactory(Config config, SharedCommunicationObjects sharedCommunicationObjects) { + this(config, sharedCommunicationObjects, emptyMap()); + } + + public BackendApiFactory( + Config config, + SharedCommunicationObjects sharedCommunicationObjects, + Map requestHeaders) { + this(config, sharedCommunicationObjects, requestHeaders, false); + } + + /** + * Creates a backend factory with per-request headers and optional send-once transport semantics. + * + *

When {@code sendOnce} is true, both the explicit HTTP retry policy and OkHttp's automatic + * connection retry are disabled. This is required for event payloads that do not carry an + * idempotency key. + */ + public BackendApiFactory( + Config config, + SharedCommunicationObjects sharedCommunicationObjects, + Map requestHeaders, + boolean sendOnce) { this.config = config; this.sharedCommunicationObjects = sharedCommunicationObjects; + this.requestHeaders = unmodifiableMap(new HashMap<>(requestHeaders)); + this.sendOnce = sendOnce; } public @Nullable BackendApi createBackendApi(Intake intake) { @@ -67,7 +101,9 @@ public BackendApi createDirectIntakeApi( apiKey, traceId, retryPolicyFactory(), - directIntakeHttpClient(sharedCommunicationObjects.getIntakeHttpClient(), followRedirects), + configureHttpClient( + directIntakeHttpClient( + sharedCommunicationObjects.getIntakeHttpClient(), followRedirects)), responseCompression); } @@ -87,11 +123,14 @@ private static HttpUrl buildDirectIntakeUrl(Intake intake, Config config) { } static HttpUrl buildEventPlatformIntakeUrl(String site) { - if (site == null || site.isEmpty()) { + if (!isValidDnsSuffix(site)) { throw new IllegalArgumentException("Invalid Datadog site"); } String expectedHost = Intake.EVENT_PLATFORM.getUrlPrefix() + "." + site; + if (expectedHost.length() > MAX_DNS_HOST_LENGTH) { + throw new IllegalArgumentException("Invalid Datadog site"); + } HttpUrl url = new HttpUrl.Builder() .scheme("https") @@ -106,6 +145,38 @@ static HttpUrl buildEventPlatformIntakeUrl(String site) { return url; } + private static boolean isValidDnsSuffix(@Nullable String site) { + if (site == null || site.isEmpty()) { + return false; + } + + int labelLength = 0; + for (int i = 0; i < site.length(); i++) { + final char character = site.charAt(i); + if (character == '.') { + if (labelLength == 0 || labelLength > MAX_DNS_LABEL_LENGTH || site.charAt(i - 1) == '-') { + return false; + } + labelLength = 0; + } else { + if ((!isAsciiLetterOrDigit(character) && character != '-') + || (labelLength == 0 && character == '-')) { + return false; + } + labelLength++; + } + } + return labelLength > 0 + && labelLength <= MAX_DNS_LABEL_LENGTH + && site.charAt(site.length() - 1) != '-'; + } + + private static boolean isAsciiLetterOrDigit(final char character) { + return (character >= 'a' && character <= 'z') + || (character >= 'A' && character <= 'Z') + || (character >= '0' && character <= '9'); + } + /** Creates an API client that uses the specified retry policy with a compatible local proxy. */ public @Nullable BackendApi createEvpProxyApi(Intake intake) { return createEvpProxyApi(intake, true); @@ -119,32 +190,104 @@ static HttpUrl buildEventPlatformIntakeUrl(String site) { /** Creates an API client that sends data through a compatible local EVP proxy. */ public @Nullable BackendApi createEvpProxyApi( Intake intake, boolean responseCompression, HttpRetryPolicy.Factory retryPolicyFactory) { + return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, false, false); + } + + /** + * Creates an EVP proxy client after Agent discovery, optionally forcing a fresh discovery and + * requiring the Agent to advertise every configured request header. + * + *

The {@code forceDiscovery} form is intended for bounded unavailable-route recovery probes. + */ + public @Nullable BackendApi createEvpProxyApi( + Intake intake, + boolean responseCompression, + HttpRetryPolicy.Factory retryPolicyFactory, + boolean forceDiscovery, + boolean requireConfiguredRequestHeaders) { DDAgentFeaturesDiscovery featuresDiscovery = sharedCommunicationObjects.featuresDiscovery(config); - featuresDiscovery.discoverIfOutdated(); - if (!featuresDiscovery.supportsEvpProxy()) { - return null; + if (forceDiscovery) { + featuresDiscovery.discover(); + } else { + featuresDiscovery.discoverIfOutdated(); } String evpProxyEndpoint = featuresDiscovery.getEvpProxyEndpoint(); + if (evpProxyEndpoint != null + && requireConfiguredRequestHeaders + && !featuresDiscovery.supportsEvpProxyHeaders(requestHeaders.keySet())) { + evpProxyEndpoint = null; + } + if (evpProxyEndpoint == null) { + return null; + } + + return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, evpProxyEndpoint); + } + + /** Creates an EVP proxy client for a fixed compatibility endpoint without Agent discovery. */ + public BackendApi createEvpProxyApiForEndpoint( + Intake intake, + boolean responseCompression, + HttpRetryPolicy.Factory retryPolicyFactory, + String evpProxyEndpoint) { + return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, evpProxyEndpoint); + } + private BackendApi createEvpProxyApi( + Intake intake, + boolean responseCompression, + HttpRetryPolicy.Factory retryPolicyFactory, + String evpProxyEndpoint) { String traceId = config.getIdGenerationStrategy().generateTraceId().toString(); log.debug( "Creating EVP proxy client for {} using endpoint {} with responseCompression={}", intake, evpProxyEndpoint, responseCompression); - HttpUrl evpProxyUrl = sharedCommunicationObjects.agentUrl.resolve(evpProxyEndpoint); + HttpUrl evpProxyUrl = appendPath(sharedCommunicationObjects.agentUrl, evpProxyEndpoint); String subdomain = intake.getUrlPrefix(); return new EvpProxyApi( traceId, evpProxyUrl, subdomain, - retryPolicyFactory, - sharedCommunicationObjects.agentHttpClient, + sendOnce ? HttpRetryPolicy.Factory.NEVER_RETRY : retryPolicyFactory, + configureHttpClient(sharedCommunicationObjects.agentHttpClient), responseCompression); } - private static HttpRetryPolicy.Factory retryPolicyFactory() { - return new HttpRetryPolicy.Factory(5, 100, 2.0, true); + static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { + int firstCharacter = 0; + while (firstCharacter < path.length() && path.charAt(firstCharacter) == '/') { + firstCharacter++; + } + return baseUrl.newBuilder().addPathSegments(path.substring(firstCharacter)).build(); + } + + OkHttpClient configureHttpClient(final OkHttpClient httpClient) { + if (requestHeaders.isEmpty() && !sendOnce) { + return httpClient; + } + final OkHttpClient.Builder builder = httpClient.newBuilder(); + if (sendOnce) { + builder.retryOnConnectionFailure(false); + } + if (!requestHeaders.isEmpty()) { + builder.addInterceptor( + chain -> { + final Request.Builder requestBuilder = chain.request().newBuilder(); + for (Map.Entry header : requestHeaders.entrySet()) { + requestBuilder.header(header.getKey(), header.getValue()); + } + return chain.proceed(requestBuilder.build()); + }); + } + return builder.build(); + } + + private HttpRetryPolicy.Factory retryPolicyFactory() { + return sendOnce + ? HttpRetryPolicy.Factory.NEVER_RETRY + : new HttpRetryPolicy.Factory(5, 100, 2.0, true); } } diff --git a/communication/src/main/java/datadog/communication/EvpProxy.java b/communication/src/main/java/datadog/communication/EvpProxy.java index c2453bccb25..05ce9757339 100644 --- a/communication/src/main/java/datadog/communication/EvpProxy.java +++ b/communication/src/main/java/datadog/communication/EvpProxy.java @@ -5,6 +5,15 @@ public final class EvpProxy { public static final String SUBDOMAIN_HEADER = "X-Datadog-EVP-Subdomain"; + /** Identifies the SDK that produced an EVP request. */ + public static final String ORIGIN_HEADER = "DD-EVP-ORIGIN"; + + /** Identifies the version of the SDK that produced an EVP request. */ + public static final String ORIGIN_VERSION_HEADER = "DD-EVP-ORIGIN-VERSION"; + + /** Origin header value identifying this tracing library. */ + public static final String JAVA_TRACING_LIBRARY = "dd-trace-java"; + /** * Default SDK-side target for uncompressed EVP request bodies. Writers may split batches at or * below this size to keep Agent proxy requests comfortably bounded. diff --git a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java index 16be2e84b98..c7fc60f1d1f 100644 --- a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java +++ b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java @@ -24,6 +24,7 @@ import java.nio.ByteBuffer; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import okhttp3.HttpUrl; @@ -98,6 +99,7 @@ private static class State { String debuggerSnapshotEndpoint; String debuggerDiagnosticsEndpoint; String evpProxyEndpoint; + Set evpProxyAllowedHeaders = emptySet(); String version; String telemetryProxyEndpoint; Set peerTags = emptySet(); @@ -156,7 +158,7 @@ private void doDiscovery(State newState) { try (Recording recording = discoveryTimer.start()) { boolean fallback = true; final Request request = - prepareRequest(agentBaseUrl.resolve("info"), emptyMap()).get().build(); + prepareRequest(appendPath(agentBaseUrl, "info"), emptyMap()).get().build(); try (Response response = client.newCall(request).execute()) { if (response.isSuccessful()) { processInfoResponseHeaders(response); @@ -206,7 +208,7 @@ private String probeTracesEndpoint(State newState, List endpoints) { try (Response response = client .newCall( - prepareRequest(agentBaseUrl.resolve(candidate), emptyMap()) + prepareRequest(appendPath(agentBaseUrl, candidate), emptyMap()) .put(msgpackRequestBodyOf(singletonList(ByteBuffer.wrap(PROBE_MESSAGE)))) .build()) .execute()) { @@ -289,6 +291,16 @@ private boolean processInfoResponse(State newState, String response) { break; } } + final Object allowedHeadersObj = map.get("evp_proxy_allowed_headers"); + if (allowedHeadersObj instanceof List) { + final Set allowedHeaders = new HashSet<>(); + for (Object header : (List) allowedHeadersObj) { + if (header instanceof String) { + allowedHeaders.add(((String) header).toLowerCase(Locale.ROOT)); + } + } + newState.evpProxyAllowedHeaders = unmodifiableSet(allowedHeaders); + } for (String endpoint : telemetryProxyEndpoints) { if (containsEndpoint(endpoints, endpoint)) { @@ -424,7 +436,7 @@ public String getEvpProxyEndpoint() { } public HttpUrl buildUrl(String endpoint) { - return agentBaseUrl.resolve(endpoint); + return appendPath(agentBaseUrl, endpoint); } public boolean supportsDataStreams() { @@ -435,6 +447,17 @@ public boolean supportsEvpProxy() { return discoveryState.evpProxyEndpoint != null; } + /** Returns whether the Agent advertises forwarding every required EVP request header. */ + public boolean supportsEvpProxyHeaders(final Iterable requiredHeaders) { + final Set allowedHeaders = discoveryState.evpProxyAllowedHeaders; + for (String requiredHeader : requiredHeaders) { + if (!allowedHeaders.contains(requiredHeader.toLowerCase(Locale.ROOT))) { + return false; + } + } + return true; + } + public boolean supportsContentEncodingHeadersWithEvpProxy() { // content encoding headers are supported in /v4 and above final String evpProxyEndpoint = discoveryState.evpProxyEndpoint; @@ -469,4 +492,12 @@ public boolean active() { public boolean supportsTelemetryProxy() { return discoveryState.telemetryProxyEndpoint != null; } + + private static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { + int firstCharacter = 0; + while (firstCharacter < path.length() && path.charAt(firstCharacter) == '/') { + firstCharacter++; + } + return baseUrl.newBuilder().addPathSegments(path.substring(firstCharacter)).build(); + } } diff --git a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java index 44690ef1c3f..5ead5496322 100644 --- a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java +++ b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java @@ -1,10 +1,19 @@ package datadog.communication; +import static datadog.communication.EvpProxy.JAVA_TRACING_LIBRARY; +import static datadog.communication.EvpProxy.ORIGIN_HEADER; +import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_AGENTLESS_URL; +import static datadog.trace.api.config.GeneralConfig.API_KEY; +import static java.util.Collections.singletonMap; 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; +import static org.junit.jupiter.api.Assertions.assertTrue; import datadog.communication.ddagent.DDAgentFeaturesDiscovery; import datadog.communication.ddagent.SharedCommunicationObjects; @@ -15,7 +24,12 @@ import datadog.trace.api.intake.Intake; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.HashMap; import java.util.Locale; +import java.util.Map; +import java.util.Properties; +import java.util.stream.Stream; import okhttp3.HttpUrl; import okhttp3.MediaType; import okhttp3.OkHttpClient; @@ -25,6 +39,7 @@ import okhttp3.mockwebserver.RecordedRequest; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; @@ -33,8 +48,18 @@ class BackendApiFactoryTest { private static final MediaType JSON = MediaType.parse("application/json"); @ParameterizedTest - @ValueSource(strings = {"datadoghq.com", "custom.example", "DATADOGHQ.EU"}) + @ValueSource(strings = {"datadoghq.com", "custom.example", "DATADOGHQ.EU", "mock-intake.invalid"}) void eventPlatformDirectIntakeUsesExactHttpsHost(String site) { + assertEventPlatformIntakeUrl(site); + } + + @ParameterizedTest + @MethodSource("boundaryValidSites") + void eventPlatformDirectIntakeAcceptsDnsLengthBoundaries(String site) { + assertEventPlatformIntakeUrl(site); + } + + private static void assertEventPlatformIntakeUrl(final String site) { final HttpUrl url = BackendApiFactory.buildEventPlatformIntakeUrl(site); assertEquals("https", url.scheme()); @@ -63,13 +88,58 @@ void eventPlatformDirectIntakeUsesExactHttpsHost(String site) { "data doghq.com", " datadoghq.com", "datadoghq.com ", - "datadoghq.com\\evil.example" + "datadoghq.com\\evil.example", + "-foo.bar", + "foo-.bar", + "foo.-bar", + "foo.bar-", + "foo_bar.com", + ".foo.bar", + "foo..bar", + "foo.bar." }) void eventPlatformDirectIntakeRejectsUnsafeSite(String site) { assertThrows( IllegalArgumentException.class, () -> BackendApiFactory.buildEventPlatformIntakeUrl(site)); } + @ParameterizedTest + @MethodSource("invalidLengthSites") + void eventPlatformDirectIntakeRejectsDnsLengthOverflow(String site) { + assertThrows( + IllegalArgumentException.class, () -> BackendApiFactory.buildEventPlatformIntakeUrl(site)); + } + + private static Stream boundaryValidSites() { + return Stream.of( + repeatedAsciiLabel(63) + ".invalid", + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(39)); + } + + private static Stream invalidLengthSites() { + return Stream.of( + repeatedAsciiLabel(64) + ".invalid", + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(40)); + } + + private static String repeatedAsciiLabel(final int length) { + final char[] label = new char[length]; + Arrays.fill(label, 'a'); + return new String(label); + } + @ParameterizedTest @ValueSource(ints = {301, 302, 307, 308}) void featureFlagDirectIntakeDoesNotFollowRedirects(final int statusCode) throws Exception { @@ -153,6 +223,296 @@ void advertisedEvpProxyEndpointSupportsDisabledResponseCompression() throws Exce } } + @Test + void evpProxyPreservesConfiguredAgentBasePath() throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + agent.start(); + try { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/agent/base"))); + final BackendApi api = factory.createBackendApi(Intake.EVENT_PLATFORM, false); + + assertNotNull(api); + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + + assertEquals("/agent/base/evp_proxy/v4/api/v2/flagevaluation", agent.takeRequest().getPath()); + } finally { + agent.shutdown(); + } + } + + @Test + void discoveryPreservesAgentBasePathAndAcceptsCaseInsensitiveIdentityCapabilities() + throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"endpoints\":[\"v0.5/traces\",\"/evp_proxy/v4/\"]," + + "\"evp_proxy_allowed_headers\":[\"dd-evp-origin\"," + + "\"DD-EVP-ORIGIN-VERSION\"]}")); + agent.start(); + try { + final DDAgentFeaturesDiscovery discovery = + new DDAgentFeaturesDiscovery( + new OkHttpClient(), + Monitoring.DISABLED, + agent.url("/agent/base"), + ProtocolVersion.V0_5, + true, + false); + + discovery.discover(); + + assertEquals("/agent/base/info", agent.takeRequest().getPath()); + assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); + assertTrue(discovery.supportsEvpProxyHeaders(sdkHeaders().keySet())); + } finally { + agent.shutdown(); + } + } + + @ParameterizedTest + @ValueSource( + strings = { + "{}", + "{\"evp_proxy_allowed_headers\":null}", + "{\"evp_proxy_allowed_headers\":[\"DD-EVP-ORIGIN\"]}", + "{\"evp_proxy_allowed_headers\":[\"DD-EVP-ORIGIN-VERSION\"]}" + }) + void discoveryRejectsMissingOrPartialIdentityCapabilities(final String capabilityJson) + throws Exception { + final MockWebServer agent = new MockWebServer(); + final String fields = capabilityJson.substring(1, capabilityJson.length() - 1); + agent.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"endpoints\":[\"v0.5/traces\",\"evp_proxy/v4/\"]" + + (fields.isEmpty() ? "" : "," + fields) + + "}")); + agent.start(); + try { + final DDAgentFeaturesDiscovery discovery = + new DDAgentFeaturesDiscovery( + new OkHttpClient(), + Monitoring.DISABLED, + agent.url("/"), + ProtocolVersion.V0_5, + true, + false); + + discovery.discover(); + + assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); + assertFalse(discovery.supportsEvpProxyHeaders(sdkHeaders().keySet())); + } finally { + agent.shutdown(); + } + } + + @Test + void featureFlagProxyRequestCarriesSdkIdentityWithoutDirectCredentials() throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + agent.start(); + try { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), + sharedCommunicationObjects(discovery, agent.url("/")), + sdkHeaders(), + true); + final BackendApi api = + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true); + + assertNotNull(api); + api.post( + "exposures", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + + final RecordedRequest request = agent.takeRequest(); + assertEquals("/evp_proxy/v4/api/v2/exposures", request.getPath()); + assertEquals("event-platform-intake", request.getHeader("X-Datadog-EVP-Subdomain")); + assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); + assertEquals("test-version", request.getHeader(ORIGIN_VERSION_HEADER)); + assertNull(request.getHeader("DD-API-KEY")); + assertEquals(1, agent.getRequestCount()); + } finally { + agent.shutdown(); + } + } + + @Test + void directIntakeSendsConfiguredRequestHeaders() throws Exception { + final MockWebServer intake = new MockWebServer(); + intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + intake.start(); + try { + final Properties properties = new Properties(); + properties.setProperty(API_KEY, "api-key"); + properties.setProperty( + CIVISIBILITY_AGENTLESS_URL, intake.url("/").toString().replaceAll("/$", "")); + final Config config = Config.get(properties); + final BackendApiFactory factory = + new BackendApiFactory( + config, + sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), + singletonMap(ORIGIN_HEADER, JAVA_TRACING_LIBRARY)); + + // followRedirects=false mirrors the feature-flagging caller, so this also covers the + // interaction between the redirect-scoped client and the header interceptor. + final BackendApi api = factory.createDirectIntakeApi(Intake.API, false, false); + + assertNotNull(api); + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + + final RecordedRequest request = intake.takeRequest(); + assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); + assertEquals("api-key", request.getHeader("DD-API-KEY")); + } finally { + intake.shutdown(); + } + } + + @Test + void featureFlagDirectRequestCarriesCredentialsAndIdentityExactlyOnce() throws Exception { + final MockWebServer intake = new MockWebServer(); + intake.enqueue(new MockResponse().setResponseCode(500).setBody("failed")); + intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + intake.start(); + final OkHttpClient sharedClient = new OkHttpClient.Builder().build(); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), + sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), + sdkHeaders(), + true); + final OkHttpClient directClient = factory.configureHttpClient(sharedClient); + try { + final IntakeApi api = + new IntakeApi( + intake.url("/api/v2/"), + "api-key", + "123", + HttpRetryPolicy.Factory.NEVER_RETRY, + directClient, + false); + + assertThrows( + IOException.class, + () -> + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false)); + + final RecordedRequest request = intake.takeRequest(); + assertEquals("/api/v2/flagevaluation", request.getPath()); + assertEquals("api-key", request.getHeader("DD-API-KEY")); + assertEquals("dd-trace-java", request.getHeader("DD-EVP-ORIGIN")); + assertEquals("test-version", request.getHeader("DD-EVP-ORIGIN-VERSION")); + assertNull(request.getHeader("X-Datadog-EVP-Subdomain")); + assertEquals(1, intake.getRequestCount()); + assertEquals(false, directClient.retryOnConnectionFailure()); + } finally { + directClient.dispatcher().executorService().shutdownNow(); + directClient.connectionPool().evictAll(); + sharedClient.dispatcher().executorService().shutdownNow(); + sharedClient.connectionPool().evictAll(); + intake.shutdown(); + } + } + + @Test + void fixedV2CompatibilityRouteDoesNotDependOnDiscovery() throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue(new MockResponse().setResponseCode(500).setBody("ambiguous")); + agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + agent.start(); + try { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(null); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/"))); + + final BackendApi api = + factory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + V2_EVP_PROXY_ENDPOINT); + + assertNotNull(api); + assertThrows( + HttpResponseException.class, + () -> + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false)); + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + assertEquals("/evp_proxy/v2/api/v2/flagevaluation", agent.takeRequest().getPath()); + assertEquals("/evp_proxy/v2/api/v2/flagevaluation", agent.takeRequest().getPath()); + assertEquals(2, agent.getRequestCount()); + } finally { + agent.shutdown(); + } + } + + @Test + void capabilityGatedProxyRequiresConfiguredRequestHeaders() { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT, false); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, null), sdkHeaders(), true); + + assertNull( + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)); + } + + @Test + void recoveryRequestForcesFreshDiscovery() { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(null); + final BackendApiFactory factory = + new BackendApiFactory(Config.get(), sharedCommunicationObjects(discovery, null)); + + assertNull( + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, true, true)); + assertEquals(1, discovery.forcedDiscoveries); + assertEquals(0, discovery.outdatedDiscoveries); + } + @Test void explicitNoRetryProxyPolicyDoesNotReplayAmbiguousFailure() throws Exception { final MockWebServer agent = new MockWebServer(); @@ -193,6 +553,13 @@ private static SharedCommunicationObjects sharedCommunicationObjects( return sco; } + private static Map sdkHeaders() { + final Map headers = new HashMap<>(2); + headers.put("DD-EVP-ORIGIN", "dd-trace-java"); + headers.put("DD-EVP-ORIGIN-VERSION", "test-version"); + return headers; + } + private static final class TestSharedCommunicationObjects extends SharedCommunicationObjects { private final DDAgentFeaturesDiscovery discovery; @@ -208,8 +575,16 @@ public DDAgentFeaturesDiscovery featuresDiscovery(final Config config) { private static final class FakeFeaturesDiscovery extends DDAgentFeaturesDiscovery { private final String evpProxyEndpoint; + private final boolean supportsRequestHeaders; + private int forcedDiscoveries; + private int outdatedDiscoveries; private FakeFeaturesDiscovery(final String evpProxyEndpoint) { + this(evpProxyEndpoint, true); + } + + private FakeFeaturesDiscovery( + final String evpProxyEndpoint, final boolean supportsRequestHeaders) { super( new OkHttpClient(), Monitoring.DISABLED, @@ -218,10 +593,18 @@ private FakeFeaturesDiscovery(final String evpProxyEndpoint) { true, false); this.evpProxyEndpoint = evpProxyEndpoint; + this.supportsRequestHeaders = supportsRequestHeaders; } @Override - public void discoverIfOutdated() {} + public void discover() { + forcedDiscoveries++; + } + + @Override + public void discoverIfOutdated() { + outdatedDiscoveries++; + } @Override public String getEvpProxyEndpoint() { @@ -232,5 +615,10 @@ public String getEvpProxyEndpoint() { public boolean supportsEvpProxy() { return evpProxyEndpoint != null; } + + @Override + public boolean supportsEvpProxyHeaders(final Iterable requiredHeaders) { + return supportsRequestHeaders; + } } } diff --git a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java index 9aa879ad705..d51def6fa3d 100644 --- a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java +++ b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java @@ -8,6 +8,7 @@ import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.bootstrap.instrumentation.shutdown.ShutdownHelper; +import java.util.Set; import net.bytebuddy.asm.Advice; /** @@ -15,7 +16,7 @@ * before the shutdown hooks are called.
*/ @AutoService(InstrumenterModule.class) -public class ShutdownInstrumentation extends InstrumenterModule.Tracing +public class ShutdownInstrumentation extends InstrumenterModule implements Instrumenter.ForBootstrap, Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { public ShutdownInstrumentation() { @@ -27,6 +28,13 @@ public String instrumentedType() { return "java.lang.Shutdown"; } + @Override + public boolean isApplicable(Set enabledSystems) { + // Agent-owned subsystems such as Feature Flagging can run while tracing is disabled. Their + // bounded final drains still depend on ShutdownHelper running before application hooks. + return true; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( diff --git a/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java b/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java new file mode 100644 index 00000000000..7dc4596c447 --- /dev/null +++ b/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java @@ -0,0 +1,45 @@ +package datadog.trace.agent; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.agent.test.IntegrationTestUtils; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Collections; +import java.util.concurrent.TimeUnit; +import jvmbootstraptest.AgentLoadedChecker; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +class TraceDisabledFeatureFlaggingShutdownTest { + @Test + @Timeout(value = 30, unit = TimeUnit.SECONDS) + void featureFlaggingIsStoppedByTheRealAgentWhenTracingIsDisabled() throws Exception { + try (ByteArrayOutputStream output = new ByteArrayOutputStream(); + PrintStream printStream = new PrintStream(output, true, StandardCharsets.UTF_8.name())) { + int exitCode = + IntegrationTestUtils.runOnSeparateJvm( + AgentLoadedChecker.class.getName(), + Arrays.asList( + "-Ddatadog.slf4j.simpleLogger.defaultLogLevel=debug", + "-Ddd.trace.enabled=false", + "-Ddd.feature.flags.enabled=true", + "-Ddd.feature.flags.configuration.source=agentless", + "-Ddd.jmxfetch.enabled=false", + "-Ddd.profiling.enabled=false", + "-Ddd.remote_config.enabled=false", + "-Ddd.telemetry.enabled=false"), + Collections.emptyList(), + Collections.emptyMap(), + printStream); + + String logs = output.toString(StandardCharsets.UTF_8.name()); + assertEquals(0, exitCode); + assertTrue(logs.contains("Shutting down agent")); + assertTrue(logs.contains("Feature Flagging system stopped")); + } + } +} diff --git a/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java b/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java index 7f105d00213..038c409cf64 100644 --- a/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java +++ b/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java @@ -94,7 +94,8 @@ private static void initializeSystem(final SharedCommunicationObjects sco, final LOGGER.debug("Feature Flagging system disabled by unsupported configuration source"); return; } - final ExposureWriter exposureWriter = new ExposureWriterImpl(sco, config); + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + final ExposureWriter exposureWriter = new ExposureWriterImpl(sco, config, routeSelector); initialize(configService, exposureWriter); final boolean evalCountsEnabled = @@ -103,7 +104,8 @@ private static void initializeSystem(final SharedCommunicationObjects sco, final .getBoolean(FeatureFlaggingConfig.FLAGGING_EVALUATION_COUNTS_ENABLED, true); FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(evalCountsEnabled); if (evalCountsEnabled) { - final FlagEvaluationWriterImpl evalWriter = new FlagEvaluationWriterImpl(sco, config); + final FlagEvaluationWriterImpl evalWriter = + new FlagEvaluationWriterImpl(sco, config, routeSelector); // Publish before start() so a failed start is still reachable by the rollback in stop(). FLAG_EVAL_WRITER = evalWriter; evalWriter.start(); diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java index 769ebfd1dd1..bfb5d42b759 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java @@ -7,32 +7,74 @@ import java.io.IOException; import java.io.InputStream; import java.net.ConnectException; +import java.util.function.LongSupplier; import java.util.function.Supplier; import javax.annotation.Nullable; import okhttp3.RequestBody; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** Sends Feature Flag events through a local EVP proxy, with a safe direct intake fallback. */ +/** Sends Feature Flag events through the process-wide Agentless EVP route selector. */ final class AgentlessFeatureFlagBackendApi implements BackendApi { private static final Logger LOGGER = LoggerFactory.getLogger(AgentlessFeatureFlagBackendApi.class); - private final BackendApi proxyApi; + private final FeatureFlagRouteSelector routeSelector; + private final Supplier proxyApiSupplier; private final Supplier directApiSupplier; private final String eventType; - private volatile BackendApi activeApi; + private volatile BackendApi proxyApi; + private volatile BackendApi directApi; private volatile boolean directApiCreationAttempted; AgentlessFeatureFlagBackendApi( - final BackendApi proxyApi, + @Nullable final BackendApi proxyApi, + @Nullable final BackendApi directApi, + final Supplier proxyApiSupplier, final Supplier directApiSupplier, final String eventType) { + this( + proxyApi, + directApi, + proxyApiSupplier, + directApiSupplier, + eventType, + new FeatureFlagRouteSelector()); + } + + AgentlessFeatureFlagBackendApi( + @Nullable final BackendApi proxyApi, + @Nullable final BackendApi directApi, + final Supplier proxyApiSupplier, + final Supplier directApiSupplier, + final String eventType, + final LongSupplier nanoTime, + final long recoveryIntervalNanos) { + this( + proxyApi, + directApi, + proxyApiSupplier, + directApiSupplier, + eventType, + new FeatureFlagRouteSelector(nanoTime, recoveryIntervalNanos)); + } + + AgentlessFeatureFlagBackendApi( + @Nullable final BackendApi proxyApi, + @Nullable final BackendApi directApi, + final Supplier proxyApiSupplier, + final Supplier directApiSupplier, + final String eventType, + final FeatureFlagRouteSelector routeSelector) { this.proxyApi = proxyApi; + this.directApi = directApi; + this.proxyApiSupplier = proxyApiSupplier; this.directApiSupplier = directApiSupplier; this.eventType = eventType; - this.activeApi = proxyApi; + this.routeSelector = routeSelector; + this.directApiCreationAttempted = directApi != null; + routeSelector.initialize(proxyApi != null, directApi != null); } @Override @@ -43,59 +85,110 @@ public T post( @Nullable final OkHttpUtils.CustomListener requestListener, final boolean requestCompression) throws IOException { - final BackendApi selectedApi = activeApi; + final SelectedApi selected = selectApi(); try { - return selectedApi.post( + return selected.api.post( uri, requestBody, responseParser, requestListener, requestCompression); } catch (final IOException exception) { - if (selectedApi != proxyApi || !isDefinitiveRejection(exception)) { + if (!selected.local) { throw exception; } - final BackendApi directApi = getOrCreateDirectApi(); - if (directApi == null) { + final BackendApi fallbackApi = getOrCreateDirectApi(); + routeSelector.localFailure(fallbackApi != null); + if (fallbackApi == null || !isSafeToReplayDirectly(exception)) { throw exception; } - return directApi.post(uri, requestBody, responseParser, requestListener, requestCompression); + return fallbackApi.post( + uri, requestBody, responseParser, requestListener, requestCompression); } } - @Nullable - private BackendApi getOrCreateDirectApi() { - final BackendApi selectedApi = activeApi; - if (selectedApi != proxyApi) { - return selectedApi; + private SelectedApi selectApi() throws IOException { + FeatureFlagRouteSelector.Route selectedRoute = routeSelector.current(); + if (selectedRoute == FeatureFlagRouteSelector.Route.UNAVAILABLE + && routeSelector.tryBeginLocalRecovery()) { + final BackendApi recoveredProxyApi = discoverProxyApi(); + if (recoveredProxyApi != null) { + proxyApi = recoveredProxyApi; + routeSelector.localRecovered(); + } + selectedRoute = routeSelector.current(); } - synchronized (this) { - final BackendApi currentApi = activeApi; - if (currentApi != proxyApi) { - return currentApi; + if (selectedRoute == FeatureFlagRouteSelector.Route.LOCAL) { + BackendApi selectedProxyApi = proxyApi; + if (selectedProxyApi == null) { + selectedProxyApi = discoverProxyApi(); + if (selectedProxyApi != null) { + proxyApi = selectedProxyApi; + } else { + final BackendApi selectedDirectApi = getOrCreateDirectApi(); + routeSelector.localFailure(selectedDirectApi != null); + if (selectedDirectApi != null) { + return new SelectedApi(selectedDirectApi, false); + } + throw unavailableRoute(); + } } - if (directApiCreationAttempted) { - return null; + return new SelectedApi(selectedProxyApi, true); + } + + if (selectedRoute == FeatureFlagRouteSelector.Route.DIRECT) { + final BackendApi selectedDirectApi = getOrCreateDirectApi(); + if (selectedDirectApi != null) { + return new SelectedApi(selectedDirectApi, false); } + } + throw unavailableRoute(); + } - final BackendApi directApi = directApiSupplier.get(); - if (directApi != null) { - LOGGER.debug( - "Switching Feature Flagging {} delivery from the local EVP proxy to direct intake", - eventType); - activeApi = directApi; + @Nullable + private BackendApi discoverProxyApi() { + try { + return proxyApiSupplier.get(); + } catch (final RuntimeException exception) { + LOGGER.debug("Could not discover the local Feature Flagging {} route", eventType, exception); + return null; + } + } + + @Nullable + private BackendApi getOrCreateDirectApi() { + if (directApiCreationAttempted) { + return directApi; + } + synchronized (this) { + if (!directApiCreationAttempted) { + directApi = directApiSupplier.get(); + directApiCreationAttempted = true; } - directApiCreationAttempted = true; return directApi; } } - private static boolean isDefinitiveRejection(final IOException exception) { + private IOException unavailableRoute() { + return new IOException("No Feature Flagging " + eventType + " delivery route is available"); + } + + private static boolean isSafeToReplayDirectly(final IOException exception) { if (exception instanceof ConnectException) { return true; } if (exception instanceof HttpResponseException) { final int statusCode = ((HttpResponseException) exception).getStatusCode(); - return statusCode == 403 || statusCode == 404 || statusCode == 405; + return statusCode == 404 || statusCode == 405; } return false; } + + private static final class SelectedApi { + private final BackendApi api; + private final boolean local; + + private SelectedApi(final BackendApi api, final boolean local) { + this.api = api; + this.local = local; + } + } } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java index fcd50e5dc34..7ca34d4c6cb 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java @@ -7,6 +7,7 @@ import datadog.common.queue.MessagePassingBlockingQueue; import datadog.common.queue.Queues; +import datadog.communication.BackendApi; import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.trace.api.Config; import datadog.trace.api.featureflag.FeatureFlaggingGateway; @@ -17,6 +18,9 @@ import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -27,14 +31,31 @@ public class ExposureWriterImpl implements ExposureWriter { private static final int DEFAULT_FLUSH_INTERVAL_IN_SECONDS = 1; private static final int FLUSH_THRESHOLD = 100; private static final String EXPOSURES_ROUTE = "exposures"; + static final long SHUTDOWN_TIMEOUT_MILLIS = SECONDS.toMillis(5); private final MessagePassingBlockingQueue queue; + private final ExposureSerializingHandler serializer; private final Thread serializerThread; + private final ReentrantReadWriteLock lifecycleLock = new ReentrantReadWriteLock(); + private final AtomicBoolean closed = new AtomicBoolean(false); + private final long shutdownTimeoutMillis; public ExposureWriterImpl(final SharedCommunicationObjects sco, final Config config) { this(DEFAULT_CAPACITY, DEFAULT_FLUSH_INTERVAL_IN_SECONDS, SECONDS, sco, config); } + ExposureWriterImpl( + final SharedCommunicationObjects sco, + final Config config, + final FeatureFlagRouteSelector routeSelector) { + this( + DEFAULT_CAPACITY, + DEFAULT_FLUSH_INTERVAL_IN_SECONDS, + SECONDS, + new FeatureFlagBackendApiFactory(config, sco, FeatureFlagEventType.EXPOSURE, routeSelector), + config); + } + ExposureWriterImpl( final int capacity, final long flushInterval, @@ -55,35 +76,94 @@ public ExposureWriterImpl(final SharedCommunicationObjects sco, final Config con final TimeUnit timeUnit, final FeatureFlagBackendApiFactory backendApiFactory, final Config config) { + this( + capacity, + flushInterval, + timeUnit, + backendApiFactory::create, + config, + SHUTDOWN_TIMEOUT_MILLIS); + } + + ExposureWriterImpl( + final int capacity, + final long flushInterval, + final TimeUnit timeUnit, + final Supplier backendApiSupplier, + final Config config, + final long shutdownTimeoutMillis) { this.queue = Queues.mpscBlockingConsumerArrayQueue(capacity); - final ExposureSerializingHandler serializer = + this.serializer = new ExposureSerializingHandler( - backendApiFactory, + backendApiSupplier, queue, flushInterval, timeUnit, FeatureFlagEvpContext.from(config), this::close); this.serializerThread = newAgentThread(FEATURE_FLAG_EXPOSURE_PROCESSOR, serializer); + this.shutdownTimeoutMillis = shutdownTimeoutMillis; } @Override public void init() { - FeatureFlaggingGateway.addExposureListener(this); - this.serializerThread.start(); + lifecycleLock.writeLock().lock(); + try { + if (closed.get()) { + return; + } + FeatureFlaggingGateway.addExposureListener(this); + this.serializerThread.start(); + } finally { + lifecycleLock.writeLock().unlock(); + } } @Override public void close() { - FeatureFlaggingGateway.removeExposureListener(this); - if (this.serializerThread.isAlive()) { - this.serializerThread.interrupt(); + final boolean workerRunning; + lifecycleLock.writeLock().lock(); + try { + if (!closed.compareAndSet(false, true)) { + return; + } + // Exclude all producers before asking the single consumer to perform its final drain. + FeatureFlaggingGateway.removeExposureListener(this); + workerRunning = this.serializerThread.isAlive(); + if (workerRunning) { + serializer.requestShutdown(); + this.serializerThread.interrupt(); + } + } finally { + lifecycleLock.writeLock().unlock(); + } + + // start() failure invokes close() from the serializer itself. It cannot join itself, and no + // final flush is possible when no backend route was created. + if (!workerRunning || Thread.currentThread() == this.serializerThread) { + return; + } + try { + // Bound application shutdown even if the final best-effort network request does not return. + this.serializerThread.join(shutdownTimeoutMillis); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); } } @Override public void accept(final ExposureEvent event) { - queue.offer(event); + // Multiple producers may still enqueue concurrently under the read lock. close() takes the + // write lock, so every accepted event is in the queue before the worker's final drain begins, + // while stale CopyOnWriteArrayList dispatch snapshots are rejected after shutdown starts. + lifecycleLock.readLock().lock(); + try { + if (!closed.get()) { + queue.offer(event); + } + } finally { + lifecycleLock.readLock().unlock(); + } } @VisibleForTesting @@ -107,9 +187,10 @@ private static class ExposureSerializingHandler implements Runnable { private final List buffer = new ArrayList<>(); private final Runnable errorCallback; + private final AtomicBoolean shutdownRequested = new AtomicBoolean(false); ExposureSerializingHandler( - final FeatureFlagBackendApiFactory backendApiFactory, + final Supplier backendApiSupplier, final MessagePassingBlockingQueue queue, final long flushInterval, final TimeUnit timeUnit, @@ -117,8 +198,7 @@ private static class ExposureSerializingHandler implements Runnable { final Runnable errorCallback) { this.queue = queue; this.cache = new LRUExposureCache(queue.capacity()); - this.evpPublisher = - new FeatureFlagEvpPublisher<>(backendApiFactory::create, ExposuresRequest.class); + this.evpPublisher = new FeatureFlagEvpPublisher<>(backendApiSupplier, ExposuresRequest.class); this.context = context; this.lastTicks = System.nanoTime(); @@ -129,6 +209,10 @@ private static class ExposureSerializingHandler implements Runnable { LOGGER.debug("starting exposure serializer"); } + void requestShutdown() { + shutdownRequested.set(true); + } + @Override public void run() { if (!evpPublisher.start()) { @@ -140,13 +224,25 @@ public void run() { runDutyCycle(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); + } finally { + // close() interrupts this thread to wake queue.poll(). OkHttp fails fast when the calling + // thread is interrupted, so clear the flag for the final best-effort drain and restore it + // only after the final send-once attempt has completed. + final boolean wasInterrupted = Thread.interrupted(); + try { + drainAndFlush(); + } finally { + if (wasInterrupted) { + Thread.currentThread().interrupt(); + } + } } LOGGER.debug("exposure processor worker exited. submitting exposures stopped."); } private void runDutyCycle() throws InterruptedException { final Thread thread = Thread.currentThread(); - while (!thread.isInterrupted()) { + while (!thread.isInterrupted() && !shutdownRequested.get()) { ExposureEvent event; while ((event = queue.poll(100, TimeUnit.MILLISECONDS)) != null) { if (addToBuffer(event)) { @@ -162,6 +258,14 @@ private void consumeBatch() { queue.drain(this::addToBuffer, queue.size()); } + private void drainAndFlush() { + ExposureEvent event; + while ((event = queue.poll()) != null) { + addToBuffer(event); + } + flush(); + } + /** Adds an element to the buffer taking care of duplicated exposures thanks to the LRU cache */ private boolean addToBuffer(final ExposureEvent event) { if (cache.add(event)) { @@ -172,35 +276,39 @@ private boolean addToBuffer(final ExposureEvent event) { } protected void flushIfNecessary() { + if (!buffer.isEmpty() && shouldFlush()) { + flush(); + } + } + + private void flush() { if (buffer.isEmpty()) { return; } - if (shouldFlush()) { - final byte[] payload; - try { - final ExposuresRequest exposures = new ExposuresRequest(this.context, this.buffer); - payload = evpPublisher.serialize(exposures); - } catch (RuntimeException e) { - LOGGER.error(EXCLUDE_TELEMETRY, "Could not serialize exposures; dropping batch", e); - this.buffer.clear(); - return; - } - try { - evpPublisher.post(EXPOSURES_ROUTE, payload); - } catch (Exception e) { - LOGGER.debug("Could not submit exposures", e); - } finally { - // Best-effort delivery must not retry an ambiguously accepted batch. A later definitive - // proxy rejection could otherwise replay the same exposures through direct intake. - this.buffer.clear(); - } + final byte[] payload; + try { + final ExposuresRequest exposures = new ExposuresRequest(this.context, this.buffer); + payload = evpPublisher.serialize(exposures); + } catch (RuntimeException e) { + LOGGER.error(EXCLUDE_TELEMETRY, "Could not serialize exposures; dropping batch", e); + this.buffer.clear(); + return; + } + try { + evpPublisher.post(EXPOSURES_ROUTE, payload); + } catch (Exception e) { + LOGGER.debug("Could not submit exposures", e); + } finally { + // Best-effort delivery must not retry an ambiguously accepted batch. A later definitive + // proxy rejection could otherwise replay the same exposures through direct intake. + this.buffer.clear(); } } private boolean shouldFlush() { long nanoTime = System.nanoTime(); long ticks = nanoTime - lastTicks; - if (ticks > ticksRequiredToFlush || queue.size() >= FLUSH_THRESHOLD) { + if (ticks > ticksRequiredToFlush || buffer.size() >= FLUSH_THRESHOLD) { lastTicks = nanoTime; return true; } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java index 451b903ecc7..7c41efcc551 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java @@ -1,13 +1,21 @@ package com.datadog.featureflag; +import static datadog.communication.EvpProxy.JAVA_TRACING_LIBRARY; +import static datadog.communication.EvpProxy.ORIGIN_HEADER; +import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; +import static java.util.Collections.unmodifiableMap; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.communication.ddagent.TracerVersion; import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -16,66 +24,95 @@ final class FeatureFlagBackendApiFactory { private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagBackendApiFactory.class); + private static final Map REQUEST_HEADERS = requestHeaders(); private final Config config; private final BackendApiFactory backendApiFactory; private final FeatureFlagEventType eventType; + private final FeatureFlagRouteSelector routeSelector; FeatureFlagBackendApiFactory( final Config config, final SharedCommunicationObjects sharedCommunicationObjects, final FeatureFlagEventType eventType) { - this(config, new BackendApiFactory(config, sharedCommunicationObjects), eventType); + this(config, sharedCommunicationObjects, eventType, new FeatureFlagRouteSelector()); + } + + FeatureFlagBackendApiFactory( + final Config config, + final SharedCommunicationObjects sharedCommunicationObjects, + final FeatureFlagEventType eventType, + final FeatureFlagRouteSelector routeSelector) { + this( + config, + new BackendApiFactory(config, sharedCommunicationObjects, REQUEST_HEADERS, true), + eventType, + routeSelector); } FeatureFlagBackendApiFactory( final Config config, final BackendApiFactory backendApiFactory, final FeatureFlagEventType eventType) { + this(config, backendApiFactory, eventType, new FeatureFlagRouteSelector()); + } + + FeatureFlagBackendApiFactory( + final Config config, + final BackendApiFactory backendApiFactory, + final FeatureFlagEventType eventType, + final FeatureFlagRouteSelector routeSelector) { this.config = config; this.backendApiFactory = backendApiFactory; this.eventType = eventType; + this.routeSelector = routeSelector; } @Nullable BackendApi create() { - final boolean directFallbackAvailable = - CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource()) - && hasDirectCredentials(); - final BackendApi proxyApi = - directFallbackAvailable - ? backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, - eventType.responseCompressionEnabled(), - HttpRetryPolicy.Factory.NEVER_RETRY) - : backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, eventType.responseCompressionEnabled()); - if (!CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource())) { - if (proxyApi == null) { - LOGGER.warn( - "Feature Flagging {} delivery is disabled because the local Agent does not support the EVP proxy", - eventType.logName()); - } - return proxyApi; - } - - if (proxyApi != null) { - if (directFallbackAvailable) { - return new AgentlessFeatureFlagBackendApi( - proxyApi, this::createDirectApi, eventType.logName()); - } - return proxyApi; + final boolean agentless = + CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource()); + if (!agentless) { + // Preserve the historical Agent-backed client: fixed EVP v2, no /info discovery, no direct + // credentials, and no Agentless route state. + return backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + eventType.responseCompressionEnabled(), + HttpRetryPolicy.Factory.NEVER_RETRY, + V2_EVP_PROXY_ENDPOINT); } + final BackendApi proxyApi = createProxyApi(false); final BackendApi directApi = createDirectApi(); - if (directApi != null) { - return directApi; + if (proxyApi == null && directApi == null) { + LOGGER.warn( + "Feature Flagging {} delivery is waiting for a compatible local EVP proxy because direct intake credentials are unavailable", + eventType.logName()); } + return new AgentlessFeatureFlagBackendApi( + proxyApi, + directApi, + () -> createProxyApi(true), + this::createDirectApi, + eventType.logName(), + routeSelector); + } + + @Nullable + private BackendApi createProxyApi(final boolean forceDiscovery) { + return backendApiFactory.createEvpProxyApi( + Intake.EVENT_PLATFORM, + eventType.responseCompressionEnabled(), + HttpRetryPolicy.Factory.NEVER_RETRY, + forceDiscovery, + true); + } - LOGGER.warn( - "Feature Flagging {} delivery is disabled because no compatible local EVP proxy or direct intake credentials are available", - eventType.logName()); - return null; + private static Map requestHeaders() { + final Map headers = new HashMap<>(2); + headers.put(ORIGIN_HEADER, JAVA_TRACING_LIBRARY); + headers.put(ORIGIN_VERSION_HEADER, TracerVersion.TRACER_VERSION); + return unmodifiableMap(headers); } private boolean hasDirectCredentials() { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java new file mode 100644 index 00000000000..da3a2da2125 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java @@ -0,0 +1,79 @@ +package com.datadog.featureflag; + +import java.util.concurrent.TimeUnit; +import java.util.function.LongSupplier; + +/** Process-wide route state shared by all Feature Flagging event writers. */ +final class FeatureFlagRouteSelector { + + static final long DEFAULT_RECOVERY_INTERVAL_NANOS = TimeUnit.MINUTES.toNanos(1); + + enum Route { + UNINITIALIZED, + LOCAL, + DIRECT, + UNAVAILABLE + } + + private final LongSupplier nanoTime; + private final long recoveryIntervalNanos; + private volatile Route route = Route.UNINITIALIZED; + private volatile long nextLocalDiscoveryNanos; + + FeatureFlagRouteSelector() { + this(System::nanoTime, DEFAULT_RECOVERY_INTERVAL_NANOS); + } + + FeatureFlagRouteSelector(final LongSupplier nanoTime, final long recoveryIntervalNanos) { + this.nanoTime = nanoTime; + this.recoveryIntervalNanos = recoveryIntervalNanos; + } + + synchronized Route initialize(final boolean localAvailable, final boolean directAvailable) { + if (route == Route.UNINITIALIZED || route == Route.UNAVAILABLE) { + if (localAvailable) { + route = Route.LOCAL; + } else if (directAvailable) { + route = Route.DIRECT; + } else { + route = Route.UNAVAILABLE; + scheduleLocalDiscovery(); + } + } + return route; + } + + Route current() { + return route; + } + + synchronized Route localFailure(final boolean directAvailable) { + if (route == Route.LOCAL) { + if (directAvailable) { + route = Route.DIRECT; + } else { + route = Route.UNAVAILABLE; + scheduleLocalDiscovery(); + } + } + return route; + } + + synchronized boolean tryBeginLocalRecovery() { + if (route != Route.UNAVAILABLE || nanoTime.getAsLong() - nextLocalDiscoveryNanos < 0) { + return false; + } + scheduleLocalDiscovery(); + return true; + } + + synchronized void localRecovered() { + if (route == Route.UNAVAILABLE) { + route = Route.LOCAL; + } + } + + private void scheduleLocalDiscovery() { + nextLocalDiscoveryNanos = nanoTime.getAsLong() + recoveryIntervalNanos; + } +} 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 2da8ffe0256..4969376771c 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 @@ -108,6 +108,20 @@ public FlagEvaluationWriterImpl(final SharedCommunicationObjects sco, final Conf config); } + FlagEvaluationWriterImpl( + final SharedCommunicationObjects sco, + final Config config, + final FeatureFlagRouteSelector routeSelector) { + this( + DEFAULT_CAPACITY, + FLUSH_INTERVAL_SECONDS, + SECONDS, + new FeatureFlagBackendApiFactory( + config, sco, FeatureFlagEventType.FLAG_EVALUATION, routeSelector) + ::create, + config); + } + FlagEvaluationWriterImpl( final int capacity, final long flushInterval, diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java index 117c72ba2a1..012ba26ea01 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import datadog.communication.BackendApi; import datadog.communication.HttpResponseException; @@ -15,7 +16,11 @@ import java.net.SocketTimeoutException; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Stream; import javax.annotation.Nullable; import okhttp3.MediaType; @@ -29,7 +34,7 @@ class AgentlessFeatureFlagBackendApiTest { @ParameterizedTest - @ValueSource(ints = {403, 404, 405}) + @ValueSource(ints = {404, 405}) void replaysRejectedBatchDirectlyAndKeepsDirectRoute(final int statusCode) throws Exception { final RecordingBackendApi local = new RecordingBackendApi(new HttpResponseException(statusCode, "rejected")); @@ -38,6 +43,8 @@ void replaysRejectedBatchDirectlyAndKeepsDirectRoute(final int statusCode) throw final AgentlessFeatureFlagBackendApi api = new AgentlessFeatureFlagBackendApi( local, + null, + () -> local, () -> { directApiCreations.incrementAndGet(); return direct; @@ -66,7 +73,7 @@ void fallsBackAfterConnectionRefusal(final String route, final String eventType) new RecordingBackendApi(new ConnectException("connection refused")); final RecordingBackendApi direct = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi(local, () -> direct, eventType); + new AgentlessFeatureFlagBackendApi(local, null, () -> local, () -> direct, eventType); api.post(route, requestBody(eventType), stream -> null, null, false); @@ -75,37 +82,154 @@ void fallsBackAfterConnectionRefusal(final String route, final String eventType) } @Test - void doesNotReturnToLocalRouteAfterSwitchingToDirectIntake() throws Exception { - final RecordingBackendApi local = + void keepsDirectRouteStickyAfterLocalFailure() throws Exception { + final AtomicLong clock = new AtomicLong(); + final RecordingBackendApi unavailableLocal = new RecordingBackendApi(new ConnectException("connection refused")); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); final RecordingBackendApi direct = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi(local, () -> direct, "exposure"); + new AgentlessFeatureFlagBackendApi( + unavailableLocal, null, () -> recoveredLocal, () -> direct, "exposure", clock::get, 10); api.post("exposures", requestBody("first"), stream -> null, null, false); - direct.failure = new IOException("direct intake failed"); + clock.set(9); + api.post("exposures", requestBody("second"), stream -> null, null, false); + clock.set(10); + api.post("exposures", requestBody("third"), stream -> null, null, false); - assertThrows( - IOException.class, - () -> api.post("exposures", requestBody("second"), stream -> null, null, false)); - assertEquals(1, local.calls); - assertEquals(2, direct.calls); + assertEquals(1, unavailableLocal.calls); + assertEquals(3, direct.calls); + assertEquals(0, recoveredLocal.calls); } @ParameterizedTest - @ValueSource(ints = {429, 500}) - void doesNotReplayAmbiguousHttpFailure(final int statusCode) { - assertNoDirectReplay(new HttpResponseException(statusCode, "ambiguous")); + @ValueSource(ints = {403, 429, 500}) + void doesNotReplayAmbiguousHttpFailureButSwitchesFutureBatches(final int statusCode) + throws Exception { + assertNoSameBatchReplayButUsesDirectForNext(new HttpResponseException(statusCode, "ambiguous")); + } + + @Test + void doesNotReplayTimeoutButSwitchesFutureBatches() throws Exception { + assertNoSameBatchReplayButUsesDirectForNext(new SocketTimeoutException("timed out")); + } + + @Test + void doesNotReplayConnectionResetButSwitchesFutureBatches() throws Exception { + assertNoSameBatchReplayButUsesDirectForNext(new SocketException("connection reset")); } @Test - void doesNotReplayTimeout() { - assertNoDirectReplay(new SocketTimeoutException("timed out")); + void startsDirectAndNeverProbesLocal() throws Exception { + final AtomicLong clock = new AtomicLong(); + final RecordingBackendApi direct = new RecordingBackendApi(); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + direct, + () -> { + proxyApiCreations.incrementAndGet(); + return recoveredLocal; + }, + () -> direct, + "exposure", + clock::get, + 10); + + api.post("exposures", requestBody("first"), stream -> null, null, false); + clock.set(9); + api.post("exposures", requestBody("second"), stream -> null, null, false); + clock.set(10); + api.post("exposures", requestBody("third"), stream -> null, null, false); + + assertEquals(0, proxyApiCreations.get()); + assertEquals(3, direct.calls); + assertEquals(0, recoveredLocal.calls); + } + + @Test + void concurrentSendersDoNotBlockOnOrDuplicateARecoveryProbe() throws Exception { + final AtomicLong clock = new AtomicLong(10); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final CountDownLatch probeStarted = new CountDownLatch(1); + final CountDownLatch releaseProbe = new CountDownLatch(1); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + null, + () -> { + proxyApiCreations.incrementAndGet(); + probeStarted.countDown(); + try { + assertTrue(releaseProbe.await(5, TimeUnit.SECONDS)); + } catch (final InterruptedException exception) { + Thread.currentThread().interrupt(); + throw new AssertionError(exception); + } + return recoveredLocal; + }, + () -> null, + "flag evaluation", + clock::get, + 10); + + // Construction schedules the first probe for t=20. + clock.set(20); + final CompletableFuture recoveringPost = + CompletableFuture.runAsync( + () -> { + try { + api.post("flagevaluation", requestBody("probe"), stream -> null, null, false); + } catch (final IOException exception) { + throw new AssertionError(exception); + } + }); + assertTrue(probeStarted.await(5, TimeUnit.SECONDS)); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("parallel"), stream -> null, null, false)); + releaseProbe.countDown(); + recoveringPost.get(5, TimeUnit.SECONDS); + + assertEquals(1, proxyApiCreations.get()); + assertEquals(1, recoveredLocal.calls); } @Test - void doesNotReplayConnectionReset() { - assertNoDirectReplay(new SocketException("connection reset")); + void failedRecoveryIsStickyForAnotherCooldown() throws Exception { + final AtomicLong clock = new AtomicLong(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + null, + () -> { + proxyApiCreations.incrementAndGet(); + return null; + }, + () -> null, + "exposure", + clock::get, + 10); + + clock.set(10); + assertThrows( + IOException.class, + () -> api.post("exposures", requestBody("first"), stream -> null, null, false)); + assertThrows( + IOException.class, + () -> api.post("exposures", requestBody("second"), stream -> null, null, false)); + clock.set(20); + assertThrows( + IOException.class, + () -> api.post("exposures", requestBody("third"), stream -> null, null, false)); + + assertEquals(2, proxyApiCreations.get()); } @Test @@ -116,6 +240,8 @@ void doesNotRetryDirectApiCreationWhenFallbackIsUnavailable() { final AgentlessFeatureFlagBackendApi api = new AgentlessFeatureFlagBackendApi( local, + null, + () -> local, () -> { directApiCreations.incrementAndGet(); return null; @@ -126,20 +252,183 @@ void doesNotRetryDirectApiCreationWhenFallbackIsUnavailable() { HttpResponseException.class, () -> api.post("exposures", requestBody("first"), stream -> null, null, false)); assertThrows( - HttpResponseException.class, + IOException.class, () -> api.post("exposures", requestBody("second"), stream -> null, null, false)); - assertEquals(2, local.calls); + assertEquals(1, local.calls); + assertEquals(1, directApiCreations.get()); + } + + @Test + void permitsUnavailableStartupSoLocalDeliveryCanRecover() { + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi(null, null, () -> null, () -> null, "flag evaluation"); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("first"), stream -> null, null, false)); + } + + @Test + void propagatesDirectFailuresWithoutTryingToFallback() { + final SocketTimeoutException failure = new SocketTimeoutException("direct timeout"); + final RecordingBackendApi direct = new RecordingBackendApi(failure); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + direct, + () -> { + proxyApiCreations.incrementAndGet(); + return null; + }, + () -> direct, + "exposure"); + + assertSame( + failure, + assertThrows( + SocketTimeoutException.class, + () -> api.post("exposures", requestBody("direct"), stream -> null, null, false))); + assertEquals(0, proxyApiCreations.get()); + assertEquals(1, direct.calls); + } + + @Test + void concurrentProxyFailuresOnlyTransitionTheMatchingRouteOnce() throws Exception { + final CountDownLatch localCallsStarted = new CountDownLatch(2); + final CountDownLatch releaseFailures = new CountDownLatch(1); + final BackendApi local = + new BackendApi() { + @Override + public T post( + final String uri, + final RequestBody requestBody, + final IOThrowingFunction responseParser, + @Nullable final OkHttpUtils.CustomListener requestListener, + final boolean requestCompression) + throws IOException { + localCallsStarted.countDown(); + try { + assertTrue(releaseFailures.await(5, TimeUnit.SECONDS)); + } catch (final InterruptedException exception) { + Thread.currentThread().interrupt(); + throw new AssertionError(exception); + } + throw new HttpResponseException(404, "local route missing"); + } + }; + final AtomicInteger directCalls = new AtomicInteger(); + final BackendApi direct = + new BackendApi() { + @Override + public T post( + final String uri, + final RequestBody requestBody, + final IOThrowingFunction responseParser, + @Nullable final OkHttpUtils.CustomListener requestListener, + final boolean requestCompression) { + directCalls.incrementAndGet(); + return null; + } + }; + final AtomicInteger directApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + local, + null, + () -> local, + () -> { + directApiCreations.incrementAndGet(); + return direct; + }, + "exposure"); + + final CompletableFuture first = + CompletableFuture.runAsync(() -> postWithoutFailure(api, "first concurrent proxy request")); + final CompletableFuture second = + CompletableFuture.runAsync( + () -> postWithoutFailure(api, "second concurrent proxy request")); + assertTrue(localCallsStarted.await(5, TimeUnit.SECONDS)); + releaseFailures.countDown(); + first.get(5, TimeUnit.SECONDS); + second.get(5, TimeUnit.SECONDS); + assertEquals(1, directApiCreations.get()); + assertEquals(2, directCalls.get()); + } + + @Test + void sharesRouteTransitionsAcrossExposureAndFlagEvaluationWriters() throws Exception { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + final RecordingBackendApi exposureLocal = + new RecordingBackendApi(new SocketTimeoutException("ambiguous timeout")); + final RecordingBackendApi exposureDirect = new RecordingBackendApi(); + final RecordingBackendApi evaluationLocal = new RecordingBackendApi(); + final RecordingBackendApi evaluationDirect = new RecordingBackendApi(); + final AgentlessFeatureFlagBackendApi exposureApi = + new AgentlessFeatureFlagBackendApi( + exposureLocal, + exposureDirect, + () -> exposureLocal, + () -> exposureDirect, + "exposure", + routeSelector); + final AgentlessFeatureFlagBackendApi evaluationApi = + new AgentlessFeatureFlagBackendApi( + evaluationLocal, + evaluationDirect, + () -> evaluationLocal, + () -> evaluationDirect, + "flag evaluation", + routeSelector); + + assertThrows( + SocketTimeoutException.class, + () -> exposureApi.post("exposures", requestBody("first"), stream -> null, null, false)); + evaluationApi.post("flagevaluation", requestBody("second"), stream -> null, null, false); + + assertEquals(1, exposureLocal.calls); + assertEquals(0, exposureDirect.calls); + assertEquals(0, evaluationLocal.calls); + assertEquals(1, evaluationDirect.calls); + } + + @Test + void workingDirectRouteDoesNotAttemptRecovery() throws Exception { + final AtomicLong clock = new AtomicLong(); + final RecordingBackendApi direct = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + direct, + () -> { + proxyApiCreations.incrementAndGet(); + throw new IllegalStateException("discovery failed"); + }, + () -> direct, + "exposure", + clock::get, + 10); + + clock.set(10); + api.post("exposures", requestBody("survives recovery failure"), stream -> null, null, false); + + assertEquals(1, direct.calls); + assertEquals(0, proxyApiCreations.get()); } - private static void assertNoDirectReplay(final IOException failure) { + private static void assertNoSameBatchReplayButUsesDirectForNext(final IOException failure) + throws Exception { final RecordingBackendApi local = new RecordingBackendApi(failure); final RecordingBackendApi direct = new RecordingBackendApi(); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = new AgentlessFeatureFlagBackendApi( local, + null, + () -> local, () -> { directApiCreations.incrementAndGet(); return direct; @@ -149,16 +438,26 @@ private static void assertNoDirectReplay(final IOException failure) { assertThrows( IOException.class, () -> api.post("flagevaluation", requestBody("evaluation"), stream -> null, null, false)); + api.post("flagevaluation", requestBody("next"), stream -> null, null, false); assertEquals(1, local.calls); - assertEquals(0, direct.calls); - assertEquals(0, directApiCreations.get()); + assertEquals(1, direct.calls); + assertEquals(1, directApiCreations.get()); } private static RequestBody requestBody(final String value) { return RequestBody.create(MediaType.parse("application/json"), value); } + private static void postWithoutFailure( + final AgentlessFeatureFlagBackendApi api, final String body) { + try { + api.post("exposures", requestBody(body), stream -> null, null, false); + } catch (final IOException exception) { + throw new AssertionError(exception); + } + } + private static Stream featureFlagRoutes() { return Stream.of( Arguments.of("exposures", "exposure"), Arguments.of("flagevaluation", "flag evaluation")); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index f70a4ff0fa1..a0c4f7d59ee 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -4,6 +4,7 @@ import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -57,6 +58,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicBoolean; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; import okhttp3.RequestBody; @@ -65,20 +67,19 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.tabletest.junit.TableTest; class ExposureWriterTests { - private static final String EXPOSURES_ENDPOINT = "/evp_proxy/api/v2/exposures"; + private static final String EXPOSURES_ENDPOINT = "/evp_proxy/v2/api/v2/exposures"; private static final String DIRECT_EXPOSURES_ENDPOINT = "/api/v2/exposures"; private static final String API_KEY = "test-api-key"; private static final double TIMEOUT_SECONDS = 5; private final PollingConditions poll = new PollingConditions(TIMEOUT_SECONDS); private Queue requests; + private Queue requestAttempts; private Set failed; private JavaTestHttpServer server; private SharedCommunicationObjects sharedCommunicationObjects; @@ -86,6 +87,7 @@ class ExposureWriterTests { @BeforeEach void setUp() { requests = new ConcurrentLinkedQueue<>(); + requestAttempts = new ConcurrentLinkedQueue<>(); failed = Collections.newSetFromMap(new ConcurrentHashMap()); JsonAdapter adapter = new Moshi.Builder().build().adapter(ExposuresRequest.class); @@ -114,6 +116,7 @@ private void handleExposureRequest(HandlerApi api, JsonAdapter adapter.fromJson( Okio.buffer(Okio.source(new ByteArrayInputStream(api.getRequest().getBody())))); String serviceName = exposuresRequest.context.get("service"); + requestAttempts.add(serviceName); boolean failForever = "fail-forever".equals(serviceName); boolean fail = serviceName.startsWith("fail") && (failed.add(serviceName) || failForever); if (fail) { @@ -262,10 +265,183 @@ void testHighLoadScenario() throws Exception { } } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - void testFailuresAreRetried(boolean finallyFail) throws Exception { - String serviceName = finallyFail ? "fail-forever" : "fail-once"; + @Test + void testQueueThresholdFlushesWithoutWaitingForTheInterval() throws Exception { + Config config = mockConfig("threshold-service"); + List exposures = buildExposures(101); + + try (ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 8, Long.MAX_VALUE, NANOSECONDS, sharedCommunicationObjects, config)) { + for (ExposureEvent exposure : exposures) { + writer.accept(exposure); + } + writer.init(); + + poll.eventually(() -> assertExposures(allExposures(), exposures)); + } + } + + @Test + void testCloseDrainsAndFinalFlushesExactlyOnceWithoutWaitingForTheInterval() throws Exception { + Config config = mockConfig("shutdown-service"); + List exposures = buildExposures(5); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, Long.MAX_VALUE, NANOSECONDS, sharedCommunicationObjects, config); + + writer.init(); + for (ExposureEvent exposure : exposures) { + writer.accept(exposure); + } + + writer.close(); + + assertFalse(writer.isSerializerThreadAlive()); + assertEquals(1, requests.size()); + assertExposures(allExposures(), exposures); + + // A repeated close and a stale listener invocation after close must neither replay the batch + // nor leave an event stranded in the queue. + writer.close(); + writer.accept(buildExposure()); + MILLISECONDS.sleep(200); + assertEquals(1, requests.size()); + assertEquals(0, writer.queueSize()); + } + + @Test + void testFinalFlushRunsWithoutTheInterruptFlagSet() throws Exception { + BackendApi backendApi = mock(BackendApi.class); + AtomicBoolean interruptedDuringPost = new AtomicBoolean(true); + when(backendApi.post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false))) + .thenAnswer( + invocation -> { + interruptedDuringPost.set(Thread.currentThread().isInterrupted()); + return null; + }); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + () -> backendApi, + mockConfig("shutdown-service"), + ExposureWriterImpl.SHUTDOWN_TIMEOUT_MILLIS); + + writer.init(); + writer.accept(buildExposure()); + writer.close(); + + verify(backendApi, times(1)) + .post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false)); + assertFalse( + interruptedDuringPost.get(), + "The final HTTP request must not inherit the interrupt used to wake queue polling"); + assertFalse(writer.isSerializerThreadAlive()); + } + + @Test + void testCloseWaitIsBoundedWhenFinalPostDoesNotReturn() throws Exception { + BackendApi backendApi = mock(BackendApi.class); + CountDownLatch postStarted = new CountDownLatch(1); + CountDownLatch releasePost = new CountDownLatch(1); + when(backendApi.post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false))) + .thenAnswer( + invocation -> { + postStarted.countDown(); + releasePost.await(); + return null; + }); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + () -> backendApi, + mockConfig("blocked-shutdown-service"), + 100); + + try { + writer.init(); + writer.accept(buildExposure()); + long start = System.nanoTime(); + + writer.close(); + + long elapsedMillis = NANOSECONDS.toMillis(System.nanoTime() - start); + assertTrue(postStarted.await(1, java.util.concurrent.TimeUnit.SECONDS)); + assertTrue(elapsedMillis < 2000, "close exceeded its configured bounded wait"); + assertTrue(writer.isSerializerThreadAlive()); + } finally { + releasePost.countDown(); + } + poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); + } + + @Test + void testStaleGatewayDispatchCannotEnqueueAfterClose() throws Exception { + CountDownLatch dispatchSnapshotTaken = new CountDownLatch(1); + CountDownLatch releaseDispatch = new CountDownLatch(1); + FeatureFlaggingGateway.ExposureListener blocker = + ignored -> { + dispatchSnapshotTaken.countDown(); + try { + releaseDispatch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }; + FeatureFlaggingGateway.addExposureListener(blocker); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + sharedCommunicationObjects, + mockConfig("stale-dispatch-service")); + Thread dispatcher = + new Thread(() -> FeatureFlaggingGateway.dispatch(buildExposure()), "exposure-dispatcher"); + + try { + writer.init(); + dispatcher.start(); + assertTrue(dispatchSnapshotTaken.await(5, java.util.concurrent.TimeUnit.SECONDS)); + + writer.close(); + releaseDispatch.countDown(); + dispatcher.join(5000); + + assertFalse(dispatcher.isAlive()); + assertFalse(writer.isSerializerThreadAlive()); + assertEquals(0, writer.queueSize()); + assertTrue(requests.isEmpty()); + } finally { + releaseDispatch.countDown(); + dispatcher.join(5000); + FeatureFlaggingGateway.removeExposureListener(blocker); + writer.close(); + } + } + + @Test + void testCloseBeforeInitPreventsLaterStartAndAccept() { + ExposureWriterImpl writer = + new ExposureWriterImpl(sharedCommunicationObjects, mockConfig("never-started-service")); + + writer.close(); + writer.init(); + writer.accept(buildExposure()); + writer.close(); + + assertFalse(writer.isSerializerThreadAlive()); + assertEquals(0, writer.queueSize()); + assertTrue(requests.isEmpty()); + } + + @Test + void testHttpFailureIsNotRetriedAtTransportLayer() throws Exception { + String serviceName = "fail-once"; Config config = mockConfig(serviceName); try (ExposureWriterImpl writer = @@ -273,13 +449,11 @@ void testFailuresAreRetried(boolean finallyFail) throws Exception { writer.init(); writer.accept(buildExposure()); - MILLISECONDS.sleep(500); // wait for a flush to happen - ExposuresRequest found = findRequest(serviceName); - if (finallyFail) { - assertNull(found, requests.toString()); - } else { - poll.eventually(() -> assertNotNull(findRequest(serviceName), requests.toString())); - } + poll.eventually(() -> assertEquals(1, Collections.frequency(requestAttempts, serviceName))); + MILLISECONDS.sleep(500); + + assertEquals(1, Collections.frequency(requestAttempts, serviceName)); + assertNull(findRequest(serviceName), requests.toString()); } } @@ -309,7 +483,7 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception final BackendApi proxyApi = mock(BackendApi.class); final BackendApi directApi = mock(BackendApi.class); when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, true, HttpRetryPolicy.Factory.NEVER_RETRY)) + Intake.EVENT_PLATFORM, true, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)) .thenReturn(proxyApi); when(backendApiFactory.createDirectIntakeApi(eq(Intake.EVENT_PLATFORM), eq(true), eq(false))) .thenReturn(directApi); @@ -353,17 +527,14 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception } @Test - void testWriterStopsReceivingExposuresIfEvpProxyIsNotAvailable() throws Exception { + void testAgentlessWriterWaitsForUnavailableProxyRecovery() throws Exception { SharedCommunicationObjects sharedCommunicationObjects = sharedCommunicationObjects(false); + Config config = mockConfig("unavailable-service"); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); - try (ExposureWriterImpl writer = - new ExposureWriterImpl(sharedCommunicationObjects, Config.get())) { + try (ExposureWriterImpl writer = new ExposureWriterImpl(sharedCommunicationObjects, config)) { writer.init(); - poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); - - FeatureFlaggingGateway.dispatch(buildExposure()); - - assertEquals(0, writer.queueSize()); + poll.eventually(() -> assertTrue(writer.isSerializerThreadAlive())); } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java index b89cf3ce2b1..58811e0bfb6 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java @@ -2,10 +2,10 @@ import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; import static com.datadog.featureflag.FeatureFlagEventType.FLAG_EVALUATION; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_REMOTE_CONFIG; import static org.junit.jupiter.api.Assertions.assertInstanceOf; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -22,38 +22,33 @@ class FeatureFlagBackendApiFactoryTest { @Test - void remoteConfigUsesOnlyLocalEvpProxy() { + void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { final Config config = config(CONFIGURATION_SOURCE_REMOTE_CONFIG, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); final BackendApi proxyApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi(Intake.EVENT_PLATFORM, false)).thenReturn(proxyApi); + when(backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + V2_EVP_PROXY_ENDPOINT)) + .thenReturn(proxyApi); final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); assertSame(proxyApi, selected); + verify(backendApiFactory, never()) + .createEvpProxyApi( + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @Test - void remoteConfigDisablesDeliveryWhenLocalEvpProxyIsUnavailable() { - final Config config = config(CONFIGURATION_SOURCE_REMOTE_CONFIG, "api-key"); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, EXPOSURE).create(); - - assertNull(selected); - verify(backendApiFactory).createEvpProxyApi(Intake.EVENT_PLATFORM, true); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, true, false); - } - - @Test - void agentlessPrefersLocalEvpProxyWithDirectFallback() { + void agentlessPrefersCapabilityGatedLocalRouteWithDirectFallbackReady() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY)) + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)) .thenReturn(mock(BackendApi.class)); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenReturn(mock(BackendApi.class)); @@ -63,68 +58,43 @@ void agentlessPrefersLocalEvpProxyWithDirectFallback() { assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); verify(backendApiFactory) - .createEvpProxyApi(Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); + .createEvpProxyApi( + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true); + verify(backendApiFactory).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @Test - void agentlessUsesDirectIntakeWhenLocalEvpProxyIsUnavailable() { + void agentlessUsesDirectIntakeWhenLocalRouteIsUnavailable() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi directApi = mock(BackendApi.class); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) - .thenReturn(directApi); - - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - - assertSame(directApi, selected); - } - - @Test - void agentlessUsesLocalEvpProxyWhenApiKeyIsUnavailable() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, null); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi proxyApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi(Intake.EVENT_PLATFORM, false)).thenReturn(proxyApi); + .thenReturn(mock(BackendApi.class)); final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - assertSame(proxyApi, selected); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); + assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); } @Test - void agentlessDisablesDeliveryWhenNoRouteIsAvailable() { + void agentlessKeepsWriterAliveWhileEveryRouteIsUnavailable() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, null); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - - assertNull(selected); - } - - @Test - void agentlessDisablesDeliveryWhenApiKeyIsEmpty() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, ""); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, EXPOSURE).create(); - assertNull(selected); + assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, true, false); } @Test - void agentlessDoesNotValidateDirectUrlWhileLocalRouteIsAvailable() { + void agentlessKeepsCompatibleLocalRouteWhenDirectUrlIsInvalid() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); final BackendApi proxyApi = mock(BackendApi.class); when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY)) + Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)) .thenReturn(proxyApi); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenThrow(new IllegalArgumentException("invalid URL")); @@ -133,20 +103,6 @@ void agentlessDoesNotValidateDirectUrlWhileLocalRouteIsAvailable() { new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); - } - - @Test - void agentlessDisablesDeliveryWhenDirectUrlIsInvalidAndLocalRouteIsUnavailable() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) - .thenThrow(new IllegalArgumentException("invalid URL")); - - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - - assertNull(selected); } private static Config config(final String source, final String apiKey) { From e20a9d77c333ddb0c0444835f23fafba70952373 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Sat, 12 Sep 2026 04:26:30 +0000 Subject: [PATCH 2/8] test(openfeature): cover EVP fallback edge states --- .../AgentlessFeatureFlagBackendApiTest.java | 75 +++++++++++++++++++ .../featureflag/ExposureWriterTests.java | 24 ++++++ .../FeatureFlagRouteSelectorTest.java | 33 ++++++++ 3 files changed, 132 insertions(+) create mode 100644 products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java index 012ba26ea01..69792ae35bf 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java @@ -394,6 +394,81 @@ void sharesRouteTransitionsAcrossExposureAndFlagEvaluationWriters() throws Excep assertEquals(1, evaluationDirect.calls); } + @Test + void sharedLocalRouteDiscoversWriterSpecificProxy() throws Exception { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + routeSelector.initialize(true, false); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + null, + () -> { + proxyApiCreations.incrementAndGet(); + return recoveredLocal; + }, + () -> null, + "flag evaluation", + routeSelector); + + api.post("flagevaluation", requestBody("local"), stream -> null, null, false); + + assertEquals(1, proxyApiCreations.get()); + assertEquals(1, recoveredLocal.calls); + assertEquals(FeatureFlagRouteSelector.Route.LOCAL, routeSelector.current()); + } + + @Test + void sharedLocalRouteUsesDirectWhenWriterSpecificProxyIsUnavailable() throws Exception { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + routeSelector.initialize(true, false); + final RecordingBackendApi direct = new RecordingBackendApi(); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, direct, () -> null, () -> direct, "flag evaluation", routeSelector); + + api.post("flagevaluation", requestBody("direct"), stream -> null, null, false); + + assertEquals(1, direct.calls); + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); + } + + @Test + void sharedLocalRouteBecomesUnavailableWhenWriterHasNoRoute() { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + routeSelector.initialize(true, false); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, + null, + () -> { + throw new IllegalStateException("discovery failed"); + }, + () -> null, + "flag evaluation", + routeSelector); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("missing"), stream -> null, null, false)); + assertEquals(FeatureFlagRouteSelector.Route.UNAVAILABLE, routeSelector.current()); + } + + @Test + void sharedDirectRouteWithoutWriterClientIsUnavailable() { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + routeSelector.initialize(false, true); + final AgentlessFeatureFlagBackendApi api = + new AgentlessFeatureFlagBackendApi( + null, null, () -> null, () -> null, "flag evaluation", routeSelector); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("missing"), stream -> null, null, false)); + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); + } + @Test void workingDirectRouteDoesNotAttemptRecovery() throws Exception { final AtomicLong clock = new AtomicLong(); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index a0c4f7d59ee..65a4057e6b1 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -439,6 +439,30 @@ void testCloseBeforeInitPreventsLaterStartAndAccept() { assertTrue(requests.isEmpty()); } + @Test + void testMissingBackendClosesWriterFromSerializerThread() throws Exception { + CountDownLatch backendRequested = new CountDownLatch(1); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + () -> { + backendRequested.countDown(); + return null; + }, + mockConfig("missing-backend-service"), + 100); + + writer.init(); + + assertTrue(backendRequested.await(5, java.util.concurrent.TimeUnit.SECONDS)); + poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); + writer.accept(buildExposure()); + assertEquals(0, writer.queueSize()); + writer.close(); + } + @Test void testHttpFailureIsNotRetriedAtTransportLayer() throws Exception { String serviceName = "fail-once"; diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java new file mode 100644 index 00000000000..d95cacdb94b --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java @@ -0,0 +1,33 @@ +package com.datadog.featureflag; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +import java.util.concurrent.atomic.AtomicLong; +import org.junit.jupiter.api.Test; + +class FeatureFlagRouteSelectorTest { + + @Test + void directRouteIsTerminalAcrossInitializationAndRecoverySignals() { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(false, true)); + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(true, false)); + assertFalse(routeSelector.tryBeginLocalRecovery()); + + routeSelector.localRecovered(); + + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); + } + + @Test + void unavailableRouteCanBeReinitializedWithLocal() { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(clock::get, 10); + + assertEquals( + FeatureFlagRouteSelector.Route.UNAVAILABLE, routeSelector.initialize(false, false)); + assertEquals(FeatureFlagRouteSelector.Route.LOCAL, routeSelector.initialize(true, false)); + } +} From 1b5766b56c67bb7151a084abb752e62e5bb4b332 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Sat, 19 Sep 2026 02:54:29 +0000 Subject: [PATCH 3/8] Separate EVP identity emission from fallback hardening Move identity attribution to a follow-up branch while keeping forwarding-capability requirements explicit and independent of emitted headers. Preserve credential isolation, no-replay coverage, and existing lifecycle hardening. Validation: communication tests 234/234 and feature-flagging-lib tests 279/279 pass; scoped Spotless formatting passes. Environment: Datadog workspace --- .../communication/BackendApiFactory.java | 54 ++------- .../java/datadog/communication/EvpProxy.java | 3 - .../communication/BackendApiFactoryTest.java | 109 ++++++------------ .../FeatureFlagBackendApiFactory.java | 18 +-- .../featureflag/ExposureWriterTests.java | 9 +- .../FeatureFlagBackendApiFactoryTest.java | 27 ++++- 6 files changed, 81 insertions(+), 139 deletions(-) diff --git a/communication/src/main/java/datadog/communication/BackendApiFactory.java b/communication/src/main/java/datadog/communication/BackendApiFactory.java index 08c2b08d897..b233094f048 100644 --- a/communication/src/main/java/datadog/communication/BackendApiFactory.java +++ b/communication/src/main/java/datadog/communication/BackendApiFactory.java @@ -1,7 +1,6 @@ package datadog.communication; -import static java.util.Collections.emptyMap; -import static java.util.Collections.unmodifiableMap; +import static java.util.Collections.emptyList; import datadog.communication.ddagent.DDAgentFeaturesDiscovery; import datadog.communication.ddagent.SharedCommunicationObjects; @@ -9,12 +8,9 @@ import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; import datadog.trace.util.throwable.FatalAgentMisconfigurationError; -import java.util.HashMap; -import java.util.Map; import javax.annotation.Nullable; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; -import okhttp3.Request; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -26,35 +22,23 @@ public class BackendApiFactory { private final Config config; private final SharedCommunicationObjects sharedCommunicationObjects; - private final Map requestHeaders; private final boolean sendOnce; public BackendApiFactory(Config config, SharedCommunicationObjects sharedCommunicationObjects) { - this(config, sharedCommunicationObjects, emptyMap()); - } - - public BackendApiFactory( - Config config, - SharedCommunicationObjects sharedCommunicationObjects, - Map requestHeaders) { - this(config, sharedCommunicationObjects, requestHeaders, false); + this(config, sharedCommunicationObjects, false); } /** - * Creates a backend factory with per-request headers and optional send-once transport semantics. + * Creates a backend factory with optional send-once transport semantics. * *

When {@code sendOnce} is true, both the explicit HTTP retry policy and OkHttp's automatic * connection retry are disabled. This is required for event payloads that do not carry an * idempotency key. */ public BackendApiFactory( - Config config, - SharedCommunicationObjects sharedCommunicationObjects, - Map requestHeaders, - boolean sendOnce) { + Config config, SharedCommunicationObjects sharedCommunicationObjects, boolean sendOnce) { this.config = config; this.sharedCommunicationObjects = sharedCommunicationObjects; - this.requestHeaders = unmodifiableMap(new HashMap<>(requestHeaders)); this.sendOnce = sendOnce; } @@ -190,12 +174,14 @@ private static boolean isAsciiLetterOrDigit(final char character) { /** Creates an API client that sends data through a compatible local EVP proxy. */ public @Nullable BackendApi createEvpProxyApi( Intake intake, boolean responseCompression, HttpRetryPolicy.Factory retryPolicyFactory) { - return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, false, false); + return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, false, emptyList()); } /** * Creates an EVP proxy client after Agent discovery, optionally forcing a fresh discovery and - * requiring the Agent to advertise every configured request header. + * requiring the Agent to advertise every specified forwarding header. + * + *

Callers supply the forwarding capabilities their event protocol requires. * *

The {@code forceDiscovery} form is intended for bounded unavailable-route recovery probes. */ @@ -204,7 +190,7 @@ private static boolean isAsciiLetterOrDigit(final char character) { boolean responseCompression, HttpRetryPolicy.Factory retryPolicyFactory, boolean forceDiscovery, - boolean requireConfiguredRequestHeaders) { + Iterable requiredProxyHeaders) { DDAgentFeaturesDiscovery featuresDiscovery = sharedCommunicationObjects.featuresDiscovery(config); if (forceDiscovery) { @@ -214,8 +200,7 @@ private static boolean isAsciiLetterOrDigit(final char character) { } String evpProxyEndpoint = featuresDiscovery.getEvpProxyEndpoint(); if (evpProxyEndpoint != null - && requireConfiguredRequestHeaders - && !featuresDiscovery.supportsEvpProxyHeaders(requestHeaders.keySet())) { + && !featuresDiscovery.supportsEvpProxyHeaders(requiredProxyHeaders)) { evpProxyEndpoint = null; } if (evpProxyEndpoint == null) { @@ -265,24 +250,7 @@ static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { } OkHttpClient configureHttpClient(final OkHttpClient httpClient) { - if (requestHeaders.isEmpty() && !sendOnce) { - return httpClient; - } - final OkHttpClient.Builder builder = httpClient.newBuilder(); - if (sendOnce) { - builder.retryOnConnectionFailure(false); - } - if (!requestHeaders.isEmpty()) { - builder.addInterceptor( - chain -> { - final Request.Builder requestBuilder = chain.request().newBuilder(); - for (Map.Entry header : requestHeaders.entrySet()) { - requestBuilder.header(header.getKey(), header.getValue()); - } - return chain.proceed(requestBuilder.build()); - }); - } - return builder.build(); + return sendOnce ? httpClient.newBuilder().retryOnConnectionFailure(false).build() : httpClient; } private HttpRetryPolicy.Factory retryPolicyFactory() { diff --git a/communication/src/main/java/datadog/communication/EvpProxy.java b/communication/src/main/java/datadog/communication/EvpProxy.java index 05ce9757339..301c48f0ff5 100644 --- a/communication/src/main/java/datadog/communication/EvpProxy.java +++ b/communication/src/main/java/datadog/communication/EvpProxy.java @@ -11,9 +11,6 @@ public final class EvpProxy { /** Identifies the version of the SDK that produced an EVP request. */ public static final String ORIGIN_VERSION_HEADER = "DD-EVP-ORIGIN-VERSION"; - /** Origin header value identifying this tracing library. */ - public static final String JAVA_TRACING_LIBRARY = "dd-trace-java"; - /** * Default SDK-side target for uncompressed EVP request bodies. Writers may split batches at or * below this size to keep Agent proxy requests comfortably bounded. diff --git a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java index 5ead5496322..f3479237c01 100644 --- a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java +++ b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java @@ -1,13 +1,10 @@ package datadog.communication; -import static datadog.communication.EvpProxy.JAVA_TRACING_LIBRARY; import static datadog.communication.EvpProxy.ORIGIN_HEADER; import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; -import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_AGENTLESS_URL; -import static datadog.trace.api.config.GeneralConfig.API_KEY; -import static java.util.Collections.singletonMap; +import static java.util.Arrays.asList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -25,10 +22,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.Arrays; -import java.util.HashMap; import java.util.Locale; -import java.util.Map; -import java.util.Properties; import java.util.stream.Stream; import okhttp3.HttpUrl; import okhttp3.MediaType; @@ -275,7 +269,7 @@ void discoveryPreservesAgentBasePathAndAcceptsCaseInsensitiveIdentityCapabilitie assertEquals("/agent/base/info", agent.takeRequest().getPath()); assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); - assertTrue(discovery.supportsEvpProxyHeaders(sdkHeaders().keySet())); + assertTrue(discovery.supportsEvpProxyHeaders(asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); } finally { agent.shutdown(); } @@ -314,14 +308,26 @@ void discoveryRejectsMissingOrPartialIdentityCapabilities(final String capabilit discovery.discover(); assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); - assertFalse(discovery.supportsEvpProxyHeaders(sdkHeaders().keySet())); + assertFalse(discovery.supportsEvpProxyHeaders(asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + + // The capability gate must not depend on whether the factory emits SDK identity headers. + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/")), true); + assertNull( + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); } finally { agent.shutdown(); } } @Test - void featureFlagProxyRequestCarriesSdkIdentityWithoutDirectCredentials() throws Exception { + void featureFlagProxyRequestNeverCarriesDirectCredentials() throws Exception { final MockWebServer agent = new MockWebServer(); agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); agent.start(); @@ -329,13 +335,14 @@ void featureFlagProxyRequestCarriesSdkIdentityWithoutDirectCredentials() throws final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT); final BackendApiFactory factory = new BackendApiFactory( - Config.get(), - sharedCommunicationObjects(discovery, agent.url("/")), - sdkHeaders(), - true); + Config.get(), sharedCommunicationObjects(discovery, agent.url("/")), true); final BackendApi api = factory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true); + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); assertNotNull(api); api.post( @@ -348,8 +355,6 @@ void featureFlagProxyRequestCarriesSdkIdentityWithoutDirectCredentials() throws final RecordedRequest request = agent.takeRequest(); assertEquals("/evp_proxy/v4/api/v2/exposures", request.getPath()); assertEquals("event-platform-intake", request.getHeader("X-Datadog-EVP-Subdomain")); - assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); - assertEquals("test-version", request.getHeader(ORIGIN_VERSION_HEADER)); assertNull(request.getHeader("DD-API-KEY")); assertEquals(1, agent.getRequestCount()); } finally { @@ -358,44 +363,7 @@ void featureFlagProxyRequestCarriesSdkIdentityWithoutDirectCredentials() throws } @Test - void directIntakeSendsConfiguredRequestHeaders() throws Exception { - final MockWebServer intake = new MockWebServer(); - intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); - intake.start(); - try { - final Properties properties = new Properties(); - properties.setProperty(API_KEY, "api-key"); - properties.setProperty( - CIVISIBILITY_AGENTLESS_URL, intake.url("/").toString().replaceAll("/$", "")); - final Config config = Config.get(properties); - final BackendApiFactory factory = - new BackendApiFactory( - config, - sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), - singletonMap(ORIGIN_HEADER, JAVA_TRACING_LIBRARY)); - - // followRedirects=false mirrors the feature-flagging caller, so this also covers the - // interaction between the redirect-scoped client and the header interceptor. - final BackendApi api = factory.createDirectIntakeApi(Intake.API, false, false); - - assertNotNull(api); - api.post( - "flagevaluation", - RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), - stream -> null, - null, - false); - - final RecordedRequest request = intake.takeRequest(); - assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); - assertEquals("api-key", request.getHeader("DD-API-KEY")); - } finally { - intake.shutdown(); - } - } - - @Test - void featureFlagDirectRequestCarriesCredentialsAndIdentityExactlyOnce() throws Exception { + void featureFlagDirectRequestCarriesCredentialsExactlyOnce() throws Exception { final MockWebServer intake = new MockWebServer(); intake.enqueue(new MockResponse().setResponseCode(500).setBody("failed")); intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); @@ -403,10 +371,7 @@ void featureFlagDirectRequestCarriesCredentialsAndIdentityExactlyOnce() throws E final OkHttpClient sharedClient = new OkHttpClient.Builder().build(); final BackendApiFactory factory = new BackendApiFactory( - Config.get(), - sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), - sdkHeaders(), - true); + Config.get(), sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), true); final OkHttpClient directClient = factory.configureHttpClient(sharedClient); try { final IntakeApi api = @@ -431,8 +396,6 @@ void featureFlagDirectRequestCarriesCredentialsAndIdentityExactlyOnce() throws E final RecordedRequest request = intake.takeRequest(); assertEquals("/api/v2/flagevaluation", request.getPath()); assertEquals("api-key", request.getHeader("DD-API-KEY")); - assertEquals("dd-trace-java", request.getHeader("DD-EVP-ORIGIN")); - assertEquals("test-version", request.getHeader("DD-EVP-ORIGIN-VERSION")); assertNull(request.getHeader("X-Datadog-EVP-Subdomain")); assertEquals(1, intake.getRequestCount()); assertEquals(false, directClient.retryOnConnectionFailure()); @@ -489,15 +452,18 @@ void fixedV2CompatibilityRouteDoesNotDependOnDiscovery() throws Exception { } @Test - void capabilityGatedProxyRequiresConfiguredRequestHeaders() { + void capabilityGatedProxyRequiresHeadersWithoutRequestHeaderConfiguration() { final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT, false); final BackendApiFactory factory = - new BackendApiFactory( - Config.get(), sharedCommunicationObjects(discovery, null), sdkHeaders(), true); + new BackendApiFactory(Config.get(), sharedCommunicationObjects(discovery, null), true); assertNull( factory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)); + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); } @Test @@ -508,7 +474,11 @@ void recoveryRequestForcesFreshDiscovery() { assertNull( factory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, true, true)); + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + true, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); assertEquals(1, discovery.forcedDiscoveries); assertEquals(0, discovery.outdatedDiscoveries); } @@ -553,13 +523,6 @@ private static SharedCommunicationObjects sharedCommunicationObjects( return sco; } - private static Map sdkHeaders() { - final Map headers = new HashMap<>(2); - headers.put("DD-EVP-ORIGIN", "dd-trace-java"); - headers.put("DD-EVP-ORIGIN-VERSION", "test-version"); - return headers; - } - private static final class TestSharedCommunicationObjects extends SharedCommunicationObjects { private final DDAgentFeaturesDiscovery discovery; diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java index 7c41efcc551..f5b75e2f815 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java @@ -1,21 +1,17 @@ package com.datadog.featureflag; -import static datadog.communication.EvpProxy.JAVA_TRACING_LIBRARY; import static datadog.communication.EvpProxy.ORIGIN_HEADER; import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; -import static java.util.Collections.unmodifiableMap; +import static java.util.Arrays.asList; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; import datadog.communication.ddagent.SharedCommunicationObjects; -import datadog.communication.ddagent.TracerVersion; import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; -import java.util.HashMap; -import java.util.Map; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -24,7 +20,6 @@ final class FeatureFlagBackendApiFactory { private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagBackendApiFactory.class); - private static final Map REQUEST_HEADERS = requestHeaders(); private final Config config; private final BackendApiFactory backendApiFactory; @@ -45,7 +40,7 @@ final class FeatureFlagBackendApiFactory { final FeatureFlagRouteSelector routeSelector) { this( config, - new BackendApiFactory(config, sharedCommunicationObjects, REQUEST_HEADERS, true), + new BackendApiFactory(config, sharedCommunicationObjects, true), eventType, routeSelector); } @@ -105,14 +100,7 @@ private BackendApi createProxyApi(final boolean forceDiscovery) { eventType.responseCompressionEnabled(), HttpRetryPolicy.Factory.NEVER_RETRY, forceDiscovery, - true); - } - - private static Map requestHeaders() { - final Map headers = new HashMap<>(2); - headers.put(ORIGIN_HEADER, JAVA_TRACING_LIBRARY); - headers.put(ORIGIN_VERSION_HEADER, TracerVersion.TRACER_VERSION); - return unmodifiableMap(headers); + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); } private boolean hasDirectCredentials() { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index 65a4057e6b1..8c243ae5a95 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -1,6 +1,9 @@ package com.datadog.featureflag; +import static datadog.communication.EvpProxy.ORIGIN_HEADER; +import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; +import static java.util.Arrays.asList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -507,7 +510,11 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception final BackendApi proxyApi = mock(BackendApi.class); final BackendApi directApi = mock(BackendApi.class); when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, true, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)) + Intake.EVENT_PLATFORM, + true, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) .thenReturn(proxyApi); when(backendApiFactory.createDirectIntakeApi(eq(Intake.EVENT_PLATFORM), eq(true), eq(false))) .thenReturn(directApi); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java index 58811e0bfb6..f77d064b9c0 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java @@ -2,9 +2,12 @@ import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; import static com.datadog.featureflag.FeatureFlagEventType.FLAG_EVALUATION; +import static datadog.communication.EvpProxy.ORIGIN_HEADER; +import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_REMOTE_CONFIG; +import static java.util.Arrays.asList; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertSame; import static org.mockito.Mockito.mock; @@ -39,7 +42,11 @@ void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { assertSame(proxyApi, selected); verify(backendApiFactory, never()) .createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true); + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @@ -48,7 +55,11 @@ void agentlessPrefersCapabilityGatedLocalRouteWithDirectFallbackReady() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)) + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) .thenReturn(mock(BackendApi.class)); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenReturn(mock(BackendApi.class)); @@ -59,7 +70,11 @@ void agentlessPrefersCapabilityGatedLocalRouteWithDirectFallbackReady() { assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); verify(backendApiFactory) .createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true); + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); verify(backendApiFactory).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @@ -94,7 +109,11 @@ void agentlessKeepsCompatibleLocalRouteWhenDirectUrlIsInvalid() { final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); final BackendApi proxyApi = mock(BackendApi.class); when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY, false, true)) + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) .thenReturn(proxyApi); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenThrow(new IllegalArgumentException("invalid URL")); From 96a3427bb9bcb640165306d16af141e9455e2cc4 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Sat, 19 Sep 2026 02:56:42 +0000 Subject: [PATCH 4/8] Identify Java feature flag events on local and direct EVP routes Add immutable per-request identity headers independently of forwarding capability checks and send-once policy. Emit dd-trace-java and the tracer version for both exposure and flag-evaluation requests. Validation: communication tests 235/235 and feature-flagging-lib tests 279/279 pass; scoped Spotless formatting passes. Local/direct wire assertions preserve credential isolation and one-send behavior. Environment: Datadog workspace --- .../communication/BackendApiFactory.java | 49 +++++++++++-- .../java/datadog/communication/EvpProxy.java | 3 + .../communication/BackendApiFactoryTest.java | 69 +++++++++++++++++-- .../FeatureFlagBackendApiFactory.java | 15 +++- 4 files changed, 126 insertions(+), 10 deletions(-) diff --git a/communication/src/main/java/datadog/communication/BackendApiFactory.java b/communication/src/main/java/datadog/communication/BackendApiFactory.java index b233094f048..31020eca2ea 100644 --- a/communication/src/main/java/datadog/communication/BackendApiFactory.java +++ b/communication/src/main/java/datadog/communication/BackendApiFactory.java @@ -1,6 +1,8 @@ package datadog.communication; import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static java.util.Collections.unmodifiableMap; import datadog.communication.ddagent.DDAgentFeaturesDiscovery; import datadog.communication.ddagent.SharedCommunicationObjects; @@ -8,9 +10,12 @@ import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; import datadog.trace.util.throwable.FatalAgentMisconfigurationError; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; +import okhttp3.Request; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -22,23 +27,40 @@ public class BackendApiFactory { private final Config config; private final SharedCommunicationObjects sharedCommunicationObjects; + private final Map requestHeaders; private final boolean sendOnce; public BackendApiFactory(Config config, SharedCommunicationObjects sharedCommunicationObjects) { - this(config, sharedCommunicationObjects, false); + this(config, sharedCommunicationObjects, emptyMap()); + } + + public BackendApiFactory( + Config config, + SharedCommunicationObjects sharedCommunicationObjects, + Map requestHeaders) { + this(config, sharedCommunicationObjects, requestHeaders, false); + } + + public BackendApiFactory( + Config config, SharedCommunicationObjects sharedCommunicationObjects, boolean sendOnce) { + this(config, sharedCommunicationObjects, emptyMap(), sendOnce); } /** - * Creates a backend factory with optional send-once transport semantics. + * Creates a backend factory with per-request headers and optional send-once transport semantics. * *

When {@code sendOnce} is true, both the explicit HTTP retry policy and OkHttp's automatic * connection retry are disabled. This is required for event payloads that do not carry an * idempotency key. */ public BackendApiFactory( - Config config, SharedCommunicationObjects sharedCommunicationObjects, boolean sendOnce) { + Config config, + SharedCommunicationObjects sharedCommunicationObjects, + Map requestHeaders, + boolean sendOnce) { this.config = config; this.sharedCommunicationObjects = sharedCommunicationObjects; + this.requestHeaders = unmodifiableMap(new HashMap<>(requestHeaders)); this.sendOnce = sendOnce; } @@ -181,7 +203,7 @@ private static boolean isAsciiLetterOrDigit(final char character) { * Creates an EVP proxy client after Agent discovery, optionally forcing a fresh discovery and * requiring the Agent to advertise every specified forwarding header. * - *

Callers supply the forwarding capabilities their event protocol requires. + *

Required forwarding capabilities are independent of any headers configured on this factory. * *

The {@code forceDiscovery} form is intended for bounded unavailable-route recovery probes. */ @@ -250,7 +272,24 @@ static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { } OkHttpClient configureHttpClient(final OkHttpClient httpClient) { - return sendOnce ? httpClient.newBuilder().retryOnConnectionFailure(false).build() : httpClient; + if (requestHeaders.isEmpty() && !sendOnce) { + return httpClient; + } + final OkHttpClient.Builder builder = httpClient.newBuilder(); + if (sendOnce) { + builder.retryOnConnectionFailure(false); + } + if (!requestHeaders.isEmpty()) { + builder.addInterceptor( + chain -> { + final Request.Builder requestBuilder = chain.request().newBuilder(); + for (Map.Entry header : requestHeaders.entrySet()) { + requestBuilder.header(header.getKey(), header.getValue()); + } + return chain.proceed(requestBuilder.build()); + }); + } + return builder.build(); } private HttpRetryPolicy.Factory retryPolicyFactory() { diff --git a/communication/src/main/java/datadog/communication/EvpProxy.java b/communication/src/main/java/datadog/communication/EvpProxy.java index 301c48f0ff5..05ce9757339 100644 --- a/communication/src/main/java/datadog/communication/EvpProxy.java +++ b/communication/src/main/java/datadog/communication/EvpProxy.java @@ -11,6 +11,9 @@ public final class EvpProxy { /** Identifies the version of the SDK that produced an EVP request. */ public static final String ORIGIN_VERSION_HEADER = "DD-EVP-ORIGIN-VERSION"; + /** Origin header value identifying this tracing library. */ + public static final String JAVA_TRACING_LIBRARY = "dd-trace-java"; + /** * Default SDK-side target for uncompressed EVP request bodies. Writers may split batches at or * below this size to keep Agent proxy requests comfortably bounded. diff --git a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java index f3479237c01..6974d413fa6 100644 --- a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java +++ b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java @@ -1,10 +1,14 @@ package datadog.communication; +import static datadog.communication.EvpProxy.JAVA_TRACING_LIBRARY; import static datadog.communication.EvpProxy.ORIGIN_HEADER; import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_AGENTLESS_URL; +import static datadog.trace.api.config.GeneralConfig.API_KEY; import static java.util.Arrays.asList; +import static java.util.Collections.singletonMap; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -22,7 +26,10 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.HashMap; import java.util.Locale; +import java.util.Map; +import java.util.Properties; import java.util.stream.Stream; import okhttp3.HttpUrl; import okhttp3.MediaType; @@ -327,7 +334,7 @@ void discoveryRejectsMissingOrPartialIdentityCapabilities(final String capabilit } @Test - void featureFlagProxyRequestNeverCarriesDirectCredentials() throws Exception { + void featureFlagProxyRequestCarriesSdkIdentityWithoutDirectCredentials() throws Exception { final MockWebServer agent = new MockWebServer(); agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); agent.start(); @@ -335,7 +342,10 @@ void featureFlagProxyRequestNeverCarriesDirectCredentials() throws Exception { final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT); final BackendApiFactory factory = new BackendApiFactory( - Config.get(), sharedCommunicationObjects(discovery, agent.url("/")), true); + Config.get(), + sharedCommunicationObjects(discovery, agent.url("/")), + sdkHeaders(), + true); final BackendApi api = factory.createEvpProxyApi( Intake.EVENT_PLATFORM, @@ -355,6 +365,8 @@ void featureFlagProxyRequestNeverCarriesDirectCredentials() throws Exception { final RecordedRequest request = agent.takeRequest(); assertEquals("/evp_proxy/v4/api/v2/exposures", request.getPath()); assertEquals("event-platform-intake", request.getHeader("X-Datadog-EVP-Subdomain")); + assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); + assertEquals("test-version", request.getHeader(ORIGIN_VERSION_HEADER)); assertNull(request.getHeader("DD-API-KEY")); assertEquals(1, agent.getRequestCount()); } finally { @@ -363,7 +375,44 @@ void featureFlagProxyRequestNeverCarriesDirectCredentials() throws Exception { } @Test - void featureFlagDirectRequestCarriesCredentialsExactlyOnce() throws Exception { + void directIntakeSendsConfiguredRequestHeaders() throws Exception { + final MockWebServer intake = new MockWebServer(); + intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + intake.start(); + try { + final Properties properties = new Properties(); + properties.setProperty(API_KEY, "api-key"); + properties.setProperty( + CIVISIBILITY_AGENTLESS_URL, intake.url("/").toString().replaceAll("/$", "")); + final Config config = Config.get(properties); + final BackendApiFactory factory = + new BackendApiFactory( + config, + sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), + singletonMap(ORIGIN_HEADER, JAVA_TRACING_LIBRARY)); + + // followRedirects=false mirrors the feature-flagging caller, so this also covers the + // interaction between the redirect-scoped client and the header interceptor. + final BackendApi api = factory.createDirectIntakeApi(Intake.API, false, false); + + assertNotNull(api); + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + + final RecordedRequest request = intake.takeRequest(); + assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); + assertEquals("api-key", request.getHeader("DD-API-KEY")); + } finally { + intake.shutdown(); + } + } + + @Test + void featureFlagDirectRequestCarriesCredentialsAndIdentityExactlyOnce() throws Exception { final MockWebServer intake = new MockWebServer(); intake.enqueue(new MockResponse().setResponseCode(500).setBody("failed")); intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); @@ -371,7 +420,10 @@ void featureFlagDirectRequestCarriesCredentialsExactlyOnce() throws Exception { final OkHttpClient sharedClient = new OkHttpClient.Builder().build(); final BackendApiFactory factory = new BackendApiFactory( - Config.get(), sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), true); + Config.get(), + sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), + sdkHeaders(), + true); final OkHttpClient directClient = factory.configureHttpClient(sharedClient); try { final IntakeApi api = @@ -396,6 +448,8 @@ void featureFlagDirectRequestCarriesCredentialsExactlyOnce() throws Exception { final RecordedRequest request = intake.takeRequest(); assertEquals("/api/v2/flagevaluation", request.getPath()); assertEquals("api-key", request.getHeader("DD-API-KEY")); + assertEquals("dd-trace-java", request.getHeader("DD-EVP-ORIGIN")); + assertEquals("test-version", request.getHeader("DD-EVP-ORIGIN-VERSION")); assertNull(request.getHeader("X-Datadog-EVP-Subdomain")); assertEquals(1, intake.getRequestCount()); assertEquals(false, directClient.retryOnConnectionFailure()); @@ -523,6 +577,13 @@ private static SharedCommunicationObjects sharedCommunicationObjects( return sco; } + private static Map sdkHeaders() { + final Map headers = new HashMap<>(2); + headers.put("DD-EVP-ORIGIN", "dd-trace-java"); + headers.put("DD-EVP-ORIGIN-VERSION", "test-version"); + return headers; + } + private static final class TestSharedCommunicationObjects extends SharedCommunicationObjects { private final DDAgentFeaturesDiscovery discovery; diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java index f5b75e2f815..0570fe331be 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java @@ -1,17 +1,22 @@ package com.datadog.featureflag; +import static datadog.communication.EvpProxy.JAVA_TRACING_LIBRARY; import static datadog.communication.EvpProxy.ORIGIN_HEADER; import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static java.util.Arrays.asList; +import static java.util.Collections.unmodifiableMap; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.communication.ddagent.TracerVersion; import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -20,6 +25,7 @@ final class FeatureFlagBackendApiFactory { private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagBackendApiFactory.class); + private static final Map REQUEST_HEADERS = requestHeaders(); private final Config config; private final BackendApiFactory backendApiFactory; @@ -40,7 +46,7 @@ final class FeatureFlagBackendApiFactory { final FeatureFlagRouteSelector routeSelector) { this( config, - new BackendApiFactory(config, sharedCommunicationObjects, true), + new BackendApiFactory(config, sharedCommunicationObjects, REQUEST_HEADERS, true), eventType, routeSelector); } @@ -103,6 +109,13 @@ private BackendApi createProxyApi(final boolean forceDiscovery) { asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); } + private static Map requestHeaders() { + final Map headers = new HashMap<>(2); + headers.put(ORIGIN_HEADER, JAVA_TRACING_LIBRARY); + headers.put(ORIGIN_VERSION_HEADER, TracerVersion.TRACER_VERSION); + return unmodifiableMap(headers); + } + private boolean hasDirectCredentials() { final String apiKey = config.getApiKey(); return apiKey != null && !apiKey.isEmpty(); From 4218559309b03bcca3ffda8a73f087187ce20460 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Wed, 7 Oct 2026 02:11:11 +0000 Subject: [PATCH 5/8] Share recovered EVP endpoints across feature flag writers Gate shutdown instrumentation on tracing or resolved feature flag enablement, and share Agent path joining. Pin recovery generations, discovery concurrency, replay, idle recovery, and disabled-product behavior with regression tests. Environment: Datadog workspace --- .../communication/BackendApiFactory.java | 28 +-- .../ddagent/DDAgentFeaturesDiscovery.java | 9 +- .../communication/http/OkHttpUtils.java | 9 + .../communication/http/AgentUrlPathTest.java | 20 ++ .../java/lang/ShutdownInstrumentation.java | 8 +- .../lang/ShutdownInstrumentationTest.java | 38 ++++ .../AgentlessFeatureFlagBackendApi.java | 110 +++------- .../FeatureFlagBackendApiFactory.java | 29 ++- .../featureflag/FeatureFlagRouteSelector.java | 48 +++- .../AgentlessFeatureFlagBackendApiTest.java | 134 ++++++------ .../featureflag/ExposureWriterTests.java | 53 ++++- .../FeatureFlagBackendApiFactoryTest.java | 35 +-- .../FeatureFlagRouteSelectorTest.java | 45 +++- .../SharedFeatureFlagRouteTest.java | 207 ++++++++++++++++++ 14 files changed, 556 insertions(+), 217 deletions(-) create mode 100644 communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java create mode 100644 dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java create mode 100644 products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java diff --git a/communication/src/main/java/datadog/communication/BackendApiFactory.java b/communication/src/main/java/datadog/communication/BackendApiFactory.java index b233094f048..0514b39f33b 100644 --- a/communication/src/main/java/datadog/communication/BackendApiFactory.java +++ b/communication/src/main/java/datadog/communication/BackendApiFactory.java @@ -1,5 +1,6 @@ package datadog.communication; +import static datadog.communication.http.OkHttpUtils.appendPath; import static java.util.Collections.emptyList; import datadog.communication.ddagent.DDAgentFeaturesDiscovery; @@ -191,6 +192,17 @@ private static boolean isAsciiLetterOrDigit(final char character) { HttpRetryPolicy.Factory retryPolicyFactory, boolean forceDiscovery, Iterable requiredProxyHeaders) { + String endpoint = discoverEvpProxyEndpoint(forceDiscovery, requiredProxyHeaders); + return endpoint == null + ? null + : createEvpProxyApi(intake, responseCompression, retryPolicyFactory, endpoint); + } + + /** + * Discovers a capability-validated endpoint that related senders can share without re-probing. + */ + public @Nullable String discoverEvpProxyEndpoint( + boolean forceDiscovery, Iterable requiredProxyHeaders) { DDAgentFeaturesDiscovery featuresDiscovery = sharedCommunicationObjects.featuresDiscovery(config); if (forceDiscovery) { @@ -203,14 +215,10 @@ private static boolean isAsciiLetterOrDigit(final char character) { && !featuresDiscovery.supportsEvpProxyHeaders(requiredProxyHeaders)) { evpProxyEndpoint = null; } - if (evpProxyEndpoint == null) { - return null; - } - - return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, evpProxyEndpoint); + return evpProxyEndpoint; } - /** Creates an EVP proxy client for a fixed compatibility endpoint without Agent discovery. */ + /** Creates a client for a fixed compatibility or previously discovered endpoint, without I/O. */ public BackendApi createEvpProxyApiForEndpoint( Intake intake, boolean responseCompression, @@ -241,14 +249,6 @@ private BackendApi createEvpProxyApi( responseCompression); } - static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { - int firstCharacter = 0; - while (firstCharacter < path.length() && path.charAt(firstCharacter) == '/') { - firstCharacter++; - } - return baseUrl.newBuilder().addPathSegments(path.substring(firstCharacter)).build(); - } - OkHttpClient configureHttpClient(final OkHttpClient httpClient) { return sendOnce ? httpClient.newBuilder().retryOnConnectionFailure(false).build() : httpClient; } diff --git a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java index c7fc60f1d1f..5986bd44331 100644 --- a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java +++ b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java @@ -1,6 +1,7 @@ package datadog.communication.ddagent; import static datadog.communication.http.OkHttpUtils.DATADOG_CONTAINER_TAGS_HASH; +import static datadog.communication.http.OkHttpUtils.appendPath; import static datadog.communication.http.OkHttpUtils.msgpackRequestBodyOf; import static datadog.communication.http.OkHttpUtils.prepareRequest; import static datadog.communication.serialization.msgpack.MsgPackWriter.FIXARRAY; @@ -492,12 +493,4 @@ public boolean active() { public boolean supportsTelemetryProxy() { return discoveryState.telemetryProxyEndpoint != null; } - - private static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { - int firstCharacter = 0; - while (firstCharacter < path.length() && path.charAt(firstCharacter) == '/') { - firstCharacter++; - } - return baseUrl.newBuilder().addPathSegments(path.substring(firstCharacter)).build(); - } } diff --git a/communication/src/main/java/datadog/communication/http/OkHttpUtils.java b/communication/src/main/java/datadog/communication/http/OkHttpUtils.java index 44105ae772d..7ecffb29d8e 100644 --- a/communication/src/main/java/datadog/communication/http/OkHttpUtils.java +++ b/communication/src/main/java/datadog/communication/http/OkHttpUtils.java @@ -40,6 +40,15 @@ import org.slf4j.LoggerFactory; public final class OkHttpUtils { + /** Appends a relative endpoint while preserving the configured Agent URL's path prefix. */ + public static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { + int firstCharacter = 0; + while (firstCharacter < path.length() && path.charAt(firstCharacter) == '/') { + firstCharacter++; + } + return baseUrl.newBuilder().addPathSegments(path.substring(firstCharacter)).build(); + } + private static final Logger log = LoggerFactory.getLogger(OkHttpUtils.class); private static final String DATADOG_META_LANG = "Datadog-Meta-Lang"; diff --git a/communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java b/communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java new file mode 100644 index 00000000000..a65bf049349 --- /dev/null +++ b/communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java @@ -0,0 +1,20 @@ +package datadog.communication.http; + +import static datadog.communication.http.OkHttpUtils.appendPath; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import okhttp3.HttpUrl; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class AgentUrlPathTest { + @ParameterizedTest + @ValueSource(strings = {"/agent-prefix", "/agent-prefix/", "/agent%2Fprefix/"}) + void preservesPrefixForDiscoveryAndDelivery(String prefix) { + final HttpUrl base = HttpUrl.get("http://localhost:8126" + prefix); + final String normalized = prefix.endsWith("/") ? prefix : prefix + "/"; + assertEquals(normalized + "info", appendPath(base, "info").encodedPath()); + assertEquals(normalized + "evp_proxy/v4/", appendPath(base, "/evp_proxy/v4/").encodedPath()); + assertEquals(normalized + "evp_proxy/v2/", appendPath(base, "evp_proxy/v2/").encodedPath()); + } +} diff --git a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java index d51def6fa3d..5c7685594de 100644 --- a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java +++ b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java @@ -7,6 +7,7 @@ import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.Config; import datadog.trace.bootstrap.instrumentation.shutdown.ShutdownHelper; import java.util.Set; import net.bytebuddy.asm.Advice; @@ -30,9 +31,10 @@ public String instrumentedType() { @Override public boolean isApplicable(Set enabledSystems) { - // Agent-owned subsystems such as Feature Flagging can run while tracing is disabled. Their - // bounded final drains still depend on ShutdownHelper running before application hooks. - return true; + // Preserve tracing's historical hook and include Feature Flagging's bounded final drains even + // when tracing is disabled. Do not instrument shutdown when both consumers are disabled. + return enabledSystems.contains(TargetSystem.TRACING) + || Config.get().isFeatureFlaggingProviderEnabled(); } @Override diff --git a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java new file mode 100644 index 00000000000..b7477805cd5 --- /dev/null +++ b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java @@ -0,0 +1,38 @@ +package datadog.trace.instrumentation.java.lang; + +import static datadog.trace.agent.tooling.InstrumenterModule.TargetSystem.TRACING; +import static java.util.Collections.emptySet; +import static java.util.Collections.singleton; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.test.junit.utils.config.WithConfig; +import datadog.trace.test.util.DDJavaSpecification; +import org.junit.jupiter.api.Test; + +class ShutdownInstrumentationTest extends DDJavaSpecification { + @Test + @WithConfig(key = "feature.flags.enabled", value = "false") + void doesNotInstrumentShutdownWhenTracingAndFeatureFlagsAreDisabled() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "false") + void preservesTracingShutdown() { + assertTrue(new ShutdownInstrumentation().isApplicable(singleton(TRACING))); + } + + @Test + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + void enablesFeatureFlagShutdownWithoutTracing() { + assertTrue(new ShutdownInstrumentation().isApplicable(emptySet())); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "false") + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + void honorsExplicitDisableEvenWithAConfigurationSource() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java index bfb5d42b759..36324e828bf 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java @@ -7,7 +7,7 @@ import java.io.IOException; import java.io.InputStream; import java.net.ConnectException; -import java.util.function.LongSupplier; +import java.util.function.Function; import java.util.function.Supplier; import javax.annotation.Nullable; import okhttp3.RequestBody; @@ -21,60 +21,29 @@ final class AgentlessFeatureFlagBackendApi implements BackendApi { LoggerFactory.getLogger(AgentlessFeatureFlagBackendApi.class); private final FeatureFlagRouteSelector routeSelector; - private final Supplier proxyApiSupplier; + private final Supplier proxyEndpointSupplier; + private final Function proxyApiFactory; private final Supplier directApiSupplier; private final String eventType; - private volatile BackendApi proxyApi; + private BackendApi proxyApi; + private FeatureFlagRouteSelector.LocalRoute proxyRoute; private volatile BackendApi directApi; private volatile boolean directApiCreationAttempted; AgentlessFeatureFlagBackendApi( - @Nullable final BackendApi proxyApi, @Nullable final BackendApi directApi, - final Supplier proxyApiSupplier, - final Supplier directApiSupplier, - final String eventType) { - this( - proxyApi, - directApi, - proxyApiSupplier, - directApiSupplier, - eventType, - new FeatureFlagRouteSelector()); - } - - AgentlessFeatureFlagBackendApi( - @Nullable final BackendApi proxyApi, - @Nullable final BackendApi directApi, - final Supplier proxyApiSupplier, - final Supplier directApiSupplier, - final String eventType, - final LongSupplier nanoTime, - final long recoveryIntervalNanos) { - this( - proxyApi, - directApi, - proxyApiSupplier, - directApiSupplier, - eventType, - new FeatureFlagRouteSelector(nanoTime, recoveryIntervalNanos)); - } - - AgentlessFeatureFlagBackendApi( - @Nullable final BackendApi proxyApi, - @Nullable final BackendApi directApi, - final Supplier proxyApiSupplier, + final Supplier proxyEndpointSupplier, + final Function proxyApiFactory, final Supplier directApiSupplier, final String eventType, final FeatureFlagRouteSelector routeSelector) { - this.proxyApi = proxyApi; this.directApi = directApi; - this.proxyApiSupplier = proxyApiSupplier; + this.proxyEndpointSupplier = proxyEndpointSupplier; + this.proxyApiFactory = proxyApiFactory; this.directApiSupplier = directApiSupplier; this.eventType = eventType; this.routeSelector = routeSelector; this.directApiCreationAttempted = directApi != null; - routeSelector.initialize(proxyApi != null, directApi != null); } @Override @@ -90,12 +59,14 @@ public T post( return selected.api.post( uri, requestBody, responseParser, requestListener, requestCompression); } catch (final IOException exception) { - if (!selected.local) { + if (selected.localRoute == null) { throw exception; } final BackendApi fallbackApi = getOrCreateDirectApi(); - routeSelector.localFailure(fallbackApi != null); + // Route selection governs future batches; replay permission applies only to this batch. + // DIRECT is intentionally terminal, even for a transient or ambiguous local failure. + routeSelector.localFailure(selected.localRoute, fallbackApi != null); if (fallbackApi == null || !isSafeToReplayDirectly(exception)) { throw exception; } @@ -105,54 +76,44 @@ public T post( } private SelectedApi selectApi() throws IOException { - FeatureFlagRouteSelector.Route selectedRoute = routeSelector.current(); - if (selectedRoute == FeatureFlagRouteSelector.Route.UNAVAILABLE - && routeSelector.tryBeginLocalRecovery()) { - final BackendApi recoveredProxyApi = discoverProxyApi(); - if (recoveredProxyApi != null) { - proxyApi = recoveredProxyApi; - routeSelector.localRecovered(); - } - selectedRoute = routeSelector.current(); + if (routeSelector.tryBeginLocalRecovery()) { + routeSelector.localRecoveryFinished(discoverProxyEndpoint()); } - if (selectedRoute == FeatureFlagRouteSelector.Route.LOCAL) { - BackendApi selectedProxyApi = proxyApi; - if (selectedProxyApi == null) { - selectedProxyApi = discoverProxyApi(); - if (selectedProxyApi != null) { - proxyApi = selectedProxyApi; - } else { - final BackendApi selectedDirectApi = getOrCreateDirectApi(); - routeSelector.localFailure(selectedDirectApi != null); - if (selectedDirectApi != null) { - return new SelectedApi(selectedDirectApi, false); - } - throw unavailableRoute(); - } - } - return new SelectedApi(selectedProxyApi, true); + final FeatureFlagRouteSelector.LocalRoute localRoute = routeSelector.localRoute(); + if (localRoute != null) { + return new SelectedApi(getOrCreateProxyApi(localRoute), localRoute); } - if (selectedRoute == FeatureFlagRouteSelector.Route.DIRECT) { + if (routeSelector.current() == FeatureFlagRouteSelector.Route.DIRECT) { final BackendApi selectedDirectApi = getOrCreateDirectApi(); if (selectedDirectApi != null) { - return new SelectedApi(selectedDirectApi, false); + return new SelectedApi(selectedDirectApi, null); } } throw unavailableRoute(); } @Nullable - private BackendApi discoverProxyApi() { + private String discoverProxyEndpoint() { try { - return proxyApiSupplier.get(); + return proxyEndpointSupplier.get(); } catch (final RuntimeException exception) { LOGGER.debug("Could not discover the local Feature Flagging {} route", eventType, exception); return null; } } + private synchronized BackendApi getOrCreateProxyApi( + final FeatureFlagRouteSelector.LocalRoute localRoute) { + if (proxyRoute != localRoute) { + // Client construction uses the shared validated endpoint; it performs no discovery. + proxyApi = proxyApiFactory.apply(localRoute.endpoint); + proxyRoute = localRoute; + } + return proxyApi; + } + @Nullable private BackendApi getOrCreateDirectApi() { if (directApiCreationAttempted) { @@ -184,11 +145,12 @@ private static boolean isSafeToReplayDirectly(final IOException exception) { private static final class SelectedApi { private final BackendApi api; - private final boolean local; + private final FeatureFlagRouteSelector.LocalRoute localRoute; - private SelectedApi(final BackendApi api, final boolean local) { + private SelectedApi( + final BackendApi api, @Nullable final FeatureFlagRouteSelector.LocalRoute localRoute) { this.api = api; - this.local = local; + this.localRoute = localRoute; } } } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java index f5b75e2f815..e3f6ea40e36 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java @@ -77,30 +77,37 @@ BackendApi create() { V2_EVP_PROXY_ENDPOINT); } - final BackendApi proxyApi = createProxyApi(false); final BackendApi directApi = createDirectApi(); - if (proxyApi == null && directApi == null) { + final FeatureFlagRouteSelector.Route route = + routeSelector.initialize(() -> discoverProxyEndpoint(false), directApi != null); + if (route == FeatureFlagRouteSelector.Route.UNAVAILABLE) { LOGGER.warn( "Feature Flagging {} delivery is waiting for a compatible local EVP proxy because direct intake credentials are unavailable", eventType.logName()); } return new AgentlessFeatureFlagBackendApi( - proxyApi, directApi, - () -> createProxyApi(true), + () -> discoverProxyEndpoint(true), + endpoint -> + backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + eventType.responseCompressionEnabled(), + HttpRetryPolicy.Factory.NEVER_RETRY, + endpoint), this::createDirectApi, eventType.logName(), routeSelector); } @Nullable - private BackendApi createProxyApi(final boolean forceDiscovery) { - return backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, - eventType.responseCompressionEnabled(), - HttpRetryPolicy.Factory.NEVER_RETRY, - forceDiscovery, - asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + private String discoverProxyEndpoint(final boolean forceDiscovery) { + try { + return backendApiFactory.discoverEvpProxyEndpoint( + forceDiscovery, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + } catch (final RuntimeException exception) { + LOGGER.debug("Could not discover the local Feature Flagging route", exception); + return null; + } } private boolean hasDirectCredentials() { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java index da3a2da2125..a4bb8497d85 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java @@ -2,6 +2,8 @@ import java.util.concurrent.TimeUnit; import java.util.function.LongSupplier; +import java.util.function.Supplier; +import javax.annotation.Nullable; /** Process-wide route state shared by all Feature Flagging event writers. */ final class FeatureFlagRouteSelector { @@ -19,6 +21,17 @@ enum Route { private final long recoveryIntervalNanos; private volatile Route route = Route.UNINITIALIZED; private volatile long nextLocalDiscoveryNanos; + private LocalRoute localRoute; + private boolean recoveryInProgress; + + /** A validated endpoint and its identity, replaced on every successful recovery. */ + static final class LocalRoute { + final String endpoint; + + private LocalRoute(final String endpoint) { + this.endpoint = endpoint; + } + } FeatureFlagRouteSelector() { this(System::nanoTime, DEFAULT_RECOVERY_INTERVAL_NANOS); @@ -29,9 +42,13 @@ enum Route { this.recoveryIntervalNanos = recoveryIntervalNanos; } - synchronized Route initialize(final boolean localAvailable, final boolean directAvailable) { - if (route == Route.UNINITIALIZED || route == Route.UNAVAILABLE) { - if (localAvailable) { + synchronized Route initialize( + final Supplier discoverEndpoint, final boolean directAvailable) { + if (route == Route.UNINITIALIZED) { + // Startup happens once across both writers. Later unavailable probes run outside this lock. + final String endpoint = discoverEndpoint.get(); + if (endpoint != null) { + localRoute = new LocalRoute(endpoint); route = Route.LOCAL; } else if (directAvailable) { route = Route.DIRECT; @@ -47,8 +64,13 @@ Route current() { return route; } - synchronized Route localFailure(final boolean directAvailable) { - if (route == Route.LOCAL) { + synchronized @Nullable LocalRoute localRoute() { + return route == Route.LOCAL ? localRoute : null; + } + + synchronized Route localFailure(final LocalRoute failedRoute, final boolean directAvailable) { + // An in-flight send on an old endpoint cannot invalidate a newly recovered route. + if (route == Route.LOCAL && localRoute == failedRoute) { if (directAvailable) { route = Route.DIRECT; } else { @@ -60,16 +82,24 @@ synchronized Route localFailure(final boolean directAvailable) { } synchronized boolean tryBeginLocalRecovery() { - if (route != Route.UNAVAILABLE || nanoTime.getAsLong() - nextLocalDiscoveryNanos < 0) { + if (route != Route.UNAVAILABLE + || recoveryInProgress + || nanoTime.getAsLong() - nextLocalDiscoveryNanos < 0) { return false; } - scheduleLocalDiscovery(); + recoveryInProgress = true; return true; } - synchronized void localRecovered() { + synchronized void localRecoveryFinished(@Nullable final String endpoint) { + recoveryInProgress = false; if (route == Route.UNAVAILABLE) { - route = Route.LOCAL; + if (endpoint != null) { + localRoute = new LocalRoute(endpoint); + route = Route.LOCAL; + } else { + scheduleLocalDiscovery(); + } } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java index 69792ae35bf..493126b94f0 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java @@ -21,6 +21,9 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.LongSupplier; +import java.util.function.Supplier; import java.util.stream.Stream; import javax.annotation.Nullable; import okhttp3.MediaType; @@ -41,7 +44,7 @@ void replaysRejectedBatchDirectlyAndKeepsDirectRoute(final int statusCode) throw final RecordingBackendApi direct = new RecordingBackendApi(); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, null, () -> local, @@ -73,7 +76,7 @@ void fallsBackAfterConnectionRefusal(final String route, final String eventType) new RecordingBackendApi(new ConnectException("connection refused")); final RecordingBackendApi direct = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi(local, null, () -> local, () -> direct, eventType); + createApi(local, null, () -> local, () -> direct, eventType); api.post(route, requestBody(eventType), stream -> null, null, false); @@ -89,7 +92,7 @@ void keepsDirectRouteStickyAfterLocalFailure() throws Exception { final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); final RecordingBackendApi direct = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( unavailableLocal, null, () -> recoveredLocal, () -> direct, "exposure", clock::get, 10); api.post("exposures", requestBody("first"), stream -> null, null, false); @@ -127,7 +130,7 @@ void startsDirectAndNeverProbesLocal() throws Exception { final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); final AtomicInteger proxyApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( null, direct, () -> { @@ -158,7 +161,7 @@ void concurrentSendersDoNotBlockOnOrDuplicateARecoveryProbe() throws Exception { final CountDownLatch probeStarted = new CountDownLatch(1); final CountDownLatch releaseProbe = new CountDownLatch(1); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( null, null, () -> { @@ -205,7 +208,7 @@ void failedRecoveryIsStickyForAnotherCooldown() throws Exception { final AtomicLong clock = new AtomicLong(); final AtomicInteger proxyApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( null, null, () -> { @@ -238,7 +241,7 @@ void doesNotRetryDirectApiCreationWhenFallbackIsUnavailable() { new RecordingBackendApi(new HttpResponseException(404, "rejected")); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, null, () -> local, @@ -262,7 +265,7 @@ void doesNotRetryDirectApiCreationWhenFallbackIsUnavailable() { @Test void permitsUnavailableStartupSoLocalDeliveryCanRecover() { final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi(null, null, () -> null, () -> null, "flag evaluation"); + createApi(null, null, () -> null, () -> null, "flag evaluation"); assertThrows( IOException.class, @@ -275,7 +278,7 @@ void propagatesDirectFailuresWithoutTryingToFallback() { final RecordingBackendApi direct = new RecordingBackendApi(failure); final AtomicInteger proxyApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( null, direct, () -> { @@ -334,7 +337,7 @@ public T post( }; final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, null, () -> local, @@ -367,7 +370,7 @@ void sharesRouteTransitionsAcrossExposureAndFlagEvaluationWriters() throws Excep final RecordingBackendApi evaluationLocal = new RecordingBackendApi(); final RecordingBackendApi evaluationDirect = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi exposureApi = - new AgentlessFeatureFlagBackendApi( + createApi( exposureLocal, exposureDirect, () -> exposureLocal, @@ -375,7 +378,7 @@ void sharesRouteTransitionsAcrossExposureAndFlagEvaluationWriters() throws Excep "exposure", routeSelector); final AgentlessFeatureFlagBackendApi evaluationApi = - new AgentlessFeatureFlagBackendApi( + createApi( evaluationLocal, evaluationDirect, () -> evaluationLocal, @@ -395,51 +398,11 @@ void sharesRouteTransitionsAcrossExposureAndFlagEvaluationWriters() throws Excep } @Test - void sharedLocalRouteDiscoversWriterSpecificProxy() throws Exception { - final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); - routeSelector.initialize(true, false); - final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); - final AtomicInteger proxyApiCreations = new AtomicInteger(); - final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( - null, - null, - () -> { - proxyApiCreations.incrementAndGet(); - return recoveredLocal; - }, - () -> null, - "flag evaluation", - routeSelector); - - api.post("flagevaluation", requestBody("local"), stream -> null, null, false); - - assertEquals(1, proxyApiCreations.get()); - assertEquals(1, recoveredLocal.calls); - assertEquals(FeatureFlagRouteSelector.Route.LOCAL, routeSelector.current()); - } - - @Test - void sharedLocalRouteUsesDirectWhenWriterSpecificProxyIsUnavailable() throws Exception { - final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); - routeSelector.initialize(true, false); - final RecordingBackendApi direct = new RecordingBackendApi(); - final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( - null, direct, () -> null, () -> direct, "flag evaluation", routeSelector); - - api.post("flagevaluation", requestBody("direct"), stream -> null, null, false); - - assertEquals(1, direct.calls); - assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); - } - - @Test - void sharedLocalRouteBecomesUnavailableWhenWriterHasNoRoute() { - final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); - routeSelector.initialize(true, false); + void failedRecoveryExceptionRemainsUnavailable() { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(clock::get, 10); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( null, null, () -> { @@ -448,7 +411,7 @@ void sharedLocalRouteBecomesUnavailableWhenWriterHasNoRoute() { () -> null, "flag evaluation", routeSelector); - + clock.set(10); assertThrows( IOException.class, () -> api.post("flagevaluation", requestBody("missing"), stream -> null, null, false)); @@ -458,10 +421,9 @@ void sharedLocalRouteBecomesUnavailableWhenWriterHasNoRoute() { @Test void sharedDirectRouteWithoutWriterClientIsUnavailable() { final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); - routeSelector.initialize(false, true); + routeSelector.initialize(() -> null, true); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( - null, null, () -> null, () -> null, "flag evaluation", routeSelector); + createApi(null, null, () -> null, () -> null, "flag evaluation", routeSelector); assertThrows( IOException.class, @@ -475,7 +437,7 @@ void workingDirectRouteDoesNotAttemptRecovery() throws Exception { final RecordingBackendApi direct = new RecordingBackendApi(); final AtomicInteger proxyApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( null, direct, () -> { @@ -500,7 +462,7 @@ private static void assertNoSameBatchReplayButUsesDirectForNext(final IOExceptio final RecordingBackendApi direct = new RecordingBackendApi(); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, null, () -> local, @@ -524,6 +486,54 @@ private static RequestBody requestBody(final String value) { return RequestBody.create(MediaType.parse("application/json"), value); } + private static AgentlessFeatureFlagBackendApi createApi( + BackendApi local, + BackendApi direct, + Supplier discoverLocal, + Supplier createDirect, + String eventType) { + return createApi( + local, direct, discoverLocal, createDirect, eventType, new FeatureFlagRouteSelector()); + } + + private static AgentlessFeatureFlagBackendApi createApi( + BackendApi local, + BackendApi direct, + Supplier discoverLocal, + Supplier createDirect, + String eventType, + LongSupplier clock, + long interval) { + return createApi( + local, + direct, + discoverLocal, + createDirect, + eventType, + new FeatureFlagRouteSelector(clock, interval)); + } + + private static AgentlessFeatureFlagBackendApi createApi( + BackendApi local, + BackendApi direct, + Supplier discoverLocal, + Supplier createDirect, + String eventType, + FeatureFlagRouteSelector selector) { + final AtomicReference localApi = new AtomicReference<>(local); + selector.initialize(() -> local == null ? null : "local", direct != null); + return new AgentlessFeatureFlagBackendApi( + direct, + () -> { + localApi.set(discoverLocal.get()); + return localApi.get() == null ? null : "local"; + }, + endpoint -> localApi.get(), + createDirect, + eventType, + selector); + } + private static void postWithoutFailure( final AgentlessFeatureFlagBackendApi api, final String body) { try { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index 8c243ae5a95..a8ac63bead9 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -2,6 +2,7 @@ import static datadog.communication.EvpProxy.ORIGIN_HEADER; import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; @@ -62,6 +63,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; import okhttp3.RequestBody; @@ -509,12 +511,14 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); final BackendApi proxyApi = mock(BackendApi.class); final BackendApi directApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi( + when(backendApiFactory.discoverEvpProxyEndpoint( + false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + .thenReturn(V4_EVP_PROXY_ENDPOINT); + when(backendApiFactory.createEvpProxyApiForEndpoint( Intake.EVENT_PLATFORM, true, HttpRetryPolicy.Factory.NEVER_RETRY, - false, - asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + V4_EVP_PROXY_ENDPOINT)) .thenReturn(proxyApi); when(backendApiFactory.createDirectIntakeApi(eq(Intake.EVENT_PLATFORM), eq(true), eq(false))) .thenReturn(directApi); @@ -569,6 +573,49 @@ void testAgentlessWriterWaitsForUnavailableProxyRecovery() throws Exception { } } + @Test + void idleUnavailableWriterOnlyProbesForANewBatchAndStillCloses() throws Exception { + final Config config = mockConfig("idle-recovery"); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); + final AtomicLong clock = new AtomicLong(); + final BackendApiFactory backend = mock(BackendApiFactory.class); + final BackendApi proxy = mock(BackendApi.class); + when(backend.discoverEvpProxyEndpoint(true, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + .thenReturn(V4_EVP_PROXY_ENDPOINT); + when(backend.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + true, + HttpRetryPolicy.Factory.NEVER_RETRY, + V4_EVP_PROXY_ENDPOINT)) + .thenReturn(proxy); + final FeatureFlagBackendApiFactory factory = + new FeatureFlagBackendApiFactory( + config, + backend, + FeatureFlagEventType.EXPOSURE, + new FeatureFlagRouteSelector(clock::get, 10)); + final ExposureWriterImpl writer = + new ExposureWriterImpl(16, 100, MILLISECONDS, factory, config); + try { + writer.init(); + poll.eventually( + () -> + verify(backend) + .discoverEvpProxyEndpoint(false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + clock.set(10); + MILLISECONDS.sleep(300); + verify(backend, never()) + .discoverEvpProxyEndpoint(true, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + verify(proxy, never()).post(any(), any(), any(), any(), eq(false)); + writer.accept(buildExposure()); + poll.eventually(() -> verify(proxy).post(eq("exposures"), any(), any(), any(), eq(false))); + verify(backend).discoverEvpProxyEndpoint(true, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + } finally { + writer.close(); + } + assertFalse(writer.isSerializerThreadAlive()); + } + private static Config mockConfig(String serviceName) { return mockConfig(serviceName, "test", "0.0.0"); } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java index f77d064b9c0..e65fafa8bd5 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java @@ -41,12 +41,7 @@ void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { assertSame(proxyApi, selected); verify(backendApiFactory, never()) - .createEvpProxyApi( - Intake.EVENT_PLATFORM, - false, - HttpRetryPolicy.Factory.NEVER_RETRY, - false, - asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + .discoverEvpProxyEndpoint(false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @@ -54,13 +49,9 @@ void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { void agentlessPrefersCapabilityGatedLocalRouteWithDirectFallbackReady() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, - false, - HttpRetryPolicy.Factory.NEVER_RETRY, - false, - asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) - .thenReturn(mock(BackendApi.class)); + when(backendApiFactory.discoverEvpProxyEndpoint( + false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + .thenReturn(V2_EVP_PROXY_ENDPOINT); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenReturn(mock(BackendApi.class)); @@ -69,12 +60,7 @@ void agentlessPrefersCapabilityGatedLocalRouteWithDirectFallbackReady() { assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); verify(backendApiFactory) - .createEvpProxyApi( - Intake.EVENT_PLATFORM, - false, - HttpRetryPolicy.Factory.NEVER_RETRY, - false, - asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + .discoverEvpProxyEndpoint(false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); verify(backendApiFactory).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @@ -107,14 +93,9 @@ void agentlessKeepsWriterAliveWhileEveryRouteIsUnavailable() { void agentlessKeepsCompatibleLocalRouteWhenDirectUrlIsInvalid() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi proxyApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, - false, - HttpRetryPolicy.Factory.NEVER_RETRY, - false, - asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) - .thenReturn(proxyApi); + when(backendApiFactory.discoverEvpProxyEndpoint( + false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + .thenReturn(V2_EVP_PROXY_ENDPOINT); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenThrow(new IllegalArgumentException("invalid URL")); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java index d95cacdb94b..8be8de52d89 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java @@ -2,6 +2,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.concurrent.atomic.AtomicLong; import org.junit.jupiter.api.Test; @@ -12,22 +14,53 @@ class FeatureFlagRouteSelectorTest { void directRouteIsTerminalAcrossInitializationAndRecoverySignals() { final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); - assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(false, true)); - assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(true, false)); + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(() -> null, true)); + assertEquals( + FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(() -> "v4", false)); assertFalse(routeSelector.tryBeginLocalRecovery()); - routeSelector.localRecovered(); + routeSelector.localRecoveryFinished("v4"); assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); } @Test - void unavailableRouteCanBeReinitializedWithLocal() { + void anotherWriterCannotBypassTheRecoveryCooldown() { final AtomicLong clock = new AtomicLong(); final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(clock::get, 10); assertEquals( - FeatureFlagRouteSelector.Route.UNAVAILABLE, routeSelector.initialize(false, false)); - assertEquals(FeatureFlagRouteSelector.Route.LOCAL, routeSelector.initialize(true, false)); + FeatureFlagRouteSelector.Route.UNAVAILABLE, routeSelector.initialize(() -> null, false)); + assertEquals( + FeatureFlagRouteSelector.Route.UNAVAILABLE, + routeSelector.initialize( + () -> { + throw new AssertionError("duplicate discovery"); + }, + false)); + assertFalse(routeSelector.tryBeginLocalRecovery()); + clock.set(10); + assertTrue(routeSelector.tryBeginLocalRecovery()); + clock.set(100); + assertFalse(routeSelector.tryBeginLocalRecovery()); + routeSelector.localRecoveryFinished(null); + assertFalse(routeSelector.tryBeginLocalRecovery()); + clock.set(110); + assertTrue(routeSelector.tryBeginLocalRecovery()); + } + + @Test + void staleFailureCannotInvalidateANewGenerationOfTheSameEndpoint() { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + selector.initialize(() -> "v4", false); + final FeatureFlagRouteSelector.LocalRoute oldRoute = selector.localRoute(); + selector.localFailure(oldRoute, false); + clock.set(10); + assertTrue(selector.tryBeginLocalRecovery()); + selector.localRecoveryFinished("v4"); + assertNotSame(oldRoute, selector.localRoute()); + selector.localFailure(oldRoute, true); + assertEquals(FeatureFlagRouteSelector.Route.LOCAL, selector.current()); } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java new file mode 100644 index 00000000000..60d39e6faf1 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java @@ -0,0 +1,207 @@ +package com.datadog.featureflag; + +import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; +import static com.datadog.featureflag.FeatureFlagEventType.FLAG_EVALUATION; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; +import static org.junit.jupiter.api.Assertions.assertEquals; +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.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.communication.HttpResponseException; +import datadog.communication.ddagent.DDAgentFeaturesDiscovery; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.Response; +import okhttp3.ResponseBody; +import org.junit.jupiter.api.Test; + +/** Exercises both production factories and HTTP clients against one shared discovery result. */ +class SharedFeatureFlagRouteTest { + private final AtomicLong clock = new AtomicLong(); + private final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + private final DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); + private final Config config = mock(Config.class); + private final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + private final List requests = new ArrayList<>(); + private int status = 200; + + SharedFeatureFlagRouteTest() { + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); + when(config.getIdGenerationStrategy()).thenReturn(Config.get().getIdGenerationStrategy()); + when(discovery.supportsEvpProxyHeaders(any())).thenReturn(true); + sco.setFeaturesDiscovery(discovery); + sco.agentUrl = HttpUrl.get("http://localhost:8126/agent-prefix/"); + sco.agentHttpClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + requests.add(chain.request()); + return new Response.Builder() + .request(chain.request()) + .protocol(Protocol.HTTP_1_1) + .code(status) + .message("test") + .body(ResponseBody.create(null, "{}")) + .build(); + }) + .build(); + } + + @Test + void bothWritersUseOneSuccessfulRecoveryWithoutRediscovery() throws Exception { + final BackendApi exposures = create(EXPOSURE); + final BackendApi evaluations = create(FLAG_EVALUATION); + // Only the first recovery can discover a route. A redundant sibling probe would lose it. + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT).thenReturn(null); + clock.set(10); + + post(exposures, "exposures"); + post(evaluations, "flagevaluation"); + + assertEquals( + Arrays.asList( + "/agent-prefix/evp_proxy/v4/api/v2/exposures", + "/agent-prefix/evp_proxy/v4/api/v2/flagevaluation"), + paths()); + assertEquals("gzip", requests.get(0).header("Accept-Encoding")); + assertEquals("identity", requests.get(1).header("Accept-Encoding")); + verify(discovery).discover(); + verify(discovery).discoverIfOutdated(); + } + + @Test + void bothWritersReplaceTheirOldEndpointAfterRecovery() throws Exception { + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + final BackendApi exposures = create(EXPOSURE); + final BackendApi evaluations = create(FLAG_EVALUATION); + post(evaluations, "flagevaluation"); + status = 404; + assertThrows(HttpResponseException.class, () -> post(exposures, "exposures")); + when(discovery.getEvpProxyEndpoint()).thenReturn(V2_EVP_PROXY_ENDPOINT); + status = 200; + clock.set(10); + + post(exposures, "exposures"); + post(evaluations, "flagevaluation"); + + assertEquals( + Arrays.asList( + "/agent-prefix/evp_proxy/v4/api/v2/flagevaluation", + "/agent-prefix/evp_proxy/v4/api/v2/exposures", + "/agent-prefix/evp_proxy/v2/api/v2/exposures", + "/agent-prefix/evp_proxy/v2/api/v2/flagevaluation"), + paths()); + verify(discovery, times(1)).discover(); + } + + private BackendApi create(FeatureFlagEventType eventType) { + return new FeatureFlagBackendApiFactory(config, sco, eventType, selector).create(); + } + + @Test + void concurrentInitializationSharesOneDiscovery() throws Exception { + final CountDownLatch started = new CountDownLatch(1); + final CountDownLatch release = new CountDownLatch(1); + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + doAnswer( + invocation -> { + started.countDown(); + assertTrue(release.await(5, TimeUnit.SECONDS)); + return null; + }) + .when(discovery) + .discoverIfOutdated(); + final CompletableFuture exposures = + CompletableFuture.supplyAsync(() -> create(EXPOSURE)); + try { + assertTrue(started.await(5, TimeUnit.SECONDS)); + final CompletableFuture evaluations = + CompletableFuture.supplyAsync(() -> create(FLAG_EVALUATION)); + release.countDown(); + post(exposures.get(5, TimeUnit.SECONDS), "exposures"); + post(evaluations.get(5, TimeUnit.SECONDS), "flagevaluation"); + verify(discovery).discoverIfOutdated(); + assertEquals(2, requests.size()); + } finally { + release.countDown(); + } + } + + @Test + void slowRecoveryDoesNotPermitASecondProbeAfterTheCooldown() throws Exception { + final BackendApi exposures = create(EXPOSURE); + final BackendApi evaluations = create(FLAG_EVALUATION); + final CountDownLatch started = new CountDownLatch(1); + final CountDownLatch release = new CountDownLatch(1); + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + doAnswer( + invocation -> { + started.countDown(); + assertTrue(release.await(5, TimeUnit.SECONDS)); + return null; + }) + .when(discovery) + .discover(); + clock.set(10); + final CompletableFuture recovery = + CompletableFuture.runAsync( + () -> { + try { + post(exposures, "exposures"); + } catch (Exception failure) { + throw new AssertionError(failure); + } + }); + try { + assertTrue(started.await(5, TimeUnit.SECONDS)); + clock.set(100); + assertThrows(IOException.class, () -> post(evaluations, "flagevaluation")); + } finally { + release.countDown(); + } + recovery.get(5, TimeUnit.SECONDS); + post(evaluations, "flagevaluation"); + verify(discovery).discover(); + assertEquals(2, requests.size()); + } + + private List paths() { + final List paths = new ArrayList<>(); + for (Request request : requests) { + paths.add(request.url().encodedPath()); + } + return paths; + } + + private static void post(BackendApi api, String path) throws Exception { + api.post( + path, + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} From c3272076dbbc2a23a168562b0d7dc904768609fb Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Fri, 9 Oct 2026 20:13:04 +0000 Subject: [PATCH 6/8] fix(openfeature): address EVP transport and shutdown review Restore shared Agent URL compatibility, retain bounded pre-send Remote Config retries, and scope Feature Flagging shutdown to activated providers. Signal writers before sharing the native shutdown deadline; avoid interrupting sends and repeated unavailable-route errors. Add wire, lifecycle, and production-wiring regressions. SDK identity forwarding requirements move with identity emission in the child PR. --- .../communication/BackendApiFactory.java | 21 ++- .../ddagent/DDAgentFeaturesDiscovery.java | 7 +- .../communication/http/OkHttpUtils.java | 9 - .../communication/BackendApiFactoryTest.java | 8 +- .../communication/SendOnceTransportTest.java | 122 +++++++++++++ .../communication/http/AgentUrlPathTest.java | 20 --- .../java/lang/ShutdownInstrumentation.java | 12 +- .../lang/ShutdownInstrumentationTest.java | 24 ++- ...ceDisabledFeatureFlaggingShutdownTest.java | 120 ++++++++++--- .../FeatureFlaggingShutdownProbe.java | 54 ++++++ .../trace/util/AgentThreadFactory.java | 1 + .../featureflag/FeatureFlaggingSystem.java | 44 ++++- .../FeatureFlagRouteWiringTest.java | 132 +++++++++++++++ .../FeatureFlagShutdownBudgetTest.java | 108 ++++++++++++ .../FeatureFlaggingSystemTest.java | 49 ++++++ .../feature-flagging-lib/build.gradle.kts | 1 + .../AgentlessFeatureFlagBackendApi.java | 16 +- .../featureflag/ExposureWriterImpl.java | 45 +++-- .../FeatureFlagBackendApiFactory.java | 55 ++++-- .../featureflag/FeatureFlagEventWriter.java | 29 ++++ .../featureflag/FeatureFlagRouteSelector.java | 13 ++ .../featureflag/FlagEvaluationWriterImpl.java | 66 ++++---- .../AgentlessFeatureFlagBackendApiTest.java | 28 +++ .../featureflag/ExposureWriterTests.java | 33 ++-- .../FeatureFlagBackendApiFactoryTest.java | 21 +-- .../FeatureFlagRouteSelectorTest.java | 38 +++++ .../featureflag/FeatureFlagSendOnceTest.java | 160 ++++++++++++++++++ .../FeatureFlagTransportRegressionTest.java | 148 ++++++++++++++++ .../FeatureFlagWriterShutdownTest.java | 87 ++++++++++ .../FlagEvaluationWriterImplTest.java | 44 ++++- 30 files changed, 1319 insertions(+), 196 deletions(-) create mode 100644 communication/src/test/java/datadog/communication/SendOnceTransportTest.java delete mode 100644 communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java create mode 100644 dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java create mode 100644 products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java create mode 100644 products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java create mode 100644 products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java create mode 100644 products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java create mode 100644 products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java create mode 100644 products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java diff --git a/communication/src/main/java/datadog/communication/BackendApiFactory.java b/communication/src/main/java/datadog/communication/BackendApiFactory.java index 0514b39f33b..eeb960b3a70 100644 --- a/communication/src/main/java/datadog/communication/BackendApiFactory.java +++ b/communication/src/main/java/datadog/communication/BackendApiFactory.java @@ -1,6 +1,5 @@ package datadog.communication; -import static datadog.communication.http.OkHttpUtils.appendPath; import static java.util.Collections.emptyList; import datadog.communication.ddagent.DDAgentFeaturesDiscovery; @@ -32,9 +31,10 @@ public BackendApiFactory(Config config, SharedCommunicationObjects sharedCommuni /** * Creates a backend factory with optional send-once transport semantics. * - *

When {@code sendOnce} is true, both the explicit HTTP retry policy and OkHttp's automatic - * connection retry are disabled. This is required for event payloads that do not carry an - * idempotency key. + *

When {@code sendOnce} is true, automatic connection retries and redirects are disabled, and + * the default HTTP retry policy is {@link HttpRetryPolicy.Factory#NEVER_RETRY}. Explicit caller + * policies may retry proven pre-send failures, but must not replay an ambiguously accepted event + * payload without an idempotency key. */ public BackendApiFactory( Config config, SharedCommunicationObjects sharedCommunicationObjects, boolean sendOnce) { @@ -238,19 +238,26 @@ private BackendApi createEvpProxyApi( intake, evpProxyEndpoint, responseCompression); - HttpUrl evpProxyUrl = appendPath(sharedCommunicationObjects.agentUrl, evpProxyEndpoint); + HttpUrl evpProxyUrl = sharedCommunicationObjects.agentUrl.resolve(evpProxyEndpoint); String subdomain = intake.getUrlPrefix(); return new EvpProxyApi( traceId, evpProxyUrl, subdomain, - sendOnce ? HttpRetryPolicy.Factory.NEVER_RETRY : retryPolicyFactory, + retryPolicyFactory, configureHttpClient(sharedCommunicationObjects.agentHttpClient), responseCompression); } OkHttpClient configureHttpClient(final OkHttpClient httpClient) { - return sendOnce ? httpClient.newBuilder().retryOnConnectionFailure(false).build() : httpClient; + return sendOnce + ? httpClient + .newBuilder() + .retryOnConnectionFailure(false) + .followRedirects(false) + .followSslRedirects(false) + .build() + : httpClient; } private HttpRetryPolicy.Factory retryPolicyFactory() { diff --git a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java index 5986bd44331..d3696c62f5c 100644 --- a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java +++ b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java @@ -1,7 +1,6 @@ package datadog.communication.ddagent; import static datadog.communication.http.OkHttpUtils.DATADOG_CONTAINER_TAGS_HASH; -import static datadog.communication.http.OkHttpUtils.appendPath; import static datadog.communication.http.OkHttpUtils.msgpackRequestBodyOf; import static datadog.communication.http.OkHttpUtils.prepareRequest; import static datadog.communication.serialization.msgpack.MsgPackWriter.FIXARRAY; @@ -159,7 +158,7 @@ private void doDiscovery(State newState) { try (Recording recording = discoveryTimer.start()) { boolean fallback = true; final Request request = - prepareRequest(appendPath(agentBaseUrl, "info"), emptyMap()).get().build(); + prepareRequest(agentBaseUrl.resolve("info"), emptyMap()).get().build(); try (Response response = client.newCall(request).execute()) { if (response.isSuccessful()) { processInfoResponseHeaders(response); @@ -209,7 +208,7 @@ private String probeTracesEndpoint(State newState, List endpoints) { try (Response response = client .newCall( - prepareRequest(appendPath(agentBaseUrl, candidate), emptyMap()) + prepareRequest(agentBaseUrl.resolve(candidate), emptyMap()) .put(msgpackRequestBodyOf(singletonList(ByteBuffer.wrap(PROBE_MESSAGE)))) .build()) .execute()) { @@ -437,7 +436,7 @@ public String getEvpProxyEndpoint() { } public HttpUrl buildUrl(String endpoint) { - return appendPath(agentBaseUrl, endpoint); + return agentBaseUrl.resolve(endpoint); } public boolean supportsDataStreams() { diff --git a/communication/src/main/java/datadog/communication/http/OkHttpUtils.java b/communication/src/main/java/datadog/communication/http/OkHttpUtils.java index 7ecffb29d8e..44105ae772d 100644 --- a/communication/src/main/java/datadog/communication/http/OkHttpUtils.java +++ b/communication/src/main/java/datadog/communication/http/OkHttpUtils.java @@ -40,15 +40,6 @@ import org.slf4j.LoggerFactory; public final class OkHttpUtils { - /** Appends a relative endpoint while preserving the configured Agent URL's path prefix. */ - public static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { - int firstCharacter = 0; - while (firstCharacter < path.length() && path.charAt(firstCharacter) == '/') { - firstCharacter++; - } - return baseUrl.newBuilder().addPathSegments(path.substring(firstCharacter)).build(); - } - private static final Logger log = LoggerFactory.getLogger(OkHttpUtils.class); private static final String DATADOG_META_LANG = "Datadog-Meta-Lang"; diff --git a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java index f3479237c01..155b2087789 100644 --- a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java +++ b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java @@ -218,7 +218,7 @@ void advertisedEvpProxyEndpointSupportsDisabledResponseCompression() throws Exce } @Test - void evpProxyPreservesConfiguredAgentBasePath() throws Exception { + void evpProxyKeepsHistoricalRelativeUrlResolution() throws Exception { final MockWebServer agent = new MockWebServer(); agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); agent.start(); @@ -237,14 +237,14 @@ void evpProxyPreservesConfiguredAgentBasePath() throws Exception { null, false); - assertEquals("/agent/base/evp_proxy/v4/api/v2/flagevaluation", agent.takeRequest().getPath()); + assertEquals("/agent/evp_proxy/v4/api/v2/flagevaluation", agent.takeRequest().getPath()); } finally { agent.shutdown(); } } @Test - void discoveryPreservesAgentBasePathAndAcceptsCaseInsensitiveIdentityCapabilities() + void discoveryKeepsHistoricalUrlResolutionAndAcceptsCaseInsensitiveCapabilities() throws Exception { final MockWebServer agent = new MockWebServer(); agent.enqueue( @@ -267,7 +267,7 @@ void discoveryPreservesAgentBasePathAndAcceptsCaseInsensitiveIdentityCapabilitie discovery.discover(); - assertEquals("/agent/base/info", agent.takeRequest().getPath()); + assertEquals("/agent/info", agent.takeRequest().getPath()); assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); assertTrue(discovery.supportsEvpProxyHeaders(asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); } finally { diff --git a/communication/src/test/java/datadog/communication/SendOnceTransportTest.java b/communication/src/test/java/datadog/communication/SendOnceTransportTest.java new file mode 100644 index 00000000000..9d761b98e90 --- /dev/null +++ b/communication/src/test/java/datadog/communication/SendOnceTransportTest.java @@ -0,0 +1,122 @@ +package datadog.communication; + +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.communication.http.HttpRetryPolicy; +import datadog.trace.api.Config; +import datadog.trace.api.intake.Intake; +import datadog.trace.test.junit.utils.config.WithConfig; +import datadog.trace.test.util.DDJavaSpecification; +import java.io.IOException; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.SocketPolicy; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.tabletest.junit.TableTest; + +/** Pins send-once on the clients that BackendApiFactory builds, for both EVP routes. */ +class SendOnceTransportTest extends DDJavaSpecification { + private MockWebServer server; + private OkHttpClient agentClient; + private OkHttpClient intakeClient; + + @BeforeEach + void setUp() throws IOException { + server = new MockWebServer(); + server.start(); + agentClient = new OkHttpClient(); + // Direct URLs are fixed to https://event-platform-intake./. Send them to the local + // server. + intakeClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request original = chain.request(); + final HttpUrl local = + original + .url() + .newBuilder() + .scheme("http") + .host(server.getHostName()) + .port(server.getPort()) + .build(); + return chain.proceed(original.newBuilder().url(local).build()); + }) + .build(); + } + + @AfterEach + void tearDown() throws IOException { + for (final OkHttpClient client : new OkHttpClient[] {agentClient, intakeClient}) { + client.dispatcher().executorService().shutdownNow(); + client.connectionPool().evictAll(); + } + server.shutdown(); + } + + @WithConfig(key = "API_KEY", value = "api-key", env = true) + @TableTest({ + "Scenario | Route | Send Once | Requests | Second Post Fails", + "proxy send-once | proxy | true | 2 | true ", + "proxy control | proxy | false | 3 | false ", + "direct send-once | direct | true | 2 | true ", + "direct control | direct | false | 3 | false " + }) + void doesNotReplayAPostAfterThePooledConnectionDrops( + final String route, final boolean sendOnce, final int requests, final boolean secondPostFails) + throws Exception { + // The first call leaves a pooled connection. The server drops it during the second call. + server.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + final BackendApiFactory factory = factory(sendOnce); + final BackendApi api = + route.equals("proxy") + ? factory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + V4_EVP_PROXY_ENDPOINT) + : factory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); + + post(api); + if (secondPostFails) { + assertThrows(IOException.class, () -> post(api)); + } else { + post(api); + } + + assertEquals(requests, server.getRequestCount()); + } + + private BackendApiFactory factory(final boolean sendOnce) { + final SharedCommunicationObjects sco = + new SharedCommunicationObjects() { + @Override + public OkHttpClient getIntakeHttpClient() { + return intakeClient; + } + }; + sco.agentUrl = server.url("/"); + sco.agentHttpClient = agentClient; + return new BackendApiFactory(Config.get(), sco, sendOnce); + } + + private static void post(final BackendApi api) throws IOException { + api.post( + "flagevaluation", + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} diff --git a/communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java b/communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java deleted file mode 100644 index a65bf049349..00000000000 --- a/communication/src/test/java/datadog/communication/http/AgentUrlPathTest.java +++ /dev/null @@ -1,20 +0,0 @@ -package datadog.communication.http; - -import static datadog.communication.http.OkHttpUtils.appendPath; -import static org.junit.jupiter.api.Assertions.assertEquals; - -import okhttp3.HttpUrl; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; - -class AgentUrlPathTest { - @ParameterizedTest - @ValueSource(strings = {"/agent-prefix", "/agent-prefix/", "/agent%2Fprefix/"}) - void preservesPrefixForDiscoveryAndDelivery(String prefix) { - final HttpUrl base = HttpUrl.get("http://localhost:8126" + prefix); - final String normalized = prefix.endsWith("/") ? prefix : prefix + "/"; - assertEquals(normalized + "info", appendPath(base, "info").encodedPath()); - assertEquals(normalized + "evp_proxy/v4/", appendPath(base, "/evp_proxy/v4/").encodedPath()); - assertEquals(normalized + "evp_proxy/v2/", appendPath(base, "evp_proxy/v2/").encodedPath()); - } -} diff --git a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java index 5c7685594de..9aa879ad705 100644 --- a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java +++ b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/main/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentation.java @@ -7,9 +7,7 @@ import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; -import datadog.trace.api.Config; import datadog.trace.bootstrap.instrumentation.shutdown.ShutdownHelper; -import java.util.Set; import net.bytebuddy.asm.Advice; /** @@ -17,7 +15,7 @@ * before the shutdown hooks are called.
*/ @AutoService(InstrumenterModule.class) -public class ShutdownInstrumentation extends InstrumenterModule +public class ShutdownInstrumentation extends InstrumenterModule.Tracing implements Instrumenter.ForBootstrap, Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { public ShutdownInstrumentation() { @@ -29,14 +27,6 @@ public String instrumentedType() { return "java.lang.Shutdown"; } - @Override - public boolean isApplicable(Set enabledSystems) { - // Preserve tracing's historical hook and include Feature Flagging's bounded final drains even - // when tracing is disabled. Do not instrument shutdown when both consumers are disabled. - return enabledSystems.contains(TargetSystem.TRACING) - || Config.get().isFeatureFlaggingProviderEnabled(); - } - @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( diff --git a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java index b7477805cd5..13736934cda 100644 --- a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java +++ b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java @@ -3,14 +3,34 @@ import static datadog.trace.agent.tooling.InstrumenterModule.TargetSystem.TRACING; import static java.util.Collections.emptySet; import static java.util.Collections.singleton; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; import datadog.trace.test.junit.utils.config.WithConfig; import datadog.trace.test.util.DDJavaSpecification; +import java.util.EnumSet; import org.junit.jupiter.api.Test; class ShutdownInstrumentationTest extends DDJavaSpecification { + @Test + void defaultConfigurationDoesNotEnableWholeAgentShutdownWithoutTracing() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "true") + void generatedTargetMaskDoesNotDependOnFeatureFlagBuildConfiguration() { + final EnumSet systems = EnumSet.noneOf(TargetSystem.class); + for (TargetSystem system : TargetSystem.values()) { + if (new ShutdownInstrumentation().isApplicable(singleton(system))) { + systems.add(system); + } + } + assertEquals(EnumSet.of(TRACING), systems); + } + @Test @WithConfig(key = "feature.flags.enabled", value = "false") void doesNotInstrumentShutdownWhenTracingAndFeatureFlagsAreDisabled() { @@ -25,8 +45,8 @@ void preservesTracingShutdown() { @Test @WithConfig(key = "feature.flags.configuration.source", value = "agentless") - void enablesFeatureFlagShutdownWithoutTracing() { - assertTrue(new ShutdownInstrumentation().isApplicable(emptySet())); + void featureFlagsDoNotEnableWholeAgentShutdownWithoutTracing() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); } @Test diff --git a/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java b/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java index 7dc4596c447..675b60a717a 100644 --- a/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java +++ b/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java @@ -1,45 +1,115 @@ package datadog.trace.agent; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonMap; 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.assertTrue; import datadog.trace.agent.test.IntegrationTestUtils; import java.io.ByteArrayOutputStream; import java.io.PrintStream; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; +import java.util.List; +import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; import jvmbootstraptest.AgentLoadedChecker; -import org.junit.jupiter.api.Test; +import jvmbootstraptest.FeatureFlaggingShutdownProbe; +import okhttp3.mockwebserver.Dispatcher; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +@Timeout(value = 30, unit = TimeUnit.SECONDS) class TraceDisabledFeatureFlaggingShutdownTest { - @Test - @Timeout(value = 30, unit = TimeUnit.SECONDS) - void featureFlaggingIsStoppedByTheRealAgentWhenTracingIsDisabled() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void unusedFeatureFlagsDoNotEnableWholeAgentShutdown(boolean explicitlyEnabled) throws Exception { + final List options = options(false); + if (explicitlyEnabled) { + options.add("-Ddd.feature.flags.enabled=true"); + } + final String logs = run(AgentLoadedChecker.class, options); + assertFalse(logs.contains("Shutting down agent"), logs); + assertFalse(logs.contains("Feature Flagging system stopped"), logs); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void activatedProviderFlushesBothEventTypesAtJvmExit(boolean tracing) throws Exception { + final LinkedBlockingQueue events = new LinkedBlockingQueue<>(); + try (MockWebServer server = new MockWebServer()) { + server.setDispatcher( + new Dispatcher() { + @Override + public MockResponse dispatch(RecordedRequest request) { + if (request.getPath().equals("/info")) { + return new MockResponse() + .setBody( + "{\"endpoints\":[\"v0.4/traces\",\"v0.5/traces\",\"evp_proxy/v4/\"]," + + "\"evp_proxy_allowed_headers\":[\"DD-EVP-ORIGIN\",\"DD-EVP-ORIGIN-VERSION\"]}"); + } + if (request.getPath().startsWith("/evp_proxy/")) { + events.add(request); + return new MockResponse().setResponseCode(202).setBody("{}"); + } + return new MockResponse().setResponseCode(404); + } + }); + server.start(); + final List options = options(tracing); + options.add("-Ddd.trace.agent.url=" + server.url("/")); + options.add("-Ddd.feature.flags.configuration.source=agentless"); + options.add( + "-Ddd.feature.flags.configuration.source.agentless.base.url=" + server.url("/config")); + final String logs = run(FeatureFlaggingShutdownProbe.class, options); + assertTrue(logs.contains("EXIT_WITH_QUEUED_FEATURE_FLAG_EVENTS"), logs); + assertEquals(tracing, logs.contains("Shutting down agent"), logs); + assertTrue(logs.contains("Feature Flagging system stopped"), logs); + + final List paths = new ArrayList<>(); + for (int i = 0; i < 2; i++) { + final RecordedRequest event = events.poll(1, TimeUnit.SECONDS); + assertNotNull(event, logs); + paths.add(event.getPath()); + assertEquals("POST", event.getMethod()); + assertNull(event.getHeader("DD-API-KEY")); + assertTrue(event.getBody().readUtf8().contains("shutdown-flag")); + } + assertTrue(paths.contains("/evp_proxy/v4/api/v2/exposures"), paths.toString()); + assertTrue(paths.contains("/evp_proxy/v4/api/v2/flagevaluation"), paths.toString()); + assertEquals(0, events.size(), "Shutdown must not duplicate either batch"); + } + } + + private static List options(boolean tracing) { + return new ArrayList<>( + Arrays.asList( + "-Ddatadog.slf4j.simpleLogger.defaultLogLevel=debug", + "-Ddd.trace.enabled=" + tracing, + "-Ddd.jmxfetch.enabled=false", + "-Ddd.profiling.enabled=false", + "-Ddd.appsec.enabled=false", + "-Ddd.remote_config.enabled=false", + "-Ddd.telemetry.enabled=false")); + } + + private static String run(Class main, List options) throws Exception { try (ByteArrayOutputStream output = new ByteArrayOutputStream(); - PrintStream printStream = new PrintStream(output, true, StandardCharsets.UTF_8.name())) { - int exitCode = + PrintStream stream = new PrintStream(output, true, StandardCharsets.UTF_8.name())) { + final int exit = IntegrationTestUtils.runOnSeparateJvm( - AgentLoadedChecker.class.getName(), - Arrays.asList( - "-Ddatadog.slf4j.simpleLogger.defaultLogLevel=debug", - "-Ddd.trace.enabled=false", - "-Ddd.feature.flags.enabled=true", - "-Ddd.feature.flags.configuration.source=agentless", - "-Ddd.jmxfetch.enabled=false", - "-Ddd.profiling.enabled=false", - "-Ddd.remote_config.enabled=false", - "-Ddd.telemetry.enabled=false"), - Collections.emptyList(), - Collections.emptyMap(), - printStream); - - String logs = output.toString(StandardCharsets.UTF_8.name()); - assertEquals(0, exitCode); - assertTrue(logs.contains("Shutting down agent")); - assertTrue(logs.contains("Feature Flagging system stopped")); + main.getName(), options, emptyList(), singletonMap("DD_API_KEY", ""), stream); + final String logs = output.toString(StandardCharsets.UTF_8.name()); + assertEquals(0, exit, logs); + return logs; } } } diff --git a/dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java b/dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java new file mode 100644 index 00000000000..2469b51f18b --- /dev/null +++ b/dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java @@ -0,0 +1,54 @@ +package jvmbootstraptest; + +import java.util.Collections; +import java.util.Map; + +/** Activates the agent's production gateway and leaves both event types queued at JVM exit. */ +public class FeatureFlaggingShutdownProbe { + public static void main(String[] args) throws Exception { + final Class gateway = type("FeatureFlaggingGateway"); + gateway.getMethod("activate").invoke(null); + final Object writer = gateway.getMethod("getFlagEvalWriter").invoke(null); + if (writer == null) { + throw new AssertionError("Provider activation did not start the evaluation writer"); + } + final Class exposure = type("exposure.ExposureEvent"); + final Class allocation = type("exposure.Allocation"); + final Class flag = type("exposure.Flag"); + final Class variant = type("exposure.Variant"); + final Class subject = type("exposure.Subject"); + final Object event = + exposure + .getConstructor(long.class, allocation, flag, variant, subject) + .newInstance( + System.currentTimeMillis(), + allocation.getConstructor(String.class).newInstance("allocation"), + flag.getConstructor(String.class).newInstance("shutdown-flag"), + variant.getConstructor(String.class).newInstance("on"), + subject + .getConstructor(String.class, Map.class) + .newInstance("shutdown-user", Collections.emptyMap())); + gateway.getMethod("dispatch", exposure).invoke(null, event); + + final Class evaluation = type("flagevaluation.FlagEvalEvent"); + final Object evalEvent = + evaluation + .getConstructor( + String.class, String.class, String.class, String.class, long.class, Map.class) + .newInstance( + "shutdown-flag", + "on", + "allocation", + "shutdown-user", + System.currentTimeMillis(), + Collections.emptyMap()); + type("flagevaluation.FlagEvaluationWriter") + .getMethod("enqueue", evaluation) + .invoke(writer, evalEvent); + System.out.println("EXIT_WITH_QUEUED_FEATURE_FLAG_EVENTS"); + } + + private static Class type(String suffix) throws ClassNotFoundException { + return Class.forName("datadog.trace.api.featureflag." + suffix, true, null); + } +} diff --git a/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java b/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java index 81cd9a0b294..a472bdbec9d 100644 --- a/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java +++ b/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java @@ -69,6 +69,7 @@ public enum AgentThread { LLMOBS_FEEDBACK_PROCESSOR("dd-llmobs-feedback-processor"), FEATURE_FLAG_EXPOSURE_PROCESSOR("dd-ffe-exposure-processor"), + FEATURE_FLAG_SHUTDOWN("dd-feature-flagging-shutdown"), FEATURE_FLAG_EVALUATION_PROCESSOR("dd-ffe-evaluation-processor"), FEATURE_FLAG_CONFIGURATION_POLLER("dd-feature-flagging-http-poller"); diff --git a/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java b/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java index 038c409cf64..1ef61e70328 100644 --- a/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java +++ b/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java @@ -2,6 +2,8 @@ import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_REMOTE_CONFIG; +import static datadog.trace.util.AgentThreadFactory.AgentThread.FEATURE_FLAG_SHUTDOWN; +import static datadog.trace.util.AgentThreadFactory.newAgentThread; import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.trace.api.Config; @@ -27,6 +29,7 @@ interface SystemInitializer { private static volatile SpanEnrichmentWriter SPAN_ENRICHMENT_WRITER; private static volatile FeatureFlaggingGateway.ActivationListener ACTIVATION_LISTENER; private static volatile boolean STARTED; + private static Thread SHUTDOWN_HOOK; private FeatureFlaggingSystem() {} @@ -82,6 +85,13 @@ private static void initializeOrRollBack( final SystemInitializer systemInitializer) { try { systemInitializer.initialize(sco, config); + if (!config.isTraceEnabled() && (EXPOSURE_WRITER != null || FLAG_EVAL_WRITER != null)) { + // Tracing already owns the pre-application-hook shutdown path. Without it, only an + // activated Feature Flagging subsystem needs a hook; do not stop other products. + final Thread hook = newAgentThread(FEATURE_FLAG_SHUTDOWN, FeatureFlaggingSystem::stop); + Runtime.getRuntime().addShutdownHook(hook); + SHUTDOWN_HOOK = hook; + } } catch (final RuntimeException | Error e) { stop(); throw e; @@ -169,6 +179,7 @@ static ConfigurationSourceService createConfigurationSourceService( justification = "Agent-internal class; Class object does not escape to app code and lock only guards the subsystem lifecycle.") public static synchronized void stop() { + final long deadline = FeatureFlagEventWriter.shutdownDeadline(); FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(false); FeatureFlaggingGateway.setFlagEvalWriter(null); final FeatureFlaggingGateway.ActivationListener activationListener = ACTIVATION_LISTENER; @@ -182,13 +193,24 @@ public static synchronized void stop() { SPAN_ENRICHMENT_WRITER = null; EXPOSURE_WRITER = null; CONFIG_SERVICE = null; + final Thread shutdownHook = SHUTDOWN_HOOK; + SHUTDOWN_HOOK = null; + if (shutdownHook != null && shutdownHook != Thread.currentThread()) { + try { + Runtime.getRuntime().removeShutdownHook(shutdownHook); + } catch (IllegalStateException ignored) { + // JVM shutdown has already started. The idempotent hook will observe the stopped state. + } + } if (activationListener != null) { FeatureFlaggingGateway.removeActivationListener(activationListener); } - closeQuietly(flagEvalWriter); + requestWriterShutdown(flagEvalWriter); + requestWriterShutdown(exposureWriter); closeQuietly(spanEnrichmentWriter); - closeQuietly(exposureWriter); closeQuietly(configService); + awaitWriterTermination(flagEvalWriter, deadline); + awaitWriterTermination(exposureWriter, deadline); LOGGER.debug("Feature Flagging system stopped"); } @@ -196,6 +218,24 @@ static boolean isAwaitingApplicationActivation() { return ACTIVATION_LISTENER != null; } + static boolean isShutdownHookRegistered() { + return SHUTDOWN_HOOK != null; + } + + private static void requestWriterShutdown(final AutoCloseable writer) { + if (writer instanceof FeatureFlagEventWriter) { + ((FeatureFlagEventWriter) writer).requestShutdown(); + } else { + closeQuietly(writer); + } + } + + private static void awaitWriterTermination(final AutoCloseable writer, final long deadline) { + if (writer instanceof FeatureFlagEventWriter) { + ((FeatureFlagEventWriter) writer).awaitTermination(deadline); + } + } + static boolean isExposureWriterStarted() { return EXPOSURE_WRITER != null; } diff --git a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java new file mode 100644 index 00000000000..8aedc5c06f8 --- /dev/null +++ b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java @@ -0,0 +1,132 @@ +package com.datadog.featureflag; + +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static datadog.trace.test.junit.utils.config.WithConfigExtension.injectSysConfig; +import static java.util.Collections.emptyMap; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.sun.net.httpserver.HttpServer; +import datadog.communication.ddagent.DDAgentFeaturesDiscovery; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +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.featureflag.flagevaluation.FlagEvalEvent; +import datadog.trace.test.junit.utils.config.WithConfig; +import java.net.InetSocketAddress; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import okhttp3.HttpUrl; +import okhttp3.OkHttpClient; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.Response; +import okhttp3.ResponseBody; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +class FeatureFlagRouteWiringTest { + @Test + @Timeout(15) + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + @WithConfig(key = "API_KEY", value = "test-api-key", env = true) + @WithConfig(key = "site", value = "datadoghq.com") + void productionWritersShareFallbackForFutureBatches() throws Exception { + final HttpServer configurationServer = + HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + configurationServer.createContext( + "/", + exchange -> { + exchange.sendResponseHeaders(404, -1); + exchange.close(); + }); + configurationServer.start(); + injectSysConfig( + "feature.flags.configuration.source.agentless.base.url", + "http://127.0.0.1:" + configurationServer.getAddress().getPort() + "/config"); + final List requests = new CopyOnWriteArrayList<>(); + final CountDownLatch exposureDelivered = new CountDownLatch(1); + final OkHttpClient client = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request request = chain.request(); + requests.add(request); + final boolean direct = + request.url().host().equals("event-platform-intake.datadoghq.com"); + if (direct && request.url().encodedPath().equals("/api/v2/exposures")) { + exposureDelivered.countDown(); + } + return new Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(direct ? 202 : 404) + .message("test") + .body(ResponseBody.create(null, "{}")) + .build(); + }) + .build(); + final DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); + when(discovery.supportsEvpProxyHeaders(any())).thenReturn(true); + when(discovery.supportsEvpProxy()).thenReturn(true); + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + final SharedCommunicationObjects sco = + new SharedCommunicationObjects() { + @Override + public OkHttpClient getIntakeHttpClient() { + return client; + } + + @Override + public DDAgentFeaturesDiscovery featuresDiscovery(Config config) { + return discovery; + } + }; + sco.agentUrl = HttpUrl.get("http://localhost:8126/"); + sco.agentHttpClient = client; + // A terminal local configuration response avoids retry backoff before event activation. + try { + FeatureFlaggingSystem.start(sco); + FeatureFlaggingGateway.activate(); + FeatureFlaggingGateway.dispatch( + new ExposureEvent( + 1, + new Allocation("allocation"), + new Flag("flag"), + new Variant("on"), + new Subject("user", emptyMap()))); + assertTrue(exposureDelivered.await(5, TimeUnit.SECONDS)); + FeatureFlaggingGateway.getFlagEvalWriter() + .enqueue(new FlagEvalEvent("flag", "on", "allocation", "user", 1, emptyMap())); + FeatureFlaggingSystem.stop(); + + assertEquals(3, requests.size(), "One rejected local exposure, then two direct batches"); + assertEquals("/evp_proxy/v4/api/v2/exposures", requests.get(0).url().encodedPath()); + assertNull(requests.get(0).header("DD-API-KEY")); + assertEquals("/api/v2/exposures", requests.get(1).url().encodedPath()); + assertEquals("/api/v2/flagevaluation", requests.get(2).url().encodedPath()); + for (int i = 1; i < requests.size(); i++) { + assertEquals("event-platform-intake.datadoghq.com", requests.get(i).url().host()); + assertEquals("test-api-key", requests.get(i).header("DD-API-KEY")); + assertNull(requests.get(i).header("X-Datadog-EVP-Subdomain")); + } + } finally { + FeatureFlaggingSystem.stop(); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + client.connectionPool().evictAll(); + client.dispatcher().executorService().shutdownNow(); + configurationServer.stop(0); + } + } +} diff --git a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java new file mode 100644 index 00000000000..3987d852b13 --- /dev/null +++ b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java @@ -0,0 +1,108 @@ +package com.datadog.featureflag; + +import static java.util.Collections.emptyMap; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.trace.api.Config; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +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.featureflag.flagevaluation.FlagEvalEvent; +import java.lang.reflect.Field; +import java.util.concurrent.CountDownLatch; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +@Timeout(15) +class FeatureFlagShutdownBudgetTest { + private final CountDownLatch bothPostsStarted = new CountDownLatch(2); + private final CountDownLatch releasePosts = new CountDownLatch(1); + private Thread stopping; + + @AfterEach + void cleanup() throws Exception { + releasePosts.countDown(); + if (stopping != null) { + stopping.join(5000); + assertFalse(stopping.isAlive()); + } + FeatureFlaggingSystem.stop(); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + } + + @Test + void stopSignalsBothWritersBeforeWaitingForEither() throws Exception { + startWriters(); + stopping = new Thread(FeatureFlaggingSystem::stop); + stopping.start(); + assertTrue( + bothPostsStarted.await(500, MILLISECONDS), + "Exposure shutdown was delayed by the evaluation writer's wait"); + } + + @Test + void stopUsesOneBoundedBudgetForBothBlockedPosts() throws Exception { + startWriters(); + final long started = System.nanoTime(); + FeatureFlaggingSystem.stop(); + final long elapsed = NANOSECONDS.toMillis(System.nanoTime() - started); + assertTrue(elapsed < 1300, "Two writer waits consumed " + elapsed + " ms"); + } + + private void startWriters() throws Exception { + final BackendApi api = mock(BackendApi.class); + when(api.post(any(), any(), any(), any(), anyBoolean())) + .thenAnswer( + invocation -> { + bothPostsStarted.countDown(); + boolean interrupted = false; + while (releasePosts.getCount() != 0) { + try { + assertTrue(releasePosts.await(12, SECONDS)); + } catch (InterruptedException ignored) { + interrupted = true; + } + } + if (interrupted) { + Thread.currentThread().interrupt(); + } + return null; + }); + final ExposureWriterImpl exposures = + new ExposureWriterImpl(16, Long.MAX_VALUE, NANOSECONDS, () -> api, Config.get(), 5000); + final FlagEvaluationWriterImpl evaluations = + new FlagEvaluationWriterImpl(16, Long.MAX_VALUE, NANOSECONDS, () -> api, Config.get()); + setWriter("EXPOSURE_WRITER", exposures); + setWriter("FLAG_EVAL_WRITER", evaluations); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + exposures.init(); + evaluations.start(); + exposures.accept( + new ExposureEvent( + 1, + new Allocation("alloc"), + new Flag("flag"), + new Variant("on"), + new Subject("user", emptyMap()))); + evaluations.enqueue(new FlagEvalEvent("flag", "on", "alloc", "user", 1, emptyMap())); + } + + private static void setWriter(String name, Object writer) throws Exception { + final Field field = FeatureFlaggingSystem.class.getDeclaredField(name); + field.setAccessible(true); + field.set(null, writer); + } +} diff --git a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java index 5b88389aefc..23b71f97807 100644 --- a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java +++ b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java @@ -16,6 +16,7 @@ import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -37,6 +38,54 @@ import org.junit.jupiter.api.Test; class FeatureFlaggingSystemTest { + @Test + @WithConfig(key = "trace.enabled", value = "false") + @WithConfig(key = FEATURE_FLAGS_CONFIGURATION_SOURCE, value = "agentless") + void traceDisabledHookFollowsActivationStopAndRestart() { + final SharedCommunicationObjects sco = sharedCommunicationObjects(); + final ExposureWriter writer = mock(ExposureWriter.class); + final FeatureFlaggingSystem.SystemInitializer initializer = + (ignoredSco, ignoredConfig) -> FeatureFlaggingSystem.initialize(null, writer); + for (int restart = 0; restart < 2; restart++) { + FeatureFlaggingSystem.start(sco, initializer); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + FeatureFlaggingGateway.activate(); + FeatureFlaggingGateway.activate(); + assertTrue(FeatureFlaggingSystem.isShutdownHookRegistered()); + FeatureFlaggingSystem.stop(); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + } + verify(writer, times(2)).init(); + verify(writer, times(2)).close(); + } + + @Test + @WithConfig(key = "trace.enabled", value = "false") + @WithConfig(key = FEATURE_FLAGS_CONFIGURATION_SOURCE, value = "agentless") + void failedActivationDoesNotLeaveAShutdownHook() { + final ExposureWriter writer = mock(ExposureWriter.class); + FeatureFlaggingSystem.start( + sharedCommunicationObjects(), + (sco, config) -> { + FeatureFlaggingSystem.initialize(null, writer); + throw new IllegalStateException("failed activation"); + }); + assertThrows(IllegalStateException.class, FeatureFlaggingGateway::activate); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + verify(writer).close(); + } + + @Test + @WithConfig(key = "trace.enabled", value = "true") + @WithConfig(key = FEATURE_FLAGS_CONFIGURATION_SOURCE, value = "agentless") + void tracingOwnsShutdownWithoutAnAdditionalFeatureFlagHook() { + FeatureFlaggingSystem.start( + sharedCommunicationObjects(), + (sco, config) -> FeatureFlaggingSystem.initialize(null, mock(ExposureWriter.class))); + FeatureFlaggingGateway.activate(); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + } + @AfterEach void resetFlagEvaluationGateway() { FeatureFlaggingSystem.stop(); diff --git a/products/feature-flagging/feature-flagging-lib/build.gradle.kts b/products/feature-flagging/feature-flagging-lib/build.gradle.kts index 4afd818b839..49a37d50b66 100644 --- a/products/feature-flagging/feature-flagging-lib/build.gradle.kts +++ b/products/feature-flagging/feature-flagging-lib/build.gradle.kts @@ -30,6 +30,7 @@ dependencies { testImplementation(libs.bundles.junit5) testImplementation(libs.bundles.mockito) + testImplementation(libs.okhttp3.mockwebserver) testImplementation(project(":products:feature-flagging:feature-flagging-config")) testImplementation(project(":utils:test-utils")) testImplementation(project(":dd-java-agent:testing")) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java index 36324e828bf..59919840ffe 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java @@ -77,7 +77,12 @@ public T post( private SelectedApi selectApi() throws IOException { if (routeSelector.tryBeginLocalRecovery()) { - routeSelector.localRecoveryFinished(discoverProxyEndpoint()); + String endpoint = null; + try { + endpoint = discoverProxyEndpoint(); + } finally { + routeSelector.localRecoveryFinished(endpoint); + } } final FeatureFlagRouteSelector.LocalRoute localRoute = routeSelector.localRoute(); @@ -129,7 +134,14 @@ private BackendApi getOrCreateDirectApi() { } private IOException unavailableRoute() { - return new IOException("No Feature Flagging " + eventType + " delivery route is available"); + return new UnavailableRouteException(eventType); + } + + /** Expected drop while the shared selector waits for a usable route. */ + static final class UnavailableRouteException extends IOException { + private UnavailableRouteException(final String eventType) { + super("No Feature Flagging " + eventType + " delivery route is available"); + } } private static boolean isSafeToReplayDirectly(final IOException exception) { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java index 7ca34d4c6cb..ffd56e8eb8f 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java @@ -2,6 +2,7 @@ import static datadog.trace.api.telemetry.LogCollector.EXCLUDE_TELEMETRY; import static datadog.trace.util.AgentThreadFactory.AgentThread.FEATURE_FLAG_EXPOSURE_PROCESSOR; +import static datadog.trace.util.AgentThreadFactory.THREAD_JOIN_TIMOUT_MS; import static datadog.trace.util.AgentThreadFactory.newAgentThread; import static java.util.concurrent.TimeUnit.SECONDS; @@ -24,14 +25,14 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class ExposureWriterImpl implements ExposureWriter { +public class ExposureWriterImpl implements ExposureWriter, FeatureFlagEventWriter { private static final Logger LOGGER = LoggerFactory.getLogger(ExposureWriterImpl.class); private static final int DEFAULT_CAPACITY = 1 << 16; // 65536 elements private static final int DEFAULT_FLUSH_INTERVAL_IN_SECONDS = 1; private static final int FLUSH_THRESHOLD = 100; private static final String EXPOSURES_ROUTE = "exposures"; - static final long SHUTDOWN_TIMEOUT_MILLIS = SECONDS.toMillis(5); + static final long SHUTDOWN_TIMEOUT_MILLIS = THREAD_JOIN_TIMOUT_MS; private final MessagePassingBlockingQueue queue; private final ExposureSerializingHandler serializer; @@ -40,10 +41,6 @@ public class ExposureWriterImpl implements ExposureWriter { private final AtomicBoolean closed = new AtomicBoolean(false); private final long shutdownTimeoutMillis; - public ExposureWriterImpl(final SharedCommunicationObjects sco, final Config config) { - this(DEFAULT_CAPACITY, DEFAULT_FLUSH_INTERVAL_IN_SECONDS, SECONDS, sco, config); - } - ExposureWriterImpl( final SharedCommunicationObjects sco, final Config config, @@ -121,7 +118,13 @@ public void init() { @Override public void close() { - final boolean workerRunning; + final long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(shutdownTimeoutMillis); + requestShutdown(); + awaitTermination(deadline); + } + + @Override + public void requestShutdown() { lifecycleLock.writeLock().lock(); try { if (!closed.compareAndSet(false, true)) { @@ -129,26 +132,16 @@ public void close() { } // Exclude all producers before asking the single consumer to perform its final drain. FeatureFlaggingGateway.removeExposureListener(this); - workerRunning = this.serializerThread.isAlive(); - if (workerRunning) { - serializer.requestShutdown(); - this.serializerThread.interrupt(); - } + // Polling wakes within 100 ms. An interrupt can race with, or abort, the final POST. + serializer.requestShutdown(); } finally { lifecycleLock.writeLock().unlock(); } + } - // start() failure invokes close() from the serializer itself. It cannot join itself, and no - // final flush is possible when no backend route was created. - if (!workerRunning || Thread.currentThread() == this.serializerThread) { - return; - } - try { - // Bound application shutdown even if the final best-effort network request does not return. - this.serializerThread.join(shutdownTimeoutMillis); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } + @Override + public void awaitTermination(final long deadlineNanos) { + FeatureFlagEventWriter.awaitThread(serializerThread, deadlineNanos); } @Override @@ -225,9 +218,7 @@ public void run() { } catch (InterruptedException e) { Thread.currentThread().interrupt(); } finally { - // close() interrupts this thread to wake queue.poll(). OkHttp fails fast when the calling - // thread is interrupted, so clear the flag for the final best-effort drain and restore it - // only after the final send-once attempt has completed. + // Preserve an unrelated interrupt without passing it into OkHttp's final best-effort send. final boolean wasInterrupted = Thread.interrupted(); try { drainAndFlush(); @@ -296,6 +287,8 @@ private void flush() { } try { evpPublisher.post(EXPOSURES_ROUTE, payload); + } catch (AgentlessFeatureFlagBackendApi.UnavailableRouteException ignored) { + // The shared selector reports unavailable/recovered transitions once for both writers. } catch (Exception e) { LOGGER.debug("Could not submit exposures", e); } finally { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java index e3f6ea40e36..c9b610a511f 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java @@ -1,10 +1,8 @@ package com.datadog.featureflag; -import static datadog.communication.EvpProxy.ORIGIN_HEADER; -import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; -import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; @@ -12,7 +10,9 @@ import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; +import java.net.ConnectException; import javax.annotation.Nullable; +import okhttp3.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -21,6 +21,37 @@ final class FeatureFlagBackendApiFactory { private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagBackendApiFactory.class); + /** Retries connection refusal only; automatic retries and redirects remain disabled. */ + static final HttpRetryPolicy.Factory PRE_SEND_RETRY = + new HttpRetryPolicy.Factory(3, 100, 2.0) { + @Override + public HttpRetryPolicy create() { + return new PreSendRetryPolicy(); + } + }; + + private static final class PreSendRetryPolicy extends HttpRetryPolicy { + private int attemptsLeft = 3; + + private PreSendRetryPolicy() { + super(3, 100, 2.0, false); + } + + @Override + public boolean shouldRetry(final Exception exception) { + if (exception instanceof ConnectException && attemptsLeft > 0) { + attemptsLeft--; + return true; + } + return false; + } + + @Override + public boolean shouldRetry(@Nullable final Response response) { + return false; + } + } + private final Config config; private final BackendApiFactory backendApiFactory; private final FeatureFlagEventType eventType; @@ -68,23 +99,17 @@ BackendApi create() { final boolean agentless = CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource()); if (!agentless) { - // Preserve the historical Agent-backed client: fixed EVP v2, no /info discovery, no direct - // credentials, and no Agentless route state. + // Remote Config stays Agent-only. Fixed v2 intentionally replaces Java's previous + // discovered endpoint; only connection refusal retains a bounded retry. return backendApiFactory.createEvpProxyApiForEndpoint( Intake.EVENT_PLATFORM, eventType.responseCompressionEnabled(), - HttpRetryPolicy.Factory.NEVER_RETRY, + PRE_SEND_RETRY, V2_EVP_PROXY_ENDPOINT); } final BackendApi directApi = createDirectApi(); - final FeatureFlagRouteSelector.Route route = - routeSelector.initialize(() -> discoverProxyEndpoint(false), directApi != null); - if (route == FeatureFlagRouteSelector.Route.UNAVAILABLE) { - LOGGER.warn( - "Feature Flagging {} delivery is waiting for a compatible local EVP proxy because direct intake credentials are unavailable", - eventType.logName()); - } + routeSelector.initialize(() -> discoverProxyEndpoint(false), directApi != null); return new AgentlessFeatureFlagBackendApi( directApi, () -> discoverProxyEndpoint(true), @@ -102,8 +127,8 @@ BackendApi create() { @Nullable private String discoverProxyEndpoint(final boolean forceDiscovery) { try { - return backendApiFactory.discoverEvpProxyEndpoint( - forceDiscovery, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + // Identity-forwarding requirements belong with SDK identity emission in the identity PR. + return backendApiFactory.discoverEvpProxyEndpoint(forceDiscovery, emptyList()); } catch (final RuntimeException exception) { LOGGER.debug("Could not discover the local Feature Flagging route", exception); return null; diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java new file mode 100644 index 00000000000..1c7ad1219da --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java @@ -0,0 +1,29 @@ +package com.datadog.featureflag; + +import static datadog.trace.util.AgentThreadFactory.THREAD_JOIN_TIMOUT_MS; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; + +/** Internal two-phase shutdown for writers sharing one subsystem termination budget. */ +interface FeatureFlagEventWriter extends AutoCloseable { + void requestShutdown(); + + void awaitTermination(long deadlineNanos); + + static long shutdownDeadline() { + return System.nanoTime() + MILLISECONDS.toNanos(THREAD_JOIN_TIMOUT_MS); + } + + static void awaitThread(Thread worker, long deadlineNanos) { + final long remaining = deadlineNanos - System.nanoTime(); + if (worker == Thread.currentThread() || remaining <= 0) { + return; + } + try { + // timedJoin preserves sub-millisecond budgets; never turn an expired budget into join(0). + NANOSECONDS.timedJoin(worker, remaining); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java index a4bb8497d85..c18e724cbed 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java @@ -4,10 +4,14 @@ import java.util.function.LongSupplier; import java.util.function.Supplier; import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** Process-wide route state shared by all Feature Flagging event writers. */ final class FeatureFlagRouteSelector { + private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagRouteSelector.class); + static final long DEFAULT_RECOVERY_INTERVAL_NANOS = TimeUnit.MINUTES.toNanos(1); enum Route { @@ -55,6 +59,7 @@ synchronized Route initialize( } else { route = Route.UNAVAILABLE; scheduleLocalDiscovery(); + logUnavailable(); } } return route; @@ -76,6 +81,7 @@ synchronized Route localFailure(final LocalRoute failedRoute, final boolean dire } else { route = Route.UNAVAILABLE; scheduleLocalDiscovery(); + logUnavailable(); } } return route; @@ -97,6 +103,7 @@ synchronized void localRecoveryFinished(@Nullable final String endpoint) { if (endpoint != null) { localRoute = new LocalRoute(endpoint); route = Route.LOCAL; + LOGGER.info("Feature Flagging event delivery recovered through the local EVP proxy"); } else { scheduleLocalDiscovery(); } @@ -106,4 +113,10 @@ synchronized void localRecoveryFinished(@Nullable final String endpoint) { private void scheduleLocalDiscovery() { nextLocalDiscoveryNanos = nanoTime.getAsLong() + recoveryIntervalNanos; } + + private static void logUnavailable() { + LOGGER.warn( + "Feature Flagging events are being dropped because no local EVP proxy or direct intake" + + " route is available. Make the Agent reachable, or configure DD_API_KEY and a valid DD_SITE."); + } } 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 4969376771c..c2a6231076c 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 @@ -48,10 +48,11 @@ * contend only on the MPSC queue, never on a monitor, so evaluation threads do not serialize * against each other. Shutdown: close() drains the queue and performs a final flush before the * worker thread exits. Because enqueue is lock-free, a producer can still offer during shutdown; - * close() sweeps the queue once the worker has been joined, counting any remainder as a closed drop - * so shutdown loss is observable rather than silent. + * close() sweeps the queue once the worker has been joined, counting the observed remainder as + * closed drops. A producer paused before offer can resume after this sweep; shutdown is best-effort + * for events racing close, not a producer fence. */ -public class FlagEvaluationWriterImpl implements FlagEvaluationWriter { +public class FlagEvaluationWriterImpl implements FlagEvaluationWriter, FeatureFlagEventWriter { private static final Logger LOGGER = LoggerFactory.getLogger(FlagEvaluationWriterImpl.class); @@ -99,15 +100,6 @@ private static void countMetric(final String metricName, final long value, final private final ConcurrentHashMap contextTruncatedCounts = new ConcurrentHashMap<>(); - public FlagEvaluationWriterImpl(final SharedCommunicationObjects sco, final Config config) { - this( - DEFAULT_CAPACITY, - FLUSH_INTERVAL_SECONDS, - SECONDS, - new FeatureFlagBackendApiFactory(config, sco, FeatureFlagEventType.FLAG_EVALUATION)::create, - config); - } - FlagEvaluationWriterImpl( final SharedCommunicationObjects sco, final Config config, @@ -172,7 +164,13 @@ int aggregatorFullTierSizeForTest() { @Override public void close() { - final boolean workerRunning; + final long deadline = FeatureFlagEventWriter.shutdownDeadline(); + requestShutdown(); + awaitTermination(deadline); + } + + @Override + public void requestShutdown() { synchronized (lifecycleLock) { if (!closed.compareAndSet(false, true)) { return; @@ -180,31 +178,26 @@ public void close() { // Disable and deregister from the gateway so no new events are enqueued. FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(false); FeatureFlaggingGateway.setFlagEvalWriter(null); - workerRunning = this.serializerThread.isAlive(); - if (workerRunning) { - // Ask the worker to drain the queue and final-flush, then interrupt to wake it from poll(). - serializer.requestShutdown(); - this.serializerThread.interrupt(); - } + // Polling wakes within 100 ms; do not interrupt a current or final HTTP request. + serializer.requestShutdown(); } + } + + @Override + public void awaitTermination(final long deadlineNanos) { if (Thread.currentThread() == this.serializerThread) { return; } - if (workerRunning) { - try { - // Bounded wait for the worker's final flush so queued events are not lost on shutdown. - this.serializerThread.join(TimeUnit.SECONDS.toMillis(5)); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } + FeatureFlagEventWriter.awaitThread(serializerThread, deadlineNanos); // enqueue() is lock-free, so a producer that passed the closed check before close() ran can // still land an event after the worker's final drain. Sweep the remainder so that loss is // counted rather than silently stranded in a queue nobody polls again. Only safe once the // worker is gone: the queue is single-consumer, so sweeping alongside a live worker would // break that contract. If join() timed out the worker is still draining, so skip the sweep. - if (!this.serializerThread.isAlive()) { - sweepAndCountResidualEvents(); + synchronized (lifecycleLock) { + if (!this.serializerThread.isAlive()) { + sweepAndCountResidualEvents(); + } } } @@ -232,8 +225,8 @@ public void enqueue(final FlagEvalEvent event) { // Deliberately lock-free: the hand-off queue is MPSC by design, so serializing producers on a // monitor here would negate that and turn every evaluation in every application thread into // contention on one lock. A producer that passed the check above can still offer after close() - // has started; that residue is accounted for by the worker's bounded post-drain passes and by - // close()'s post-join sweep, so shutdown loss stays observable. + // has started. The final drain and post-join sweep handle observed residue, but cannot account + // for an offer from a producer that resumes after both have finished. // // Safe publication of the event (including the context snapshot built by the hook) comes from // the queue's own offer/poll ordering, not from any monitor held here. @@ -363,12 +356,9 @@ public void run() { // On exit (interrupt or shutdown request), drain everything still buffered and flush it so // queued events are not lost on shutdown. // - // close() interrupts this thread to break it out of poll(). The final flush does socket - // I/O, and OkHttp fails fast on a thread whose interrupt flag is set, so clear the flag - // for the duration of the drain and restore it afterwards. Without this the flush that - // close() exists to guarantee throws IOException, the aggregated rows are discarded by - // the clear() in flush(), and the loss is invisible to the drop counters because those - // rows already left the queue. + // Normal close only signals shutdown. If an unrelated interrupt ends the duty cycle, + // clear it during the final socket I/O and restore it afterwards; OkHttp otherwise fails + // immediately without attempting the remaining batch. final boolean wasInterrupted = Thread.interrupted(); try { drainAndFlush(); @@ -482,6 +472,8 @@ void flush() { for (final byte[] payload : payloads.bodies) { evpPublisher.post(FLAG_EVALUATION_ROUTE, payload); } + } catch (AgentlessFeatureFlagBackendApi.UnavailableRouteException ignored) { + // The shared selector reports unavailable/recovered transitions once for both writers. } catch (Exception e) { LOGGER.error("Could not submit flag evaluations", e); } finally { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java index 493126b94f0..7ae0c3de5c3 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java @@ -36,6 +36,34 @@ class AgentlessFeatureFlagBackendApiTest { + @Test + void recoveryErrorDoesNotPermanentlyBlockTheSiblingWriter() throws Exception { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + final RecordingBackendApi recovered = new RecordingBackendApi(); + final AgentlessFeatureFlagBackendApi exposures = + createApi( + null, + null, + () -> { + throw new LinkageError("probe failure"); + }, + () -> null, + "exposure", + selector); + final AgentlessFeatureFlagBackendApi evaluations = + createApi(null, null, () -> recovered, () -> null, "flag evaluation", selector); + + clock.set(10); + assertThrows( + LinkageError.class, + () -> exposures.post("exposures", requestBody("first"), stream -> null, null, false)); + clock.set(20); + evaluations.post("flagevaluation", requestBody("second"), stream -> null, null, false); + + assertEquals(1, recovered.calls); + } + @ParameterizedTest @ValueSource(ints = {404, 405}) void replaysRejectedBatchDirectlyAndKeepsDirectRoute(final int statusCode) throws Exception { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index a8ac63bead9..35034cc06f9 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -1,10 +1,8 @@ package com.datadog.featureflag; -import static datadog.communication.EvpProxy.ORIGIN_HEADER; -import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; -import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -242,7 +240,9 @@ void testHighLoadScenario() throws Exception { List exposures = buildExposures(threads * exposuresPerThread); CountDownLatch latch = new CountDownLatch(1); - try (ExposureWriterImpl writer = new ExposureWriterImpl(sharedCommunicationObjects, config)) { + try (ExposureWriterImpl writer = + new ExposureWriterImpl( + sharedCommunicationObjects, config, new FeatureFlagRouteSelector())) { writer.init(); List> futures = new ArrayList<>(); for (int index = 0; index < exposures.size(); index += exposuresPerThread) { @@ -432,7 +432,10 @@ void testStaleGatewayDispatchCannotEnqueueAfterClose() throws Exception { @Test void testCloseBeforeInitPreventsLaterStartAndAccept() { ExposureWriterImpl writer = - new ExposureWriterImpl(sharedCommunicationObjects, mockConfig("never-started-service")); + new ExposureWriterImpl( + sharedCommunicationObjects, + mockConfig("never-started-service"), + new FeatureFlagRouteSelector()); writer.close(); writer.init(); @@ -511,8 +514,7 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); final BackendApi proxyApi = mock(BackendApi.class); final BackendApi directApi = mock(BackendApi.class); - when(backendApiFactory.discoverEvpProxyEndpoint( - false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + when(backendApiFactory.discoverEvpProxyEndpoint(false, emptyList())) .thenReturn(V4_EVP_PROXY_ENDPOINT); when(backendApiFactory.createEvpProxyApiForEndpoint( Intake.EVENT_PLATFORM, @@ -567,7 +569,9 @@ void testAgentlessWriterWaitsForUnavailableProxyRecovery() throws Exception { Config config = mockConfig("unavailable-service"); when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); - try (ExposureWriterImpl writer = new ExposureWriterImpl(sharedCommunicationObjects, config)) { + try (ExposureWriterImpl writer = + new ExposureWriterImpl( + sharedCommunicationObjects, config, new FeatureFlagRouteSelector())) { writer.init(); poll.eventually(() -> assertTrue(writer.isSerializerThreadAlive())); } @@ -580,8 +584,7 @@ void idleUnavailableWriterOnlyProbesForANewBatchAndStillCloses() throws Exceptio final AtomicLong clock = new AtomicLong(); final BackendApiFactory backend = mock(BackendApiFactory.class); final BackendApi proxy = mock(BackendApi.class); - when(backend.discoverEvpProxyEndpoint(true, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) - .thenReturn(V4_EVP_PROXY_ENDPOINT); + when(backend.discoverEvpProxyEndpoint(true, emptyList())).thenReturn(V4_EVP_PROXY_ENDPOINT); when(backend.createEvpProxyApiForEndpoint( Intake.EVENT_PLATFORM, true, @@ -598,18 +601,14 @@ void idleUnavailableWriterOnlyProbesForANewBatchAndStillCloses() throws Exceptio new ExposureWriterImpl(16, 100, MILLISECONDS, factory, config); try { writer.init(); - poll.eventually( - () -> - verify(backend) - .discoverEvpProxyEndpoint(false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + poll.eventually(() -> verify(backend).discoverEvpProxyEndpoint(false, emptyList())); clock.set(10); MILLISECONDS.sleep(300); - verify(backend, never()) - .discoverEvpProxyEndpoint(true, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + verify(backend, never()).discoverEvpProxyEndpoint(true, emptyList()); verify(proxy, never()).post(any(), any(), any(), any(), eq(false)); writer.accept(buildExposure()); poll.eventually(() -> verify(proxy).post(eq("exposures"), any(), any(), any(), eq(false))); - verify(backend).discoverEvpProxyEndpoint(true, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + verify(backend).discoverEvpProxyEndpoint(true, emptyList()); } finally { writer.close(); } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java index e65fafa8bd5..bc458093c34 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java @@ -2,12 +2,10 @@ import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; import static com.datadog.featureflag.FeatureFlagEventType.FLAG_EVALUATION; -import static datadog.communication.EvpProxy.ORIGIN_HEADER; -import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_REMOTE_CONFIG; -import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertSame; import static org.mockito.Mockito.mock; @@ -17,7 +15,6 @@ import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; -import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; import org.junit.jupiter.api.Test; @@ -32,7 +29,7 @@ void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { when(backendApiFactory.createEvpProxyApiForEndpoint( Intake.EVENT_PLATFORM, false, - HttpRetryPolicy.Factory.NEVER_RETRY, + FeatureFlagBackendApiFactory.PRE_SEND_RETRY, V2_EVP_PROXY_ENDPOINT)) .thenReturn(proxyApi); @@ -40,17 +37,15 @@ void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); assertSame(proxyApi, selected); - verify(backendApiFactory, never()) - .discoverEvpProxyEndpoint(false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + verify(backendApiFactory, never()).discoverEvpProxyEndpoint(false, emptyList()); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @Test - void agentlessPrefersCapabilityGatedLocalRouteWithDirectFallbackReady() { + void agentlessPrefersLocalRouteWithDirectFallbackReady() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - when(backendApiFactory.discoverEvpProxyEndpoint( - false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + when(backendApiFactory.discoverEvpProxyEndpoint(false, emptyList())) .thenReturn(V2_EVP_PROXY_ENDPOINT); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenReturn(mock(BackendApi.class)); @@ -59,8 +54,7 @@ false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); - verify(backendApiFactory) - .discoverEvpProxyEndpoint(false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + verify(backendApiFactory).discoverEvpProxyEndpoint(false, emptyList()); verify(backendApiFactory).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @@ -93,8 +87,7 @@ void agentlessKeepsWriterAliveWhileEveryRouteIsUnavailable() { void agentlessKeepsCompatibleLocalRouteWhenDirectUrlIsInvalid() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - when(backendApiFactory.discoverEvpProxyEndpoint( - false, asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))) + when(backendApiFactory.discoverEvpProxyEndpoint(false, emptyList())) .thenReturn(V2_EVP_PROXY_ENDPOINT); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenThrow(new IllegalArgumentException("invalid URL")); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java index 8be8de52d89..7fd88d12776 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java @@ -5,11 +5,49 @@ import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertTrue; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import java.util.concurrent.atomic.AtomicLong; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; class FeatureFlagRouteSelectorTest { + @Test + void unavailableLoggingIsSharedAndOnlyRepeatsAfterRecovery() { + final Logger logger = (Logger) LoggerFactory.getLogger(FeatureFlagRouteSelector.class); + final ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + selector.initialize(() -> null, false); + selector.initialize(() -> null, false); + for (int i = 1; i <= 3; i++) { + clock.set(i * 10); + assertTrue(selector.tryBeginLocalRecovery()); + selector.localRecoveryFinished(null); + } + assertEquals(1, appender.list.size()); + assertEquals(Level.WARN, appender.list.get(0).getLevel()); + assertTrue(appender.list.get(0).getFormattedMessage().contains("DD_API_KEY")); + clock.set(40); + assertTrue(selector.tryBeginLocalRecovery()); + selector.localRecoveryFinished("v4"); + assertEquals(2, appender.list.size()); + assertEquals(Level.INFO, appender.list.get(1).getLevel()); + selector.localFailure(selector.localRoute(), false); + assertEquals(3, appender.list.size()); + assertEquals(Level.WARN, appender.list.get(2).getLevel()); + } finally { + logger.detachAppender(appender); + appender.stop(); + } + } + @Test void directRouteIsTerminalAcrossInitializationAndRecoverySignals() { final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java new file mode 100644 index 00000000000..27348f35421 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java @@ -0,0 +1,160 @@ +package com.datadog.featureflag; + +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.communication.ddagent.DDAgentFeaturesDiscovery; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import okhttp3.mockwebserver.SocketPolicy; +import org.junit.jupiter.api.Timeout; +import org.tabletest.junit.TableTest; + +@Timeout(15) +class FeatureFlagSendOnceTest { + @TableTest({ + "Source | Route | Event ", + "remote_config | proxy | EXPOSURE ", + "remote_config | proxy | FLAG_EVALUATION", + "agentless | proxy | EXPOSURE ", + "agentless | proxy | FLAG_EVALUATION", + "agentless | direct | EXPOSURE ", + "agentless | direct | FLAG_EVALUATION" + }) + void productionFactoryDoesNotReplayAnAcceptedPostOnConnectionLoss( + String source, String route, FeatureFlagEventType event) throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + server.enqueue(new MockResponse().setBody("{}")); + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(new MockResponse().setBody("{}")); + final OkHttpClient client = localClient(server); + try { + final BackendApi api = api(server, client, source, route, event); + post(api); + assertThrows(IOException.class, () -> post(api)); + assertEquals(2, server.getRequestCount()); + for (int i = 0; i < 2; i++) { + final RecordedRequest request = server.takeRequest(1, TimeUnit.SECONDS); + assertEquals("POST", request.getMethod()); + if (route.equals("proxy")) { + assertNull(request.getHeader("DD-API-KEY")); + } else { + assertEquals("test-api-key", request.getHeader("DD-API-KEY")); + assertNull(request.getHeader("X-Datadog-EVP-Subdomain")); + } + } + } finally { + client.connectionPool().evictAll(); + client.dispatcher().executorService().shutdownNow(); + } + } + } + + @TableTest({ + "Source | Route ", + "remote_config | proxy ", + "agentless | proxy ", + "agentless | direct" + }) + void redirectDoesNotForwardTheBatchOrCredentials(String source, String route) throws Exception { + try (MockWebServer server = new MockWebServer(); + MockWebServer destination = new MockWebServer()) { + server.start(); + destination.start(); + server.enqueue( + new MockResponse() + .setResponseCode(307) + .addHeader("Location", destination.url("/redirected"))); + destination.enqueue(new MockResponse().setBody("{}")); + final OkHttpClient client = localClient(server); + try { + final BackendApi api = api(server, client, source, route, FeatureFlagEventType.EXPOSURE); + assertThrows(IOException.class, () -> post(api)); + assertEquals(1, server.getRequestCount()); + assertNull(destination.takeRequest(200, TimeUnit.MILLISECONDS)); + } finally { + client.connectionPool().evictAll(); + client.dispatcher().executorService().shutdownNow(); + } + } + } + + private static OkHttpClient localClient(MockWebServer server) { + return new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request original = chain.request(); + final HttpUrl local = + original + .url() + .newBuilder() + .scheme("http") + .host(server.getHostName()) + .port(server.getPort()) + .build(); + return chain.proceed(original.newBuilder().url(local).build()); + }) + .build(); + } + + private static BackendApi api( + MockWebServer server, + OkHttpClient client, + String source, + String route, + FeatureFlagEventType event) { + final Config config = mock(Config.class); + when(config.getIdGenerationStrategy()).thenReturn(Config.get().getIdGenerationStrategy()); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(source); + when(config.getSite()).thenReturn("datadoghq.com"); + if (route.equals("direct")) { + when(config.getApiKey()).thenReturn("test-api-key"); + } + final DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); + when(discovery.supportsEvpProxyHeaders(any())).thenReturn(true); + when(discovery.supportsEvpProxy()).thenReturn(route.equals("proxy")); + when(discovery.getEvpProxyEndpoint()) + .thenReturn(route.equals("proxy") ? V4_EVP_PROXY_ENDPOINT : null); + final SharedCommunicationObjects sco = + new SharedCommunicationObjects() { + @Override + public OkHttpClient getIntakeHttpClient() { + return client; + } + + @Override + public DDAgentFeaturesDiscovery featuresDiscovery(Config ignored) { + return discovery; + } + }; + sco.agentUrl = server.url("/"); + sco.agentHttpClient = client; + return new FeatureFlagBackendApiFactory(config, sco, event).create(); + } + + private static void post(BackendApi api) throws IOException { + api.post( + "events", + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java new file mode 100644 index 00000000000..707c6fbaf36 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java @@ -0,0 +1,148 @@ +package com.datadog.featureflag; + +import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; +import static org.junit.jupiter.api.Assertions.assertEquals; +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.when; + +import datadog.communication.BackendApi; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import datadog.trace.api.ProtocolVersion; +import java.io.IOException; +import java.net.ConnectException; +import java.net.SocketException; +import java.net.SocketTimeoutException; +import java.util.ArrayList; +import java.util.List; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.Response; +import okhttp3.ResponseBody; +import org.junit.jupiter.api.Test; +import org.tabletest.junit.TableTest; + +class FeatureFlagTransportRegressionTest { + @TableTest({ + "Scenario | First Attempt | Attempts | Delivered", + "refused then accepted | refused | 2 | true ", + "refused throughout | refused-all | 4 | false ", + "forbidden | 403 | 1 | false ", + "throttled | 429 | 1 | false ", + "internal server error | 500 | 1 | false ", + "unavailable | 503 | 1 | false ", + "read timeout | timeout | 1 | false ", + "reset | reset | 1 | false ", + "broken pipe | broken-pipe | 1 | false " + }) + void remoteConfigRetriesOnlyProvenPreSendFailures( + String firstAttempt, int attempts, boolean delivered) throws Exception { + final List requests = new ArrayList<>(); + final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + sco.agentUrl = HttpUrl.get("http://localhost:8126/"); + sco.agentHttpClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request request = chain.request(); + requests.add(request); + if (firstAttempt.equals("refused-all")) { + throw new ConnectException("Connection refused"); + } + if (requests.size() == 1) { + switch (firstAttempt) { + case "refused": + throw new ConnectException("Connection refused"); + case "timeout": + throw new SocketTimeoutException("read timeout"); + case "reset": + throw new SocketException("Connection reset"); + case "broken-pipe": + throw new SocketException("Broken pipe"); + default: + return response(request, Integer.parseInt(firstAttempt), "{}"); + } + } + return response(request, 202, "{}"); + }) + .build(); + final BackendApi api = + new FeatureFlagBackendApiFactory(config("remote_config"), sco, EXPOSURE).create(); + + if (delivered) { + post(api); + } else { + assertThrows(IOException.class, () -> post(api)); + } + + assertEquals(attempts, requests.size()); + for (Request request : requests) { + assertEquals("/evp_proxy/v2/api/v2/exposures", request.url().encodedPath()); + assertNull(request.header("DD-API-KEY")); + } + } + + @Test + void agentlessWithoutIdentityKeepsAnOlderAgentUsableWithoutDirectCredentials() throws Exception { + final List requests = new ArrayList<>(); + final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + sco.agentUrl = HttpUrl.get("http://localhost:8126/"); + sco.agentHttpClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request request = chain.request(); + requests.add(request); + return response( + request, + 200, + request.url().encodedPath().equals("/info") + ? "{\"endpoints\":[\"v0.5/traces\",\"evp_proxy/v4/\"],\"evp_proxy_allowed_headers\":[]}" + : "{}"); + }) + .build(); + + final BackendApi api = + new FeatureFlagBackendApiFactory(config("agentless"), sco, EXPOSURE).create(); + post(api); + + assertEquals(2, requests.size()); + assertEquals("/info", requests.get(0).url().encodedPath()); + assertEquals("/evp_proxy/v4/api/v2/exposures", requests.get(1).url().encodedPath()); + assertNull(requests.get(1).header("DD-API-KEY")); + assertNull(requests.get(1).header("DD-EVP-ORIGIN")); + } + + private static Config config(String source) { + final Config config = mock(Config.class); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(source); + when(config.getIdGenerationStrategy()).thenReturn(Config.get().getIdGenerationStrategy()); + when(config.getProtocolVersion()).thenReturn(ProtocolVersion.V0_5); + return config; + } + + private static Response response(Request request, int code, String body) { + return new Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message("test") + .body(ResponseBody.create(null, body)) + .build(); + } + + private static void post(BackendApi api) throws IOException { + api.post( + "exposures", + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java new file mode 100644 index 00000000000..68153bd86ac --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java @@ -0,0 +1,87 @@ +package com.datadog.featureflag; + +import static com.datadog.featureflag.FlagEvaluationTestSupport.cfg; +import static com.datadog.featureflag.FlagEvaluationTestSupport.simpleEvent; +import static java.util.Collections.emptyMap; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +import datadog.trace.api.featureflag.exposure.Flag; +import datadog.trace.api.featureflag.exposure.Subject; +import datadog.trace.api.featureflag.exposure.Variant; +import java.util.concurrent.CountDownLatch; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class FeatureFlagWriterShutdownTest { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void closeDoesNotInterruptAnInFlightDelivery(boolean exposures) throws Exception { + final CountDownLatch postStarted = new CountDownLatch(1); + final CountDownLatch releasePost = new CountDownLatch(1); + final CountDownLatch interrupted = new CountDownLatch(1); + final BackendApi api = mock(BackendApi.class); + when(api.post(any(), any(), any(), any(), anyBoolean())) + .thenAnswer( + invocation -> { + postStarted.countDown(); + try { + assertTrue(releasePost.await(5, SECONDS)); + } catch (InterruptedException failure) { + interrupted.countDown(); + } + return null; + }); + final AutoCloseable writer; + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + if (exposures) { + final ExposureWriterImpl exposureWriter = + new ExposureWriterImpl(16, 0, MILLISECONDS, () -> api, cfg(), 800); + writer = exposureWriter; + exposureWriter.init(); + exposureWriter.accept( + new ExposureEvent( + 1, + new Allocation("alloc"), + new Flag("flag"), + new Variant("on"), + new Subject("user", emptyMap()))); + } else { + final FlagEvaluationWriterImpl evaluationWriter = + new FlagEvaluationWriterImpl(16, 0, MILLISECONDS, () -> api, cfg()); + writer = evaluationWriter; + evaluationWriter.startForTest(); + evaluationWriter.enqueue(simpleEvent("flag", "on")); + } + final Thread closer = + new Thread( + () -> { + try { + writer.close(); + } catch (Exception failure) { + throw new AssertionError(failure); + } + }); + try { + assertTrue(postStarted.await(5, SECONDS)); + closer.start(); + assertFalse(interrupted.await(200, MILLISECONDS), "Shutdown interrupted the active POST"); + } finally { + releasePost.countDown(); + closer.join(5000); + writer.close(); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + } + assertFalse(closer.isAlive()); + } +} 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 75a767c5c3f..c442747fea9 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 @@ -29,6 +29,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import ch.qos.logback.classic.Logger; import datadog.common.queue.MessagePassingBlockingQueue; import datadog.common.queue.Queues; import datadog.communication.BackendApi; @@ -44,6 +45,8 @@ import datadog.trace.api.intake.Intake; import datadog.trace.api.telemetry.CoreMetricCollector; import datadog.trace.api.telemetry.MetricCollector; +import datadog.trace.test.logging.TestLogCollector; +import datadog.trace.test.logging.TestLogbackAppender; import datadog.trace.test.util.PollingConditions; import java.io.IOException; import java.lang.reflect.Field; @@ -60,9 +63,47 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; class FlagEvaluationWriterImplTest { + @Test + void unavailableRouteDoesNotLogAnErrorForEachEvaluationFlush() { + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(); + selector.initialize(() -> null, false); + final BackendApi api = + new AgentlessFeatureFlagBackendApi( + null, () -> null, endpoint -> null, () -> null, "flag evaluation", selector); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(api); + final Logger logger = (Logger) LoggerFactory.getLogger(FlagEvaluationWriterImpl.class); + final TestLogbackAppender appender = new TestLogbackAppender(); + appender.setContext(logger.getLoggerContext()); + appender.start(); + logger.addAppender(appender); + TestLogCollector.enable(); + try { + logger.error("log-capture-positive-control"); + assertEquals(1, TestLogCollector.drainCapturedLogs().size()); + for (int i = 0; i < 3; i++) { + setup.handler.add(simpleEvent("flag", "on")); + setup.handler.drainAndAggregate(); + setup.handler.flush(); + } + assertEquals( + 0, + TestLogCollector.drainCapturedLogs().stream() + .filter( + log -> + log.level.equals("ERROR") + && log.message.contains("Could not submit flag evaluations")) + .count()); + } finally { + TestLogCollector.disable(); + logger.detachAppender(appender); + appender.stop(); + } + } + private static final String DIRECT_FLAG_EVALUATION_ENDPOINT = "/api/v2/flagevaluation"; private static final String API_KEY = "test-api-key"; private static final double TIMEOUT_SECONDS = 5; @@ -666,7 +707,8 @@ void encodeFailureClearsAggregatorSoLaterFlushesRecover() throws Exception { @Test void scoConstructorCreatesUsableWriter() { final FlagEvaluationWriterImpl writer = - new FlagEvaluationWriterImpl(new SharedCommunicationObjects(), cfg()); + new FlagEvaluationWriterImpl( + new SharedCommunicationObjects(), cfg(), new FeatureFlagRouteSelector()); writer.enqueue(simpleEvent("sco-flag", "on")); assertNotNull(writer.pollQueuedEventForTest()); writer.close(); From 18aa799ca0726c5eec41afb15b25cf65f50a8a16 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Mon, 12 Oct 2026 17:46:43 +0000 Subject: [PATCH 7/8] fix(openfeature): keep exposure shutdown independent of producers Keep exposure enqueueing lock-free and shutdown best effort. Preserve the existing bounded wait and avoid interrupting HTTP sends. Add a deterministic regression for a producer paused inside an offer. Environment: Datadog workspace --- .../featureflag/ExposureWriterImpl.java | 49 +++++++++-------- .../featureflag/ExposureWriterTests.java | 54 ++++++++++++++++++- 2 files changed, 80 insertions(+), 23 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java index ffd56e8eb8f..0584e5701ad 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java @@ -20,7 +20,6 @@ import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,7 +36,7 @@ public class ExposureWriterImpl implements ExposureWriter, FeatureFlagEventWrite private final MessagePassingBlockingQueue queue; private final ExposureSerializingHandler serializer; private final Thread serializerThread; - private final ReentrantReadWriteLock lifecycleLock = new ReentrantReadWriteLock(); + private final Object lifecycleLock = new Object(); private final AtomicBoolean closed = new AtomicBoolean(false); private final long shutdownTimeoutMillis; @@ -89,7 +88,24 @@ public class ExposureWriterImpl implements ExposureWriter, FeatureFlagEventWrite final Supplier backendApiSupplier, final Config config, final long shutdownTimeoutMillis) { - this.queue = Queues.mpscBlockingConsumerArrayQueue(capacity); + this( + Queues.mpscBlockingConsumerArrayQueue(capacity), + flushInterval, + timeUnit, + backendApiSupplier, + config, + shutdownTimeoutMillis); + } + + @VisibleForTesting + ExposureWriterImpl( + final MessagePassingBlockingQueue queue, + final long flushInterval, + final TimeUnit timeUnit, + final Supplier backendApiSupplier, + final Config config, + final long shutdownTimeoutMillis) { + this.queue = queue; this.serializer = new ExposureSerializingHandler( backendApiSupplier, @@ -104,15 +120,12 @@ public class ExposureWriterImpl implements ExposureWriter, FeatureFlagEventWrite @Override public void init() { - lifecycleLock.writeLock().lock(); - try { + synchronized (lifecycleLock) { if (closed.get()) { return; } FeatureFlaggingGateway.addExposureListener(this); this.serializerThread.start(); - } finally { - lifecycleLock.writeLock().unlock(); } } @@ -125,17 +138,14 @@ public void close() { @Override public void requestShutdown() { - lifecycleLock.writeLock().lock(); - try { + synchronized (lifecycleLock) { if (!closed.compareAndSet(false, true)) { return; } - // Exclude all producers before asking the single consumer to perform its final drain. + // Stop accepting new dispatches without waiting for producers already inside accept(). FeatureFlaggingGateway.removeExposureListener(this); // Polling wakes within 100 ms. An interrupt can race with, or abort, the final POST. serializer.requestShutdown(); - } finally { - lifecycleLock.writeLock().unlock(); } } @@ -146,16 +156,11 @@ public void awaitTermination(final long deadlineNanos) { @Override public void accept(final ExposureEvent event) { - // Multiple producers may still enqueue concurrently under the read lock. close() takes the - // write lock, so every accepted event is in the queue before the worker's final drain begins, - // while stale CopyOnWriteArrayList dispatch snapshots are rejected after shutdown starts. - lifecycleLock.readLock().lock(); - try { - if (!closed.get()) { - queue.offer(event); - } - } finally { - lifecycleLock.readLock().unlock(); + // Keep producers lock-free, like flag evaluations. Shutdown is best effort: an offer that + // passes this check can race past the final drain and be dropped. Stale gateway dispatches + // entering after shutdown starts are rejected without extending the shutdown wait. + if (!closed.get()) { + queue.offer(event); } } diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index 35034cc06f9..ca6d5256268 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -7,13 +7,16 @@ import static java.util.Collections.singletonMap; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; 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.assertTrue; +import static org.mockito.AdditionalAnswers.delegatesTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -22,6 +25,8 @@ import com.squareup.moshi.JsonAdapter; import com.squareup.moshi.Moshi; +import datadog.common.queue.MessagePassingBlockingQueue; +import datadog.common.queue.Queues; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; import datadog.communication.IntakeApi; @@ -342,7 +347,7 @@ void testFinalFlushRunsWithoutTheInterruptFlagSet() throws Exception { .post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false)); assertFalse( interruptedDuringPost.get(), - "The final HTTP request must not inherit the interrupt used to wake queue polling"); + "The final HTTP request must not run on an interrupted thread"); assertFalse(writer.isSerializerThreadAlive()); } @@ -384,6 +389,53 @@ void testCloseWaitIsBoundedWhenFinalPostDoesNotReturn() throws Exception { poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); } + @Test + @SuppressWarnings("unchecked") + void testCloseDoesNotWaitForProducerAlreadyInsideAccept() throws Exception { + CountDownLatch offerStarted = new CountDownLatch(1); + CountDownLatch releaseOffer = new CountDownLatch(1); + MessagePassingBlockingQueue delegate = Queues.mpscBlockingConsumerArrayQueue(16); + MessagePassingBlockingQueue queue = + mock(MessagePassingBlockingQueue.class, delegatesTo(delegate)); + doAnswer( + invocation -> { + offerStarted.countDown(); + releaseOffer.await(); + return delegate.offer(invocation.getArgument(0)); + }) + .when(queue) + .offer(any(ExposureEvent.class)); + ExposureWriterImpl writer = + new ExposureWriterImpl( + queue, + Long.MAX_VALUE, + NANOSECONDS, + () -> mock(BackendApi.class), + mockConfig("paused-producer-service"), + ExposureWriterImpl.SHUTDOWN_TIMEOUT_MILLIS); + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + writer.init(); + Future producer = executor.submit(() -> writer.accept(buildExposure())); + assertTrue(offerStarted.await(5, SECONDS)); + + // The producer has passed the closed check but cannot finish its offer. Shutdown must not + // wait for that producer, even though its event can miss the final best-effort drain. + Future closing = executor.submit(writer::close); + closing.get(2, SECONDS); + assertFalse(producer.isDone()); + assertFalse(writer.isSerializerThreadAlive()); + + releaseOffer.countDown(); + producer.get(5, SECONDS); + } finally { + releaseOffer.countDown(); + executor.shutdownNow(); + assertTrue(executor.awaitTermination(5, SECONDS)); + writer.close(); + } + } + @Test void testStaleGatewayDispatchCannotEnqueueAfterClose() throws Exception { CountDownLatch dispatchSnapshotTaken = new CountDownLatch(1); From d481d200467206d6f7e76b13eb321eed5ad8e0c6 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Tue, 13 Oct 2026 02:16:43 +0000 Subject: [PATCH 8/8] Cover bounded feature flag writer shutdown paths Exercise deadline expiry, caller interruption, in-flight delivery, and worker startup failure without weakening coverage thresholds. Environment: Datadog workspace --- .../FeatureFlagWriterShutdownTest.java | 72 ++++++++++++++++--- .../FlagEvaluationWriterImplTest.java | 39 +++++++++- 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java index 68153bd86ac..9d9b97185b2 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java @@ -5,7 +5,9 @@ import static java.util.Collections.emptyMap; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -19,16 +21,67 @@ import datadog.trace.api.featureflag.exposure.Flag; import datadog.trace.api.featureflag.exposure.Subject; import datadog.trace.api.featureflag.exposure.Variant; +import java.time.Duration; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.FutureTask; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; class FeatureFlagWriterShutdownTest { + @Test + void awaitThreadDoesNotJoinItself() { + assertTimeoutPreemptively( + Duration.ofSeconds(2), + () -> + FeatureFlagEventWriter.awaitThread( + Thread.currentThread(), System.nanoTime() + SECONDS.toNanos(30))); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void awaitThreadStopsWaitingWithoutInterruptingWorker(boolean interruptedCaller) + throws Exception { + final CountDownLatch releaseWorker = new CountDownLatch(1); + final FutureTask task = + new FutureTask<>( + () -> { + releaseWorker.await(); + return Thread.currentThread().isInterrupted(); + }); + final Thread worker = new Thread(task); + worker.start(); + try { + assertTimeoutPreemptively( + Duration.ofSeconds(2), + () -> { + try { + if (interruptedCaller) { + Thread.currentThread().interrupt(); + } + final long deadline = + System.nanoTime() + (interruptedCaller ? SECONDS.toNanos(30) : -1); + FeatureFlagEventWriter.awaitThread(worker, deadline); + assertEquals(interruptedCaller, Thread.currentThread().isInterrupted()); + } finally { + Thread.interrupted(); + } + }); + assertTrue(worker.isAlive(), "Stopping the wait must not terminate the worker"); + } finally { + releaseWorker.countDown(); + worker.join(5000); + } + assertFalse(worker.isAlive()); + assertFalse(task.get(5, SECONDS), "Waiting must not interrupt the worker"); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void closeDoesNotInterruptAnInFlightDelivery(boolean exposures) throws Exception { final CountDownLatch postStarted = new CountDownLatch(1); final CountDownLatch releasePost = new CountDownLatch(1); + final CountDownLatch postFinished = new CountDownLatch(1); final CountDownLatch interrupted = new CountDownLatch(1); final BackendApi api = mock(BackendApi.class); when(api.post(any(), any(), any(), any(), anyBoolean())) @@ -39,6 +92,8 @@ void closeDoesNotInterruptAnInFlightDelivery(boolean exposures) throws Exception assertTrue(releasePost.await(5, SECONDS)); } catch (InterruptedException failure) { interrupted.countDown(); + } finally { + postFinished.countDown(); } return null; }); @@ -63,19 +118,20 @@ void closeDoesNotInterruptAnInFlightDelivery(boolean exposures) throws Exception evaluationWriter.startForTest(); evaluationWriter.enqueue(simpleEvent("flag", "on")); } - final Thread closer = - new Thread( + final FutureTask close = + new FutureTask<>( () -> { - try { - writer.close(); - } catch (Exception failure) { - throw new AssertionError(failure); - } + writer.close(); + return null; }); + final Thread closer = new Thread(close); try { assertTrue(postStarted.await(5, SECONDS)); closer.start(); - assertFalse(interrupted.await(200, MILLISECONDS), "Shutdown interrupted the active POST"); + close.get(5, SECONDS); + assertEquals(1, releasePost.getCount(), "Shutdown must finish before delivery is released"); + assertEquals(1, postFinished.getCount(), "The active delivery must still be pending"); + assertEquals(1, interrupted.getCount(), "Shutdown interrupted the active POST"); } finally { releasePost.countDown(); closer.join(5000); 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 c442747fea9..f0bd4476dcf 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 @@ -53,9 +53,11 @@ import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; import okhttp3.RequestBody; @@ -160,6 +162,37 @@ void startRegistersWriterAndCloseDeregistersIt() { assertNull(FeatureFlaggingGateway.getFlagEvalWriter()); } + @Test + void unavailableBackendClosesWriterFromItsOwnWorker() throws Exception { + final AtomicReference worker = new AtomicReference<>(); + final CompletableFuture failure = new CompletableFuture<>(); + final FlagEvaluationWriterImpl writer = + new FlagEvaluationWriterImpl( + 16, + Long.MAX_VALUE, + TimeUnit.NANOSECONDS, + () -> { + worker.set(Thread.currentThread()); + worker.get().setUncaughtExceptionHandler((thread, error) -> failure.complete(error)); + return null; + }, + cfg()); + try { + writer.start(); + final Throwable error = failure.get(5, TimeUnit.SECONDS); + assertEquals(IllegalArgumentException.class, error.getClass()); + assertEquals("EVP Proxy not available", error.getMessage()); + worker.get().join(5000); + assertFalse(worker.get().isAlive()); + assertNull(FeatureFlaggingGateway.getFlagEvalWriter()); + + writer.startForTest(); + assertEquals(Thread.State.TERMINATED, worker.get().getState()); + } finally { + writer.close(); + } + } + @Test void queueOverflowIncrementsObservableDropCounter() { final BackendApi mockEvp = mock(BackendApi.class); @@ -412,9 +445,9 @@ void payloadLimitDropsAreCountedOnFlush() { @Test void finalFlushRunsWithoutTheInterruptFlagSet() throws Exception { - // close() interrupts the worker to break it out of poll(). The final flush does socket I/O, - // and OkHttp fails fast on an interrupted thread, so the flag must be clear by the time the - // publisher is called. A mock publisher ignores the flag, so assert on it directly. + // close() signals shutdown without interrupting the worker. The final flush does socket I/O, + // and OkHttp fails fast on an interrupted thread. A mock publisher ignores the flag, so assert + // on it directly. final java.util.concurrent.CountDownLatch posted = new java.util.concurrent.CountDownLatch(1); final boolean[] interruptedDuringPost = {true}; final BackendApi mockEvp = mock(BackendApi.class);