Harden Agentless Feature Flags EVP delivery - #12477
leoromanovsky wants to merge 3 commits into
Conversation
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
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. |
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
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-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please try again by commenting @autotest review.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
🤖 Generated with Claude Code
| if (selectedRoute == FeatureFlagRouteSelector.Route.LOCAL) { | ||
| BackendApi selectedProxyApi = proxyApi; | ||
| if (selectedProxyApi == null) { | ||
| selectedProxyApi = discoverProxyApi(); |
There was a problem hiding this comment.
When the shared route is LOCAL but this writer's own proxyApi is unavailable, every post() forces a fresh, unthrottled Agent /info discovery instead of being rate-limited like the UNAVAILABLE-route recovery path. FeatureFlagRouteSelector is shared process-wide across the exposure and flag-evaluation writers, so if one writer transitions the shared route to LOCAL but this writer's own proxyApi stays null (e.g. createProxyApi(false) never satisfied supportsEvpProxyHeaders), selectApi() calls discoverProxyApi() -> createProxyApi(true) -> DDAgentFeaturesDiscovery.discover() (a real HTTP /info round-trip) on every single post(), with no cooldown, unlike the throttled Route.UNAVAILABLE path gated by routeSelector.tryBeginLocalRecovery().
🤖 Generated with Claude Code
| final BackendApi directApi = getOrCreateDirectApi(); | ||
| if (directApi == null) { | ||
| final BackendApi fallbackApi = getOrCreateDirectApi(); | ||
| routeSelector.localFailure(fallbackApi != null); |
There was a problem hiding this comment.
Any IOException on the local proxy route (including ambiguous/transient ones like HTTP 429/500 or a timeout) permanently flips the shared route away from LOCAL, and FeatureFlagRouteSelector has no transition back from DIRECT to LOCAL. post() calls routeSelector.localFailure(fallbackApi != null) unconditionally whenever the local call throws IOException, regardless of isSafeToReplayDirectly(exception). A single transient 500 from the local Agent's EVP proxy switches the shared route to DIRECT for the remainder of the process — both exposures and flag-evaluation writers bypass the local Agent from then on even after the Agent recovers, since Route.DIRECT is terminal. Note: this is explicitly test-covered (keepsDirectRouteStickyAfterLocalFailure), so it reads as intentional design rather than an oversight — flagging it as a reliability trade-off worth an explicit maintainer sign-off.
🤖 Generated with Claude Code
| && routeSelector.tryBeginLocalRecovery()) { | ||
| final BackendApi recoveredProxyApi = discoverProxyApi(); | ||
| if (recoveredProxyApi != null) { | ||
| proxyApi = recoveredProxyApi; |
There was a problem hiding this comment.
Unsynchronized read-then-write of the volatile proxyApi field lets concurrent callers race to discover/store the local proxy client. Two threads calling post() concurrently while proxyApi is null and the shared route is UNAVAILABLE-recovering or LOCAL will both observe proxyApi == null, both invoke discoverProxyApi() (unlike getOrCreateDirectApi(), which is synchronized and guarded by directApiCreationAttempted), each creating a redundant Agent-discovery client, with the later write winning non-deterministically.
🤖 Generated with Claude Code
| } | ||
|
|
||
| @Override | ||
| public boolean isApplicable(Set<TargetSystem> enabledSystems) { |
There was a problem hiding this comment.
isApplicable() is hard-coded to always return true, so this bootstrap instrumentation of java.lang.Shutdown now applies even when no product (tracing, feature flagging, or otherwise) is enabled. Previously this extended InstrumenterModule.Tracing, gated on TargetSystem.TRACING. Since there's no TargetSystem entry for Feature Flagging, every JVM the agent attaches to now gets java.lang.Shutdown instrumented with ByteBuddy advice even when dd.trace.enabled=false and feature flagging is never used — a blanket "always applicable" special case on a shared, sensitive ForBootstrap instrumentation rather than a general enabled-system check.
🤖 Generated with Claude Code
| this.serializerThread.join(shutdownTimeoutMillis); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
In agentless mode, the writer now always starts and keeps its serializer thread alive forever (waking every 100ms) even when no delivery route (local proxy or direct credentials) will ever become available. Pre-PR, FeatureFlagBackendApiFactory.create() returned null when neither route was available, and the writer thread exited immediately. Post-PR, create() always wraps the routes in an AgentlessFeatureFlagBackendApi, so a customer with agentless feature flags enabled but no API key and no compatible local Agent gets a permanently-idle thread polling queue.poll(100ms) for the life of the JVM instead of the thread terminating.
🤖 Generated with Claude Code
| return discoveryState.telemetryProxyEndpoint != null; | ||
| } | ||
|
|
||
| private static HttpUrl appendPath(final HttpUrl baseUrl, final String path) { |
There was a problem hiding this comment.
appendPath(HttpUrl, String) is duplicated verbatim in both this file and BackendApiFactory.java (line ~244). Both classes independently strip leading slashes and call addPathSegments identically. A future fix to path-joining semantics (e.g. handling encoded slashes, empty segments, or trailing-slash edge cases) applied to one copy but not the other will silently reintroduce the agent-base-path bug this PR fixes only where the fix is remembered. Consider extracting to a shared helper.
🤖 Generated with Claude Code
This branch has not been deployed
2 participants
Motivation
Java already delivers agentless exposures (#12195) and flag evaluations (#12204) directly to EVP. This follow-up closes reliability gaps found while checking the cross-SDK contract: unsafe replay, inconsistent writer routes, lost Agent URL prefixes and permanent disablement when no route exists at startup. Tracks FFLSDK-187.
Changes
Decisions
Validation
3110972ccf744f304a7cf59973e240f9ccd1a071.1b5766b56c67bb7151a084abb752e62e5bb4b332../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 234/234, feature-flagging-lib 279/279.git diff --check: PASS. New scope-reduction commit is GitHub verified:verified=true,reason=valid.e20a9d77c333ddb0c0444835f23fafba70952373and must not be carried forward as current proof.