Repository navigation
feat(openfeature): parse observeFullEvaluationData and hash targeting_key in flagevaluations events - #5151
Conversation
…_key in flagevaluations events
Make the no-PII path the default for server-side EVP flagevaluation events,
driven by the new top-level UFC boolean observeFullEvaluationData.
Without consent, targeting_key is emitted as an unsalted SHA-256 fingerprint
prefixed with "sha256_" (71 chars) and context.evaluation is omitted entirely
(absent key, not null, not {}). With consent, both are emitted verbatim. Absent
or null consent fails closed. The digest is taken over the raw UTF-8 bytes with
no trim, case folding, or Unicode normalization so digests join across SDKs.
Consent is snapshotted in the evaluator from the exact configuration the
evaluation ran against and stamped onto evaluation metadata under the
cross-SDK key observe_full_evaluation_data. Nothing downstream of the
evaluator reads live configuration, so a Remote Config update landing between
evaluation and flush cannot retroactively apply another environment's policy.
Consent is part of the full-tier bucket key so mixed-consent evaluations never
merge, with an AND-fold on the bucket's consent field as defense in depth. When
consent is off the context is dropped from the key and never captured, because
the key must carry exactly the dimensions that survive serialization; keying on
a discarded context would burn the per-flag bucket cap on precisely the
privacy-protected traffic. Consent is deliberately not a degraded-tier
dimension: that tier emits neither targeting_key nor context, so it would add
cardinality with no privacy gain.
DoLog does not gate this behavior, and the
DD_FLAGGING_EVALUATION_COUNTS_ENABLED kill switch still disables the track
entirely.
Environment: Datadog workspace
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: fe6ad42 | Docs | Datadog PR Page | Give us feedback! |
System-tests results against this PRRan the To exercise the new behavior end-to-end, I locally removed the following markers from
Results: 26 passed, 2 skipped, 0 failed
Follow-upA companion system-tests PR should remove the four manifest lines above to enable these tests in CI for Go. |
BenchmarksBenchmark execution time: 2026-08-18 00:24:06 Comparing candidate commit fe6ad42 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 335 metrics, 0 unstable metrics, 1 flaky benchmarks without significant changes.
|
L2 dogfooding — both consent paths verified end-to-end ✅Ran this branch against ffe-dogfooding with the staging org (
The 5 "full" entries under the consent-off flip are the last flush cycle that occurred before RC delivered the new UFC — everything after the RC refresh is protected. Restored the environment to Behaviors observed exactly matching the PR's contract table:
ReproduceDD_TRACE_GO_PATH=/path/to/dd-trace-go dd-auth --domain dd.datad0g.com -- env DD_SITE=datad0g.com \
docker compose -f docker-compose.yml -f local/docker-compose.go.yml up -d --build
# wait ~60s for first flush
curl -s "http://localhost:8090/logs?service=ffe-dogfooding-go&limit=1" | jq .
# open http://localhost:8080 for the dashboardTo flip consent (requires app+api keys with # fetch current, then PUT with observe_full_evaluation_data flipped
curl -s -H "DD-API-KEY: $DD_API_KEY" -H "DD-APPLICATION-KEY: $DD_APP_KEY" \
"https://api.datad0g.com/api/v2/feature-flags/environments/<env-id>"Note: this touches the shared "Staging" environment on L3 system-tests still owed per the PR's Validation status section. L1 already covers the hashed vector exhaustively ( |
Condense the added rationale comments to <=3 lines while preserving the load-bearing "why": consent snapshot must travel with the evaluation, AND-fold defense against key drift, empty-targeting-key must not hash, shadow struct is sole populator, fail-closed default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cting-pii-in-flagevaluations-track
A wrong-typed observeFullEvaluationData ("true", 1, [], {}) used to make the
shadow-struct UnmarshalJSON return the json.Unmarshal error and reject the
whole UFC. Agentless swallows the parse error, so a fresh pod hitting a
malformed message had no last-known-good config and stranded every flag on
the SDK default until a well-formed UFC arrived.
Read the field as json.RawMessage and interpret it leniently: absent, null,
or any wrong-typed value falls to false. Fail-closed on privacy must not
cascade into fail-closed on availability. Matches the Java pilot's
LenientBooleanAdapter (dd-trace-java@a27aa082).
Add wrong-typed test cases (stringified/numeric/array/object) that assert
both ObserveFullEvaluationData=false and that the UFC still parses.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
error.message can carry raw evaluation-context values verbatim: our own evaluator wraps user-supplied attributes in error strings on variant/type mismatches and version/regex parse failures, and third-party OpenFeature providers can do the same. Under consent-off this defeats the PII guard even when targeting_key is hashed and context.evaluation is omitted. Redact in extractEvalDetails on the evaluation thread — before the value enters the aggregation key or the async writer queue, both of which live beyond the evaluation call and would leak PII if we deferred redaction to emit time. Under consent-off, drop the raw text and substitute ErrorCode (TypeMismatchCode etc.) so operators keep a stable, non-PII signal. Consent-on behavior is unchanged. Add wire-level negative test asserting a PII-shaped ErrorMessage never appears in the serialized flush payload under consent-off, with a consent-on control confirming the same string still rides through. Matches the Java pilot fix (dd-trace-java@857d65b9). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…metry counters Gap 3: enforce max_key_length on evaluation keys, apply queue bounds before enqueue to prevent overflow, and split telemetry counters for clearer observability. Co-Authored-By: Claude <noreply@anthropic.com>
…ontains CI check-format and lint (modernize/slicescontains) flagged the test file. Apply gofmt and delete the local helper in favor of the stdlib. Co-Authored-By: Claude <noreply@anthropic.com>
… DDEvaluator flattenAndPruneContext previously flattened the ENTIRE caller context and only applied the 256-field / 256-char caps AFTER traversal, so the evaluation-thread cost was proportional to the caller's complete input rather than to the bounded snapshot. This made Go's synchronous consent-on path diverge from Java's DDEvaluator.copyPrunedContext, which caps every retained-size dimension inline during the walk. Replace the flatten-then-prune pipeline with a single-pass bounded walker (copyPrunedContext, the Go mirror of Java's) that enforces depth (4), list width (256), structure width (256), field count (256), key length (256), string-value length (256), and identity-based cycle detection BEFORE recursing. The exposure track's flattenContext (flatten.go) is untouched and remains unbounded for its own contract. New truncation reason codes (max_list_elements, max_structure_properties, max_snapshot_depth, cycle) match Java's DDEvaluator.REASON_NAMES. The per-level key sort that preserves Go's determinism guarantee is retained; its O(K log K) cost is no worse than the prior pipeline and is documented. Adds consent-on benchmarks (BenchmarkFlattenAndPruneContext) and adversarial-input tests (TestFlattenAndPruneContextBoundedTraversal) covering 100-deep chains, 10k-element lists, 10k-property structures, self/mutual cycles, and over-cap field counts. Addresses review feedback from @leoromanovsky on #5151 referencing the Java contract in dd-trace-java#11639. Generated with Claude Code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…th Java SDK Rename the evaluator-stamped metadata keys to match Java's DDEvaluator / FlagEvalLoggingHook contract: dd.eval.timestamp_ms -> __dd_eval_timestamp_ms observe_full_evaluation_data -> dd.observe_full_evaluation_data dd.doLog -> __dd_do_log dd.serialId -> __dd_split_serial_id These are SDK-internal (stamped by the evaluator, read by the EVP / exposure / span-enrichment hooks, never serialized to the wire), so the rename has no wire or cross-SDK effect — it only keeps the two SDKs' in-process metadata vocabularies identical so the same keys appear when reading either source. All read/stamp sites use the constants, so no raw strings needed updating. Rename the context-truncation reason label max_field_length -> max_value_length to match Java's DDEvaluator.REASON_NAMES (bit order was already aligned; only the bit-2 label string differed). The label is the cross-SDK-observable telemetry surface, so this keeps Go's future metric tags joinable with Java's countContextTruncated tags. Also fix 5 modernize/rangeint lint failures (golangci-lint v2.12.2) by converting constant- and len-bounded for loops to range-over-int. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…ize bounding
Bring the EVP flagevaluation path to feature parity with Java's
FlagEvaluationWriterImpl (dd-trace-java#11639):
Telemetry metrics (emitted via internal/telemetry.Count under
NamespaceGeneral, mirroring Java's CoreMetricCollector.count so Go's
counters are queryable alongside Java's):
flagevaluation.rows.dropped reason:queue_overflow (preQueueOverflow + enqueueDropped)
reason:closed (new closedDrop counter; post-stop drops
no longer conflated with queue-overflow)
reason:degraded_cap (droppedDegradedOverflow)
reason:payload_limit (encode-time drops, see below)
flagevaluation.rows.degraded reason:cardinality_cap (new degradedRows counter at the
addToDegraded routing sites)
reason:payload_limit (encode-time degraded fallback)
flagevaluation.context.truncated reason:<...> (contextTruncatedByReason; reason labels
already aligned with Java)
flagevaluation.payload.splits (payloads - 1)
Payload-size bounding (the transport robustness gap Go previously lacked):
buildFlagEvalPayloads mirrors Java's FlagEvaluationPayloads.buildPayloads —
it encodes the flush's wire events into one or more payloads each no larger
than payloadSizeLimitBytes (5 MiB, matching the Agent's EVP proxy intake
limit). When the next event would exceed the limit it starts a new payload
(split); a single event too large for an empty payload is retried as a
degraded event (targeting_key + context dropped — the bulky fields); if the
degraded form still doesn't fit, the event is dropped and counted. Before,
a too-large flush was rejected wholesale by the Agent with no partial send,
no per-event accounting, and no degraded retry.
evp.go gains postRaw (send pre-encoded bytes) and marshalJSON (encode a
value with the EVP client's jsoniter config) so the splitter can encode
events incrementally and pack them under the shared envelope prefix.
Adds TestBuildFlagEvalPayloads covering the four cases: single payload,
multi-payload split, single-event degraded fallback, and drop-when-even-
degraded-won't-fit.
Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
… metadata key Revert the consent metadata key to the cross-SDK contract value 'observe_full_evaluation_data' (unprefixed snake_case), matching the PII RFC and the Java pilot (DDEvaluator.METADATA_OBSERVE_FULL_EVALUATION_DATA). The prior 'dd.'-prefixed value diverged from both the spec and Java even though the key is SDK-internal (never on the wire); the RFC names this exact key as the cross-SDK contract so the same identifier appears when reading any SDK's source. The eval-timestamp key (__dd_eval_timestamp_ms) is unchanged — that one matches Java and carries the __dd_ internal-only prefix. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…prealloc lint) Generated with Claude Code
FFE Dogfooding verification — PII protection for
|
observeFullEvaluationData |
targeting_key |
context.evaluation |
|---|---|---|
true (original) |
raw, e.g. user-28zvjdbsi7u1 |
present ({country, plan, version}) |
false (flipped) |
hashed → sha256_415a…bf73 (64 hex chars) |
omitted (0/60 events) |
true (restored) |
raw | present (50/50) |
All 60 post-propagation events with the false setting had hashed targeting_key (prefixed sha256_ + unsalted SHA-256) and zero carried context.evaluation. Restoring to true brought back raw keys + eval context, with the expected flush-straddling mix during the transition.
3. The hashing matches the spec. openfeature/flageval_pii.go does unsalted SHA-256 over raw UTF-8 bytes (no trimming/folding/normalization → byte-identical across SDKs), uses the sha256_ prefix, and returns "" for an empty targeting key so unique-subject counts aren't corrupted by an invented pseudo-subject. The privacy-preserving default (false) wins on any parse ambiguity.
Result
- ✅ Go SDK emits well-formed
flagevaluationEVP events with the complete expected tag set - ✅ End-to-end delivery to staging confirmed (timeseries query returns
ffe-dogfooding-godata) - ✅
observeFullEvaluationData=false→targeting_keyhashed (sha256_+ 64 hex),context.evaluationomitted - ✅
observeFullEvaluationData=true→ rawtargeting_key,context.evaluationpresent - ✅ Bidirectional RC toggle works; flush-straddling transition behaves as expected
- ❌ Nothing failing for the Go SDK. All 6 test flags evaluate correctly across both PII modes.
Note: the staging environment's
observe_full_evaluation_datawas temporarily set tofalsefor this test and restored to its original value (true) afterward.
System-tests validation resultsValidated this branch ( End-to-end —
|
leoromanovsky
left a comment
There was a problem hiding this comment.
Thanks. This fixes the unbounded recursive value traversal. The new depth, list, field, and cycle limits are useful. The benchmarks also cover the correct adversarial inputs.
Keeping the flag evaluation "hot path" fast even when customers mis-use is it still an important goal.
One part of my feedback remains. Top-level and nested maps still collect and sort every key before applying the 256-property limit. Evaluation-thread work therefore remains proportional to the supplied map width.
The current benchmarks show this clearly:
- 256 flat properties: 40 µs and 27 KB allocated
- 5,000 flat properties: 500 µs and 105 KB allocated
- 10,000 map properties: 837 µs and 205 KB allocated
Therefore, the claim that work is "proportional to what is kept" is not correct for maps.
Please bound map processing on the evaluation thread. For example, stop or omit the map when its property count exceeds the limit. If this remains follow-up work, please remove the bounded-cost claims and track the remaining issue explicitly.
Top-level and nested maps whose key count exceeds the corresponding cap (maxContextFields / maxStructureProperties, both 256) are now omitted entirely via an O(1) len() check rather than collected, sorted, and trimmed. This keeps the consent-on evaluation-thread cost proportional to the KEPT set (<=256), never to the supplied map width, addressing the remaining review feedback on #5151. The prior implementation collected every key, sorted it (O(K log K)), then applied the 256-property limit mid-walk — so a 10 000-property structure cost ~837 us / 205 KB even though only 256 were retained. Behavioral trade-off: there is now a cliff at the cap. A 257-property structure is dropped wholesale where a 256-property structure is kept in full. Real contexts stay well under the caps, the truncation reason is still recorded for telemetry, and determinism is preserved (deterministic empty -> same aggregation bucket). Also fix an O(input) cost in the []byte branch of copyPrunedValue: the length is now checked before the string(v) conversion, so an oversized []byte value is dropped without copying it. BenchmarkFlattenAndPruneContext (count=3, Apple M4 Max): flat_over_cap_5000: 500 us/105 KB -> ~12 ns/16 B wide_structure_10000: 837 us/205 KB -> ~101 ns/64 B oversized_byteslice_1MB: (new) ~175 ns/368 B, no 1 MB copy In-bounds cases (flat_small, flat_at_cap_256, wide_list_10000, deep_chain_100, cyclic) are unchanged; the sort now only ever runs over <=256 keys. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
@leoromanovsky Thanks — your remaining feedback is addressed in fe6ad42 (just pushed). Top-level and nested maps no longer collect and sort every key before applying the 256-property limit. The changeA map (top-level or nested) whose key count exceeds the corresponding cap is now omitted entirely via an O(1) // flattenAndPruneContext (top-level)
if len(attrs) > maxContextFields {
return nil, []string{truncReasonMaxContextFields}
}
// copyPrunedMap (nested) + reflect-map case
if len(m) > maxStructureProperties {
mask.add(bitMaxStructureProperties)
return
}The collect → sort → walk now only runs on maps already within the cap (≤256 keys), so the evaluation-thread cost is bounded by the kept set, not the supplied width. The Trade-off: there's a cliff at the cap — a 257-property structure is dropped wholesale where a 256-property structure is kept in full. Real contexts stay well under the caps, and the doc comments now state this explicitly instead of claiming "proportional to what is kept." I also caught a related O(input) cost in the Benchmark results
In-bounds cases are unchanged; the sort now only ever runs over ≤256 keys. The over-cap map path is now O(1) — just the |
|
Re-tested with ffe-dogfooding and system-tests again locally and it's all looking gtg! |
Motivation
A server SDK upgrade must not silently ship subject PII (emails, account IDs, evaluation context) to the
flagevaluationstrack. The product metric — unique-subject counts per(flag, allocation)— needs a stable per-subject identifier, not the raw subject. This PR sends a one-way SHA-256 fingerprint by default and makes full-fidelity collection an explicit per-environment opt-in.Tracked by FFL-2962, under the server-SDK fan-out FFL-2784. The Java pilot is dd-trace-java#12042. The base track for Go was #4886. Cross-SDK contract tests were merged in system-tests#7316.
What does this PR do?
This PR makes the no-PII path the default for server-side
flagevaluationevents. A new UFC booleanobserveFullEvaluationDataselects the path.observeFullEvaluationDatatargeting_keyon the wirecontext.evaluationon the wiretruefalseor absent (default)sha256_+ 64-char lowercase hex (71 chars total)User impact:
flagevaluationstrack unless the environment opts in.(flag, allocation)stay accurate. Every SDK produces a byte-identical digest for the same subject, so hashed values join across languages.targeting_keyas the raw subject must accept thesha256_prefix on the default path.DD_FLAGGING_EVALUATION_COUNTS_ENABLEDkill switch is unchanged. It still disables the track entirely.Key design points:
observeFullEvaluationDatasits at the root of the UFC, a sibling ofenvironment. The customUnmarshalJSONcopies only the fields its shadow struct lists, so the field is added to both the struct and the shadow struct. Absent and explicitnullboth fail closed tofalse.observe_full_evaluation_data. Nothing downstream of the evaluator reads live configuration, so a Remote Config update between evaluation and flush cannot retroactively apply another environment's policy. The Java pilot hit this bug. A regression test guards both directions.No new configuration keys, no dependency changes, no generated files affected.
QA
L1 — Unit tests: done.
L2 — Dogfooding: done. Ran this branch against ffe-dogfooding with the staging org (
dd.datad0g.com). Tracer built asv2.11.0-dev.1.observeFullEvaluationDatatargeting_key(wire)context.evaluationpii_classtrue(default staging state)user-xxx)fullfalse(flipped for ~2 min)sha256_<64 hex>(71 chars)protectedThe 5 "full" entries under the consent-off flip are the last flush cycle before Remote Config delivered the new UFC. Every event after the RC refresh is protected. The staging environment was restored to
trueafter verification.L3 — System-tests: done locally, still owed in CI. Ran the
FEATURE_FLAGGING_AND_EXPERIMENTATIONscenario from DataDog/system-tests locally against this branch (golang@2.11.0-dev.1, weblog variantnet-http).Results: 26 passed, 2 skipped, 0 failed.
tests/ffe/test_flag_eval_evp.py: 11/11 passed, including the three tests unlocked by this PR andTest_FFE_EVP_Flagevaluation_Degradation(previously flaky, now clean).tests/ffe/test_flag_eval_metrics.py: 15 passed, 2 skipped (pre-existingirrelevantmarkers, unrelated to this PR).A companion
system-testsPR must remove the four manifest lines above to enable these tests in CI for Go.How to reproduce the L2 verification yourself
Run the tracer against ffe-dogfooding with the staging org:
Flip consent (requires app and API keys with
feature_flag_environment_config_write):The flip touches the shared
Stagingenvironment ondd.datad0g.com. Anything else running underDD_ENV=stagingpicks up the change too. Flip back promptly.Reviewer's Checklist
va.b.c-devversion tag. Tests already exist from system-tests#7316. The manifest activation is a companion PR. See "QA" above.BenchmarkFlagEvaluationOTelPlusEVPParallelstill covers the aggregation path.make lintlocally.make testlocally.make generatelocally. No generated files or configuration keys touched.go.modchanges: none.crypto/sha256andencoding/hexare stdlib.🤖 Generated with Claude Code