Repository navigation
Conversation
Gate configuration negotiation on evaluation and exposure support; the current evaluator remains opted out. Environment: Datadog workspace Co-Authored-By: Codex GPT-6 <noreply@localhost>
hhan2024
added
tag: ai generated
ChatGPT Codex Connector
🟢 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. |
aarsilv
added
the
crucible: reviewing
| if (supported == this.dependentFlagsAdvertised) { | ||
| return; | ||
| } | ||
| this.dependentFlagsAdvertised = supported; |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
Heads up: This becomes a bug when EvaluatorCapabilities.supported() is turned on. Please fix it before that change lands.
When the advertised state changes, updateDependentFlagCapability clears current and then only adds or removes the capability bit. It does not request the configuration again, although the PR description says that subscription changes "request configuration again".
Scenario, assuming the Remote Config payload does not change when bit 50 is added (for example, an org without dependent flags), so the poller does not call accept again:
- The first supported subscriber registers, receives the cached document, and the flip to "advertised" clears
current. - Later, that subscription closes (for example, the last provider shuts down and the runtime releases its subscription). The flip back clears
currentagain. - A new runtime subscribes. It gets no replay, so its provider waits for a document until its initialization timeout. An unsupported subscriber that joins while the bit is advertised waits in the same way.
Fix: after the capability bit changes, call requestAgain() outside the subscribers lock, so the poller registers the product again and the next poll delivers the document with the new bits. Add a test for the sequence above that asserts the listener is registered again (or simulates the next poll and its accept) before asserting that the new subscriber receives the document.
I found this by reading the code; I did not run it.
|
|
||
| private boolean started; | ||
| private final Map<Consumer<byte[]>, Boolean> dependentFlagSubscribers = new IdentityHashMap<>(); | ||
| private boolean dependentFlagsAdvertised; |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Possible causes of the red spotless and test_base jobs (from reading the code; I did not run Gradle or read the job logs):
spotless: the@TableTestrows inCdnConfigurationSourceTestwere split into"…" + "…". The repository's Spotless chain runsgoogleJavaFormatand thentableTestFormatter, which formats those tables itself. Running./gradlew spotlessApplyand committing the result should settle it.test_base: this job runs with-PcheckCoverage. The new default method onFeatureFlaggingGateway.Backendand the two-argumentFeatureFlagsBackend.subscribeRemoteConfighave no tests. A test that calls each and checks that it delegates would cover them.
|
|
||
| /** Keep empty until prerequisite evaluation, errors, and exposures are integration-tested. */ | ||
| public static Set<String> supported() { | ||
| return emptySet(); |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Coordination note with #13166: nothing turns this report on, and #13166 sends the header by itself.
- This method returns
emptySet(), so this PR sends neither the header nor bit 50. - feat: Dependent Flags #13166 implements the evaluator, but it adds the header unconditionally in
CdnConfigurationSourceand never sets bit 50. - A three-way merge of the two PRs' versions of
CdnConfigurationSource.javaand its test has no textual conflict, but the merged test asserts both that the header is present and that it is absent.
Suggested order: land this PR first. Then #13166 deletes its own header and constants, and this method returns singleton(DEPENDENT_FLAG_VARIANT_KEY_V1) (with import static java.util.Collections.singleton;), with EvaluatorCapabilitiesTest and the header assertions updated. Before that change, RemoteConfigServiceImpl must request the configuration again when the capability bit changes. This is a sketch; I did not build it.
| if (EvaluatorCapabilities.supported() | ||
| .contains(EvaluatorCapabilities.DEPENDENT_FLAG_VARIANT_KEY_V1)) { | ||
| request.header( | ||
| "X-DD-FEATURE-FLAGS-CAPABILITIES", EvaluatorCapabilities.DEPENDENT_FLAG_VARIANT_KEY_V1); |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
Heads up: when the evaluator report is turned on, this gate sends X-DD-FEATURE-FLAGS-CAPABILITIES on direct UFC requests (the report is empty today). The corresponding capability-gated code in DataDog/dd-trace-go#5800 and DataDog/dd-trace-js#11038 now uses the query parameter cap_dependent-flags-variant-key=v1 on direct UFC requests instead of this header (their capability reports are currently empty; DataDog/dd-trace-js#11083 states the reason: keep the capabilities header for client-to-edge negotiation, and negotiate the direct UFC response shape through the cache-keyed URL). If that is the agreed protocol, please switch this path to the same query parameter, so all SDKs negotiate the same way.
What Does This Do
Prepare agentless capability headers and Remote Config bit 50 for
dependent-flag-variant-key-v1, gated on the SDK evaluator's report. The report remains empty, preserving current delivery until prerequisite evaluation, error propagation, and exposure reporting are implemented and integration-tested.Motivation
Follows Python agentless #21335 and Python RC #21336. The separately shipped Java SDK must declare its support to the agent; the agent's protocol constants cannot establish evaluator support.
Additional Notes
The bridge adds a default overload so older agents remain conservative. RC opt-in requires every live subscriber to report support; subscription changes remove stale replay state and request configuration again, and capability updates avoid holding the subscriber lock while acquiring the poller lock.
Added empty-report, unknown-capability, mixed-subscriber, unsubscribe, and current initial/conditional-header coverage. Formatting with pinned google-java-format 1.36.1, limited Java 11 compilation of the new reporter/SDK bridge/protocol constants, and diff checks pass; full Gradle tests/Spotless failed during setup due to exhausted workspace storage. Keep draft pending full SDK, agent-bridge/muzzle, lifecycle/concurrency, and transport validation.
Contributor Checklist