Repository navigation
Send Java SDK identity with Feature Flagging EVP events - #12575
leoromanovsky wants to merge 5 commits into
Conversation
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
leoromanovsky
added
type: feature
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Java SDK identity headers reach both local EVP and direct-intake requests while preserving credential isolation, send-once settings, and redirect behavior. The completed static review identified no actionable regression.
🤖 Bits Code Review · Commit 96a3427 · @DataDog review to ask questions
| asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); | ||
| } | ||
|
|
||
| private static Map<String, String> requestHeaders() { |
There was a problem hiding this comment.
🟠 Non-blocking suggestion — test coverage
Delete the two headers.put calls in this method and both test suites stay green. This method builds the payload the PR is named after, and no test pins its contents.
The transport is covered; the contents are not. BackendApiFactoryTest supplies its own header map, so it proves the interceptor works. Every test in FeatureFlagBackendApiFactoryTest passes mock(BackendApiFactory.class), so the argument at line 49 is never read.
Evidence — mutation results (executed)
| # | Mutation | :communication:test |
:products:…:feature-flagging-lib:test |
|---|---|---|---|
| M1 | both headers.put calls behind a never-true guard |
235/235 ✅ | 279/279 ✅ |
| M2 — positive control | configureHttpClient skips addInterceptor |
3 ❌ | 279/279 ✅ |
M2 proves the harness can go red. It just does not watch this method.
Evidence — without a fixture, the version assertion misses a hardcoded "0.0.0"
TracerVersion.TRACER_VERSION reads dd-java-agent.version from the system classloader and falls back to "0.0.0" (TracerVersion.java:11). I printed the value inside the feature-flagging-lib test JVM:
TRACER_VERSION=[0.0.0]
So assertEquals(TracerVersion.TRACER_VERSION, …) compares "0.0.0" with "0.0.0". The assertion still detects a missing or otherwise wrong version header; what it cannot do is tell the version loader's fallback apart from a hardcoded "0.0.0". The origin and map-size assertions are unaffected.
Adding src/test/resources/dd-java-agent.version fixes it. Executed results:
| Tree state | ff-lib suite |
|---|---|
| Fix + fixture | 280/280 ✅ |
Fix + fixture, version source replaced by literal "0.0.0" |
1 ❌ |
| Fix + fixture, M1 re-applied | 1 ❌ |
One local caveat: build.gradle.kts:77-82 sets normalization { runtimeClasspath { ignore("**/*.version") } }. A change to the fixture alone does not invalidate the test task cache key. Use --no-build-cache --rerun-tasks while you iterate — that is how I ran all three rows above.
Scope — what this test does and does not cover
It covers the contents of the map. It does not cover the wiring: the test still passes if line 49 stops passing REQUEST_HEADERS to the constructor. Closing that needs a real BackendApiFactory, which this test class does not use.
A smoke test can close it. OpenFeatureProviderSmokeTest runs DD_FEATURE_FLAGS_CONFIGURATION_SOURCE=remote_config (line 46), which takes the fixed EVP v2 route and so bypasses the capability gate, and decodedEvpProxyMessageCallback() (line 56) already receives the RequestApi. RequestApi.getHeader(String) exists (TestHttpServer.groovy:450). Capturing request.getHeader('DD-EVP-ORIGIN') and asserting it in 'test open feature exposures' would cover the wiring end to end. I did not write that test.
With the current AbstractSmokeTest /info response, agentless mode rejects the discovered local proxy: AbstractSmokeTest.groovy:94-100 advertises only Content-Type, Accept-Encoding, Content-Encoding, User-Agent, DD-CI-PROVIDER-NAME, so the capability gate fails. Exercising that proxy route would need the mock advertisement updated. Direct intake is a separate path and is not blocked by this.
Why coverage metrics will not flag this
The static initializer at line 28 runs requestHeaders() whenever the class loads, so line coverage reports the method as covered although no assertion reads the values. Separately, datadog.communication.BackendApiFactory sits in excludedClassesCoverage (communication/build.gradle.kts:53).
Suggested change — compiled and run; kills M1.
FeatureFlagBackendApiFactory.java:112 — widen the method:
static Map<String, String> requestHeaders() {New file products/feature-flagging/feature-flagging-lib/src/test/resources/dd-java-agent.version:
TEST-SNAPSHOT~test
FeatureFlagBackendApiFactoryTest.java — add four imports (static org.junit.jupiter.api.Assertions.assertEquals, static org.junit.jupiter.api.Assertions.assertNotEquals, datadog.communication.ddagent.TracerVersion, java.util.Map) and the test:
@Test
void requestHeadersIdentifyTheJavaTracerAndItsVersion() {
final Map<String, String> headers = FeatureFlagBackendApiFactory.requestHeaders();
assertEquals("dd-trace-java", headers.get("DD-EVP-ORIGIN"));
assertEquals(TracerVersion.TRACER_VERSION, headers.get("DD-EVP-ORIGIN-VERSION"));
// Guards the premise: without src/test/resources/dd-java-agent.version the line above
// compares the "0.0.0" fallback with itself.
assertNotEquals("0.0.0", headers.get("DD-EVP-ORIGIN-VERSION"));
assertEquals(2, headers.size());
}The header name and the origin value stay raw literals on purpose. They pin the wire format from outside the constants.
There was a problem hiding this comment.
Partially addressed in 1ac2d95866: the real-factory socket test now asserts both headers and exercises constructor wiring. The non-fallback version fixture is still missing, so I am leaving this coverage request open.
| this(config, sharedCommunicationObjects, emptyMap()); | ||
| } | ||
|
|
||
| public BackendApiFactory( |
There was a problem hiding this comment.
🟡 Non-blocking cleanup — dead API surface
Two of the four public constructors have no independent caller outside tests. Delete both 3-argument overloads now, while the API is new and nothing in this repository depends on its shape.
The one worth removing on its own merits is (Config, SCO, Map). It implicitly selects sendOnce = false. The default explicit HTTP policy then permits up to five retries — new HttpRetryPolicy.Factory(5, 100, 2.0, true) at line 298, whose first parameter is maxRetries, allowing up to six explicit attempts before accounting for retries internal to OkHttp — while the supplied OkHttp client's own retryOnConnectionFailure setting is left as it is. EVP callers can override the explicit policy, including with NEVER_RETRY. When retries are enabled, a payload with no idempotency key can produce duplicate events after an ambiguous failure, for example when the intake processes the request but the response times out. Nothing in the signature warns about it, and the Javadoc that explains sendOnce sits on the 4-argument form only.
Evidence — caller census (repo-wide grep)
| Overload | Independent callers outside tests |
|---|---|
(Config, SCO) |
LogsIntakeSystem:22, CiVisibilityServices:84,85, 2 JMH benchmarks |
(Config, SCO, Map) |
none — new in this PR; reached only through the 2-argument form and BackendApiFactoryTest:389 |
(Config, SCO, boolean) |
none — this PR orphaned it; serves only BackendApiFactoryTest:323, :512 |
(Config, SCO, Map, boolean) |
FeatureFlagBackendApiFactory:49 |
Two honest caveats. The JMH benchmarks are not production code, they just live outside src/test. And the map overload is not dead execution — the 2-argument constructor delegates to it today. The proposed change re-points that delegation at the 4-argument form.
Evidence — the deletion compiles and passes (executed)
I applied the change in a scratch worktree, updated the three test call sites, and added import static java.util.Collections.emptyMap; to the test file:
:communication:test— 235/235 ✅:products:feature-flagging:feature-flagging-lib:test— 279/279 ✅
:dd-java-agent:agent-ci-visibility:compileJava could not build in my offline worktree. It fails identically on the clean tree (unresolvable scala-library / kotlin-stdlib), and it uses only the retained 2-argument form.
Scope note. This removes a footgun from a new API; it does not fix a live bug. The only production caller that needs sendOnce already passes true explicitly (FeatureFlagBackendApiFactory:49), and the retained 2-argument form still defaults to false for the existing CI Visibility and Logs Intake callers, which is correct for them.
Suggested change — delete lines 37-47 and collapse the 2-argument form:
public BackendApiFactory(Config config, SharedCommunicationObjects sharedCommunicationObjects) {
this(config, sharedCommunicationObjects, emptyMap(), false);
}Then make the three test call sites explicit, keeping each one's existing arguments:
// BackendApiFactoryTest:322-323 — was (Config, SCO, boolean)
new BackendApiFactory(
Config.get(), sharedCommunicationObjects(discovery, agent.url("/")), emptyMap(), true);
// BackendApiFactoryTest:512 — was (Config, SCO, boolean)
new BackendApiFactory(
Config.get(), sharedCommunicationObjects(discovery, null), emptyMap(), true);
// BackendApiFactoryTest:389-392 — was (Config, SCO, Map)
new BackendApiFactory(
config,
sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null),
singletonMap(ORIGIN_HEADER, JAVA_TRACING_LIBRARY),
false);
| final RecordedRequest request = intake.takeRequest(); | ||
| assertEquals(JAVA_TRACING_LIBRARY, request.getHeader(ORIGIN_HEADER)); | ||
| assertEquals("api-key", request.getHeader("DD-API-KEY")); | ||
| } finally { |
There was a problem hiding this comment.
⚪ Nit — test hygiene
This test leaves one idle socket open to a dead port. The sibling test 40 lines down evicts its pool; this one does not. One line fixes it.
Because requestHeaders is non-empty and followRedirects=false, createDirectIntakeApi derives a fresh OkHttpClient, which opens a connection that stays in the pool. The test shuts down only the MockWebServer.
Evidence — measured effect (executed)
P5 derived client == parent = false
P5 shares parent dispatcher = true
P5 shares parent connection pool = true
P5 idle connections left in pool = 1
P5 idle connections after server shutdown = 1
P5 non-daemon OkHttp threads = none
P5 dispatcher runningCalls = 0
So: one idle socket to a dead port, held for the OkHttp keep-alive window. No non-daemon thread, so it cannot block JVM exit, and no leaked call. This is hygiene, not a leak.
Two things the obvious fix gets wrong
The dispatcher line is a no-op here. Copying the sibling's teardown verbatim would add dispatcher().executorService().shutdownNow(). getIntakeHttpClient() builds its client with new Dispatcher(RejectingExecutorService.INSTANCE) (OkHttpUtils.java:178), and that executor's shutdownNow() returns an empty list and does nothing (RejectingExecutorService.java:23). Only evictAll() does work on this client.
Worth knowing: the sibling's own dispatcher line buys little either. Its request goes through the synchronous newCall(request).execute() path (OkHttpUtils.java:428), and in the pinned OkHttp 3.12.15 Dispatcher.executorService() builds its ThreadPoolExecutor lazily on first call — I checked the bytecode. So the teardown allocates a pool that never ran a task, then shuts it down. Do not copy it here on the theory that it is doing cleanup there.
evictAll() does not stop the pool cleanup daemon thread. OkHttp documents that it can keep running after eviction. The socket closes; the thread may stay TIMED_WAITING until it times out. That is expected and harmless.
The derivation is not what allocates the pool. The derived client shares the parent's pool by reference, so the same idle socket would exist if the test used the parent directly. The reason to fix it is consistency with the sibling, not a cost introduced by this PR.
Suggested change — compiled and run; :communication:test 235/235 ✅.
Hoist the SharedCommunicationObjects out of the try so the finally can reach the parent client:
final SharedCommunicationObjects sco =
sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null);
try {
// …
final BackendApiFactory factory =
new BackendApiFactory(config, sco, singletonMap(ORIGIN_HEADER, JAVA_TRACING_LIBRARY));
// …
} finally {
// The derived client shares the parent connection pool by reference, so evicting the
// parent pool closes the idle socket this test opened.
sco.getIntakeHttpClient().connectionPool().evictAll();
intake.shutdown();
}
| * requiring the Agent to advertise every specified forwarding header. | ||
| * | ||
| * <p>Callers supply the forwarding capabilities their event protocol requires. | ||
| * <p>Required forwarding capabilities are independent of any headers configured on this factory. |
There was a problem hiding this comment.
⚪ Nit — documentation preference, strictly optional
The replaced sentence said who decides what to pass. The replacement says what does not feed the gate. Both are useful; one line holds both.
- Before:
Callers supply the forwarding capabilities their event protocol requires. - After:
Required forwarding capabilities are independent of any headers configured on this factory.
The new sentence answers the question this PR creates — do the configured request headers feed the gate? They do not, and that is worth stating. To be fair to the edit, the retained summary at line 204 already describes the mechanism ("requiring the Agent to advertise every specified forwarding header"), so nothing essential was lost. What went is the hint that the caller's event protocol is what determines the list.
Suggested change:
/**
* Creates an EVP proxy client after Agent discovery, optionally forcing a fresh discovery and
* requiring the Agent to advertise every specified forwarding header.
*
* <p>Callers supply the forwarding capabilities their event protocol requires. These are
* independent of any headers configured on this factory.
*
* <p>The {@code forceDiscovery} form is intended for bounded unavailable-route recovery probes.
*/
| this( | ||
| config, | ||
| new BackendApiFactory(config, sharedCommunicationObjects, true), | ||
| new BackendApiFactory(config, sharedCommunicationObjects, REQUEST_HEADERS, true), |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Please add a test that sends an exposure through the real writer and checks both identity headers on the wire. Today no test protects the identity headers that this line passes.
If this line passes an empty map instead of REQUEST_HEADERS, the communication and feature-flagging-lib tests still pass (235/235 and 279/279). Production then sends no identity.
The open thread at line 112 suggests a smoke test for this wiring. A unit test can do it. ExposureWriterTests already builds ExposureWriterImpl with a real SharedCommunicationObjects. Its requests go through this line, a real BackendApiFactory, and the fixed EVP v2 route to the test server. No test reads their headers.
1. Measured: three mutants pass the current tests, and the test below fails on each
| Tree | communication | feature-flagging-lib |
|---|---|---|
| this line passes an empty map | 235/235 | 279/279 |
requestHeaders() builds an empty map |
235/235 | 279/279 |
createEvpProxyApiForEndpoint builds its client from the shared agentHttpClient |
235/235 | 279/279 |
positive control: configureHttpClient never adds the interceptor |
232/235 | 279/279 |
| PR head + test below | 235/235 | 280/280 |
| test below + each of the first three mutants | 235/235 | 279/280, the new test fails |
The third mutant matters more after you restack onto #12477's head 4218559309. There, every Feature Flagging proxy send uses createEvpProxyApiForEndpoint. The communication tests check the proxy-route headers only through the five-argument createEvpProxyApi.
2. Suggested test: for ExposureWriterTests; it passes spotless and also passes on a local merge onto 4218559309
Add import datadog.communication.ddagent.TracerVersion; and this test:
@Test
void testExposureRequestsCarrySdkIdentity() throws Exception {
// The production constructor builds a real BackendApiFactory, so this covers the identity
// headers from FeatureFlagBackendApiFactory to the wire on the fixed EVP v2 route.
Config config = mockConfig("identity-service");
try (ExposureWriterImpl writer =
new ExposureWriterImpl(1 << 4, 100, MILLISECONDS, sharedCommunicationObjects, config)) {
writer.init();
writer.accept(buildExposure());
poll.eventually(
() -> {
assertNotNull(server.getLastRequest());
assertEquals(EXPOSURES_ENDPOINT, server.getLastRequest().getPath());
assertEquals("dd-trace-java", server.getLastRequest().getHeader("DD-EVP-ORIGIN"));
assertEquals(
TracerVersion.TRACER_VERSION,
server.getLastRequest().getHeader("DD-EVP-ORIGIN-VERSION"));
});
}
}In this test JVM, TracerVersion.TRACER_VERSION is "0.0.0", so the version check cannot tell the loader's fallback from a hardcoded "0.0.0". The dd-java-agent.version test resource suggested in the thread at line 112 closes that gap with a non-fallback value.
There was a problem hiding this comment.
1ac2d95866 adds both-header assertions through the production factory and real socket, including its fixed-endpoint transport. The requested real ExposureWriter assertion and non-fallback version fixture are still missing; leaving this open.
Keep both-header forwarding requirements with identity emission and preserve the validated retry, redirect, and shutdown fixes. Environment: Datadog workspace
This branch has not been deployed
4 participants
Motivation
Attribute Feature Flagging exposures and flag evaluations to the Java SDK on both local EVP proxy and direct-intake routes. Initial direct delivery already shipped; this PR isolates producer identity from the transport hardening in #12477. Tracks FFL-3016 within the FFLSDK-187 rollout.
Changes
DD-EVP-ORIGIN: dd-trace-javaandDD-EVP-ORIGIN-VERSIONusing the tracer version.Decisions
leoromanovsky/fflsdk-187-java-evp-fallback, not another language or unrelated SDK branch.Validation
1b5766b56c67bb7151a084abb752e62e5bb4b332.96a3427bb9bcb640165306d16af141e9455e2cc4../gradlew :communication:spotlessApply :products:feature-flagging:feature-flagging-lib:spotlessApply :communication:test :products:feature-flagging:feature-flagging-lib:test --console=plain --max-workers=4: PASS; communication 235/235, feature-flagging-lib 279/279.git diff --check: PASS. New commit is GitHub verified:verified=true,reason=valid.