feat(feature-flagging): FFE APM feature-flag span enrichment (experimental, gated)#11658
Conversation
…nrichment gate - Add nullable Integer serialId to UFC Split model + constructor (Moshi reflectively deserializes the serialId JSON field; no custom adapter needed) - DDEvaluator surfaces __dd_split_serial_id (when present) and __dd_do_log in OpenFeature eval metadata for APM span enrichment (JAVA-01) - Add FeatureFlaggingConfig.SPAN_ENRICHMENT_ENABLED dot-form constant mapping to DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED (distinct, off by default) - Register the env var in metadata/supported-configurations.json (version A, boolean) - Update DDEvaluatorTest Split call sites for the new constructor arg
…ceptor (gate-gated)
- ULeb128Encoder: ULEB128 delta-varint + base64 codec ported verbatim from the
frozen Node reference (dd-trace-js#8343); golden vector {100,108,128,130} -> ZAgUAg==,
SHA-256 hex targeting-key hashing; JDK stdlib only (java.util.Base64, MessageDigest)
- SpanEnrichmentAccumulator: per-trace state keyed by trace id in a shared
ConcurrentHashMap; frozen limits (200/10/20/5/64), UTF-8-safe truncation, object
default -> JSON (Pattern F tag shapes: ffe_flags_enc bare base64, ffe_subjects_enc /
ffe_runtime_defaults JSON objects)
- SpanEnrichmentHook: OpenFeature finally hook (capture) resolving the active local
root via AgentTracer and applying the Node branch (serialId -> addSerialId/addSubject;
missing variant -> addDefault); error-isolated
- SpanEnrichmentInterceptor: TraceInterceptor (write) flushing ffe_* onto the local
root onTraceComplete with unique priority 4, then clearing state; disable() for
provider-close cleanup
- Provider: gate-gated construction via ConfigHelper.env (DG-005 — nothing built/
registered when off), hook added to getProviderHooks, interceptor registered via
GlobalTracer, shutdown() disables interceptor + drains state
- feature-flagging-api: add compileOnly/testImplementation :internal-api for the
tracer/interceptor types (runtime-provided by the agent)
… golden round-trip)
- Codec golden-vector round-trip {100,108,128,130} -> ZAgUAg== (encode + decode +
empty + structural dedupe)
- no-span, finished-root (accumulate+dedupe+flush), runtime-default missing-variant
(ffe_runtime_defaults JSON object), per-subject cap (10 subj / 20 exp/subj) +
doLog gating, max-200 serial ids, object-default JSON + 64-char truncation
- gate-off negative control: no hook/interceptor constructed, not in getProviderHooks,
no accumulator state (DG-005)
- gate-on construction + provider-close cleanup (shutdown disables interceptor +
drains state); error isolation; interceptor priority uniqueness
- JUnit 5 only (no Groovy); injectable RootSpanResolver + Provider gate override
avoid static mocking (project mockito uses the subclass mock maker)
…econfig) Gap-closure for the three BLOCKER lifecycle defects in 02-REVIEW-java.md. The frozen contract (codec/limits/gate/tag shapes, golden vector ZAgUAg==) is unchanged — this fixes only how per-trace state is scoped, bounded, and flushed. CR-01 (partial-flush data loss + tag misattribution): CoreTracer.write -> interceptCompleteTrace runs onTraceComplete on EVERY flush, and PendingTrace.write(isPartial) excludes the still-open local root from a partial flush (getRootSpan() is null until the final write). The interceptor previously resolved the root by reference (reachable even when absent from the fragment) and unconditionally removed the accumulator on the first partial flush, dropping all pre-flush flags and tagging a not-yet-finished root. SpanEnrichmentInterceptor now flushes + removes ONLY when the local root is actually present in the flushed collection (the final write); a partial flush leaves the accumulator intact. Also adds the missing getTraceId() null guard (WR-01) and never falls back to tagging a non-root span (WR-02). CR-02 (unbounded state leak): State lived in a static ConcurrentHashMap whose only removal paths were trace-complete and shutdown, so dropped / Noop / never-finishing traces leaked forever (#4844 leak class). State now lives in a per-provider SpanEnrichmentStates store, hard-bounded at MAX_TRACES with FIFO eviction of the oldest in-flight entry, so a never-completing trace cannot grow the map unboundedly. CR-03 (reconfiguration corruption): Provider ignored addTraceInterceptor's return value, so a second gate-on provider (rejected on duplicate priority 4) still wired a hook into shared static state and its shutdown() cleared the first provider's live state. Provider now honors the registration result (wires the hook/interceptor only when registration succeeds) and each provider owns its own state store, so one provider's shutdown can never clear another's. Removes the global static map (review IN-03 root cause). Adds an injectable TraceInterceptorRegistrar test seam (mirrors the existing gate-override seam). Runtime-default rendering (String() parity) and hasData() are intentionally unchanged. L0 suite migrated to the instance-owned store; module 146 tests green.
Adds SpanEnrichmentLifecycleRegressionTest exercising the real tracer lifecycle
paths the original L0 suite bypassed (single root, no children, one provider).
Each test FAILS against the pre-fix code and PASSES after the fix:
CR-01 partialFlushExcludingRootPreservesPreFlushFlags / partialFlushNeverWrites-
TagsOnAChildSpan: a partial flush carrying only child spans (root excluded, as
dd-trace-core does) must not drain state or tag a child; the full pre+post-flush
serial-id set {100,108,128,130} -> 'ZAgUAg==' must land on the root at final
completion. Fail-before: tags written on the partial flush (NeverWantedButInvoked).
CR-02 neverCompletingTracesDoNotLeakUnbounded / boundedStoreEvictsOldestFirst:
3x MAX_TRACES distinct never-flushed traces keep the store bounded; eviction is
FIFO. Fail-before: store grew to 3x the cap (expected <=cap but was larger).
CR-03 secondProviderRejectedRegistrationDoesNotClearFirstProviderState /
eachProviderOwnsADistinctStateStore / hookAndInterceptorOfOneProviderShareThe-
SameStore: a second provider whose registration is rejected wires no
hook/interceptor and its shutdown does not clear the first provider's in-flight
state. Fail-before: second provider kept a non-null interceptor (expected null).
Fail-before verified by temporarily reintroducing each defect: 5/7 failed (the
remaining 2 are structural-invariant checks that hold by construction).
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. |
…hment comments Remove leaked workflow labels (e.g. JAVA-01) from production comments so the upstream PR carries only behavioral context.
- Reconfiguration safety: replace the per-provider trace interceptor with a single process-wide delegating interceptor (SpanEnrichmentInterceptor.INSTANCE) registered once and rebound per active provider. A closing provider unbinds only if still active, so provider close/reopen no longer permanently disables enrichment via a duplicate-priority rejection, and a displaced provider's late shutdown cannot clobber the active provider's in-flight state. - Runtime defaults: recursively unwrap OpenFeature Value (structure/list/scalar) to its native form before JSON serialization so ffe_runtime_defaults matches the frozen Node contract instead of emitting Value.toString(). - 128-bit trace ids: key per-trace state by the full trace id hex string rather than DDTraceId.toLong(), so two traces sharing low-order 64 bits no longer merge enrichment state. - Gate-off inertness: precompute the immutable provider-hook list once in the constructor so getProviderHooks() (called per evaluation) allocates nothing. - Add regression tests: reconfiguration, displaced-provider late shutdown, 128-bit low-bit collision, Value structure/list/scalar serialization, and gate-off zero-allocation.
…BLED; add Startup INFO log; clean tech debt
Finalization pass (handoff takeover)Picking this up to get it merge-ready. Changes since the last review, all scoped to the existing feature — the frozen wire contract (codec, limits, tag shapes, golden vector
Verification (local):
Files touched in this pass: |
…manovsky/ffe-apm-span-enrichment # Conflicts: # products/feature-flagging/feature-flagging-api/src/test/java/datadog/trace/api/openfeature/DDEvaluatorTest.java
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99422b5c34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return trace; // partial flush, or no resolvable in-fragment root: keep state untouched | ||
| } | ||
| // Key by the local-root span object to match the capture-side keying. | ||
| final SpanEnrichmentAccumulator state = states.remove((AgentSpan) localRoot); |
There was a problem hiding this comment.
When trace.experimental.long-running.enabled is true and a trace survives to a periodic long-running flush, PendingTrace emits unfinished spans including the still-running local root (writeRunningSpans adds running spans to the trace). This path then treats root presence as final and removes the accumulator, so flags evaluated before the first long-running snapshot are no longer present when the trace actually completes and later snapshots can overwrite the root tag with only newer IDs. Please keep the state until the root is finished rather than using presence in the emitted collection as the only finality check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is a real edge case with long-running traces. The fix needs a "is this local root actually finished?" signal on the interface the interceptor uses (AgentSpan), which DDSpan already tracks internally but doesn't expose. I'm handling that as a small separate core PR (add AgentSpan.isFinished()), then this interceptor will key off the finished root instead of fragment presence. Tracked as a follow-up; not gating this PR since long-running traces are off by default.
There was a problem hiding this comment.
| static final int PRIORITY = 4; | ||
|
|
||
| /** The single, long-lived interceptor registered with the tracer (reconfiguration safety). */ | ||
| static final SpanEnrichmentInterceptor INSTANCE = new SpanEnrichmentInterceptor(); |
There was a problem hiding this comment.
In servlet/app-server deployments where dd-openfeature is loaded by a webapp classloader, this singleton is only singleton per classloader, but the tracer's interceptor set is JVM-wide and has no removal API. The first webapp's interceptor remains registered and pins that classloader; after redeploy a new classloader creates a different INSTANCE, registration is rejected by the same priority, and the new provider's store is never read, so span enrichment is disabled until JVM restart. Register the long-lived interceptor from an agent/bootstrap classloader or otherwise avoid retaining application classloaders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by the capture/write split: the interceptor is now created and registered by the agent-side SpanEnrichmentWriter (agent classloader), not a per-webapp singleton in the application classloader. No application classloader is retained, so redeploys are unaffected.
…digest
- Gate the __dd_split_serial_id / __dd_do_log evaluation metadata on the
span-enrichment flag so an enabled provider with enrichment off attaches
nothing extra per evaluation (DDEvaluator).
- Extract the metadata keys to named constants (METADATA_SPLIT_SERIAL_ID /
METADATA_DO_LOG) as the single source of truth; SpanEnrichmentHook
references them.
- Rename the config constant to EXPERIMENTAL_SPAN_ENRICHMENT_ENABLED to make
its experimental status obvious at call sites.
- Reuse a ThreadLocal<MessageDigest> for subject hashing instead of
allocating a SHA-256 instance per capture (ULeb128Encoder).
Add a non-breaking default boolean isFinished() to the AgentSpan interface (internal-api), surfacing the finished state DDSpan already tracks (durationNano != 0) so other modules can query it without reaching into dd-trace-core. DDSpan's existing method overrides the default; the TrackingSpanDecorator test wrapper delegates; leaf/synthetic spans (NoopSpan etc.) keep the false default. Adds a DDSpanTest case covering false-before / true-after-finish via the interface. Motivated by the FFE span-enrichment interceptor (PR #11658), which needs to distinguish a truly-finished local root from a still-running one (long-running traces) and resolve it in O(1); that consumer lands in a separate follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| metadataBuilder.addString(METADATA_SPLIT_SERIAL_ID, split.serialId.toString()); | ||
| } | ||
| metadataBuilder.addString( | ||
| METADATA_DO_LOG, String.valueOf(allocation.doLog != null && allocation.doLog)); |
There was a problem hiding this comment.
Nit: Is it required that these are strings? I would have imagined __dd_do_log would be a boolean
| void jsonUsesPlatformWriterEscaping() { | ||
| // The platform JsonWriter escapes '/' as backslash-slash and non-ASCII as a backslash-u escape. | ||
| // That differs from the JS reference bytes but is round-trip-equivalent: all consumers | ||
| // JSON-parse these tags (backend Jackson, system-tests json.loads), so byte-parity is not |
There was a problem hiding this comment.
Another way to verify this is https://github.com/DataDog/ffe-dogfooding
You may be able to run a local build of this code. Might need some extra setup to enable enrichment, but I was ultimately able to verify that this worked for the JS implementation when I saw this in the traces
…TraceInterceptor() false return: ensureInterceptorRegistered() now latches only on a true return; registration ordering: moved ensureInterceptorRegistered() below the root == null check
PerfectSlayer
left a comment
There was a problem hiding this comment.
Approving to remove the request for changes flag.
I focused my review on the high level design, I’ll let my pear having a review of the changes in details 🙏
…manovsky/ffe-apm-span-enrichment
…ataDog/dd-trace-java into leo.romanovsky/ffe-apm-span-enrichment
…ataDog/dd-trace-java into leo.romanovsky/ffe-apm-span-enrichment
sarahchen6
left a comment
There was a problem hiding this comment.
Looks like CI still requires a spotBugs clean-up, but otherwise, let's give it a try!
…manovsky/ffe-apm-span-enrichment
…ataDog/dd-trace-java into leo.romanovsky/ffe-apm-span-enrichment
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
feat(feature-flagging): FFE APM feature-flag span enrichment
Summary
Adds Feature Flag Events (FFE) span enrichment to the OpenFeature integration. When feature
flags are evaluated, the evaluation metadata is attached to the root APM span so APM customers
can filter traces and errors by active flag variant, and the FFE/Experimentation platform can
correlate spans with experiments. The wire format matches the merged reference implementation
(
dd-trace-js#8343) so backend/Trino decode is identical.How it works
finallyhook captures the evaluation (serial ID, targeting key, runtime default).SpanEnrichmentInterceptor.findLocalRootInFragment).ffe_*tags.Configuration
Opt-in, off by default. The gate is resolved through the standard
ConfigProviderprecedence(system property → stable config → environment variable), the same mechanism as the sibling
provider-enabled gate:
Canonical key:
experimental.flagging.provider.span.enrichment.enabled(
FeatureFlaggingConfig.SPAN_ENRICHMENT_ENABLED). This is distinct fromDD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED— enabling the provider does not enable spanenrichment. The provider logs
span enrichment enabled/span enrichment disabledat INFO onstartup.
Span tags added
ffe_flags_encffe_subjects_encdoLog=true){ sha256(key): encodedIds }ffe_runtime_defaults{ flagKey: value }Limits: 200 serial IDs, 10 subjects, 20 experiments/subject, 5 runtime defaults, 64 chars/runtime-default value (UTF-8-safe truncation).
Changes
experimental.flagging.provider.span.enrichment.enabledgate(
FeatureFlaggingConfig.SPAN_ENRICHMENT_ENABLED→DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED),off by default, read via
ConfigProviderso system property + stable config + env all apply;log enabled/disabled at INFO on startup; surface the split
serialIdin eval metadata(
ufc/v1/Split.java).ULeb128Encoder— delta-varint serial IDs, SHA256-hashed subject keys.SpanEnrichmentAccumulator,SpanEnrichmentHook,SpanEnrichmentInterceptor,SpanEnrichmentStates— accumulate evaluations and writeffe_*tags onto the local root span. State is held in aWeakHashMapkeyed by the local-root span object (no central id-keyed store, no cap/eviction): it is collected with its root, so a never-completing trace can't leak. This matches the Python/Ruby SDK design.Decisions
ffe_*tags.ffe_*are bare tag names on spanmeta(not_dd.-prefixed); subject keys are SHA256 hashes emitted only when logging is authorized.Validation
FFE dogfooding app
Validated live against the
ffe-dogfoodingapp via atrace-intaketee-proxy that captures the raw trace payload and decodes theffe_*tags. Flagffe-dogfooding-string-flag(serial2312):trace.annotation) carriedffe_flags_enc = iBI=decoding to serial[2312]plus a SHA256-hashedffe_subjects_enc→[2312](doLog=true), through the realdd-openfeatureprovider path.ffe_*tags.Local system-tests run
Ran the frozen
system-testsparametric suite (tests/parametric/test_ffe/test_span_enrichment.py, unchanged) againstdd-java-agent 1.64.0-SNAPSHOT(+ customdd-trace-api,dd-openfeature) built from this branch (HEAD8491123c5d):All 18 cases ran and asserted green, covering
ffe_flags_enc/ffe_subjects_enc/ffe_runtime_defaults, delta-varint encoding, SHA256 subject keys with doLog gating, and the 200/10/20/5/64 limits. (Reported asxpassedonly because the manifest declares the feature at releasev1.64.0while the local artifact is pre-release1.64.0-SNAPSHOT; it resolves to a cleanpassedat release.) No SDK source changes were required to pass. The system-tests enablement (parametric handler +manifests/java.yml) is a separate draft PR againstDataDog/system-tests.Full dogfooding matrix + regression suite (2026-06-17)
Re-validated end-to-end through the real OpenFeature provider path (javaagent built from this
branch,
java@1.64.0-SNAPSHOT+109eab80bc) behind thetrace-intaketee-proxy, decodingffe_*with
scripts/decode_ffe_span_tags.py(root spantrace.annotation, serviceffe-dogfooding-java):ffe_flags_enc→[2312];ffe_subjects_enc={sha256(targeting key): ids}only when__dd_do_logffe_*tagsffe_flags_enc=[829,1442,2311,2312], nothing overwrittenffe_*on the local root only; the child span carries noneValuestructure unwrapped to JSON{"grëeting":"こんにちは","emoji":"🎉","n":3}(notValue(innerObject=…)), raw UTF-8 with no\uXXXXescaping, valid JSON, values truncated to 64ZAgUAg==→[100,108,128,130]Deterministic regression suite (
SpanEnrichmentHookTest+SpanEnrichmentLifecycleRegressionTest, 28 cases, all pass) additionally covers:distinctTracesDoNotMerge(state keyed by local-root span identity),hookCapturedRootAndInterceptorResolvedRootAreSameObject(the capture and write sides resolve the same local-root instance),newProviderAfterShutdownStillEnriches+lateShutdownOfDisplacedProviderDoesNotClobberActiveProvider(reconfiguration),eachProviderOwnsADistinctStateStore,partialFlushNeverWritesTagsOnAChildSpan,neverCompletingTracesAreNotCapped+unreachableRootStateIsWeaklyCollected(weak-keyed lifecycle),gateOffConstructsNothingAndAccumulatesNoState, and theValue→JSON runtime-default cases.System-tests re-confirmed against this branch's artifacts: 18 passed (
TEST_LIBRARY=java ./run.sh PARAMETRIC -k span_enrichment, libraryjava@1.64.0-SNAPSHOT+109eab80bc).