Parse observeFullEvaluationData and hash targeting_key in flagevaluations events - #12042
Conversation
…ions events Adds the top-level observeFullEvaluationData boolean to the UFC model, plumbs it through to the EVP flagevaluation event serializer, and gates PII handling on it: when the flag is absent/false the targeting key is SHA-256 hashed (sha256_<hex>) and the raw evaluation context is omitted from the wire; when true the raw targeting key and context are emitted. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Replace the inline "sha256_" literal with a documented HASHED_TARGETING_KEY_PREFIX constant describing the cross-SDK wire contract for privacy-preserving hashed targeting keys. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Parameterize the true/false config-parsing assertions with @valuesource and add a test locking in the fail-closed behaviour for an explicit JSON null: malformed config is rejected so full evaluation data is never observed off the back of it. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The flush-time read of FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled() was a TOCTOU bug: CURRENT_CONFIG could be overwritten by a later RC update between when an evaluation happened and when the batch flushed, so events could be emitted under the wrong environment's consent (the system test observed a targeting key hashed even though the active UFC said observeFullEvaluationData=true). Capture consent when the evaluation is folded into its EvalBucket instead. On merge the value is folded with AND, so any no-consent evaluation in a bucket's lifetime sinks the whole bucket to hashed/omitted (fail-closed). buildEventList now reads bucket.observeFullEvaluationData rather than the gateway. The gateway accessor is retained; it is read at aggregation time. Adds a writer-level regression guard (a bucket aggregated under consent-off stays hashed even if the gateway later reports consent-on) plus aggregator fold tests, and an end-to-end parse->dispatch->flush test. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
| Test | Result |
|---|---|
Test_FFE_EVP_Flagevaluation_ObserveFullData_Absent_Hashed |
✅ PASS |
Test_FFE_EVP_Flagevaluation_ObserveFullData_False_Hashed |
✅ PASS |
Test_FFE_EVP_Flagevaluation_ObserveFullData_True_Unhashed |
✅ PASS |
36 passed, 8 skipped, 2704 deselected, 1 xfailed, 3 xpassed in 301.86s
Weblog: spring-boot | Scenario: FEATURE_FLAGGING_AND_EXPERIMENTATION
Note: an earlier build (d65c79f266) of this PR failed True_Unhashed — the SDK was always hashing regardless of the flag value. Root cause: FlagEvaluationWriterImpl.buildEventList() read observeFullEvaluationData from CURRENT_CONFIG at flush time, which could be overwritten by a subsequent RC update before the 10s flush fired. The fix in this PR (capturing the value per-bucket at enqueue time with a privacy-preserving fold) resolves it.
|
Snapshot the PII consent flag on the evaluation thread (in the OpenFeature hook) and carry it on FlagEvalEvent, instead of reading the gateway when the event is aggregated/flushed. This pins the hashed-vs-raw decision to the configuration active at evaluation time, closing a one-directional leak window where a later Remote Config update could retroactively apply a different environment's consent to already-collected evaluations. Aggregation and flush now read event.observeFullEvaluationData and never consult the gateway; the AND-fold across a bucket's evaluations is unchanged (any no-consent evaluation sinks the bucket to hashed/omitted). Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ickie/FFL-2790-protect-pii-with-observeFullEvaluationData Bring the parent aggregate flagevaluation EVP branch, and the agentless configuration source and master through it, into the observeFullEvaluationData PII branch. No conflicts. Only three files were touched by both sides, and each merged into a coherent union: - FeatureFlaggingGateway: the parent's provider ActivationListener machinery and this branch's observeFullEvaluationData accessor are disjoint additions. - FeatureFlaggingGatewayTest and DDEvaluatorTest: unions of both suites, with no method dropped from either side. Environment: Datadog workspace Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
More details
The event-time snapshot, conservative bucket fold, and serialization gates preserve the no-PII default while retaining raw data only for explicitly consented evaluations. No additional tests recommended: production telemetry is unavailable and the PR already covers the relevant consent transitions and wire shape.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit e0e3632 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0e3632b8c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| final boolean observeFullEvaluationData = | ||
| FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled(); |
There was a problem hiding this comment.
Bind consent to the evaluated configuration
When Remote Config changes from observeFullEvaluationData=false to true while an evaluation is in progress, DDEvaluator.evaluate can finish using the old configuration while this hook reads the new global value. FeatureFlaggingGateway.dispatch updates CURRENT_CONFIG before notifying the evaluator, and the evaluator retains its configuration in a local variable, so this race can mark an evaluation performed without consent as consented and emit its raw targeting key and context. Carry the consent from the exact ServerConfiguration used by the evaluator, for example through evaluation metadata, instead of rereading the gateway here.
Useful? React with 👍 / 👎.
| final boolean observeFullEvaluationData = event.observeFullEvaluationData; | ||
| final Map<String, Object> prunedAttrs = pruneContext(event.contextAttributes()); | ||
| final String ctxKey = canonicalContextKey(prunedAttrs); |
There was a problem hiding this comment.
Stop keying redacted buckets by hidden context
perf: When observeFullEvaluationData is false—the new default—a changing context field such as a request ID still contributes to ctxKey and creates a distinct full-tier bucket, even though serialization omits that context. At sufficient traffic this emits many wire-identical rows, exhausts the 10,000-bucket per-flag cap within a flush interval, and forces subsequent evaluations into the degraded tier without targeting information. No-consent evaluations should aggregate without the context dimension that will be discarded.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
Background
Feature-flag evaluations are reported to Datadog so users can see how their flags behaved in production. Today, every evaluation carries the raw targeting key (the identifier of the subject being evaluated; often a user email or user ID) and the full evaluation context. Those fields can contain personally identifiable information (PII).
Motivation & High level changes
This PR lets the server decide, per environment, whether that raw data may be observed via a new boolean on the UFC. It is the Java piece of the cross-SDK "Protecting PII in flagevaluations" initiative.
dd-trace-javais the pilot SDK; the same pattern will be copied to the other SDKs afterward.This directly resolves the privacy concern raised on the parent PR #11639, where @AlexeyKuznetsov-DD flagged that flag-evaluation reporting was default-on and shipped the raw
targeting_key(user IDs / emails) plus the full evaluation context in clear text, and asked for hashing / opt-in / an explicit privacy sign-off. The no-PII path is now the default.observeFullEvaluationDatatruefalseor absent (privacy-preserving default)sha256_<hex>"Absent" is treated exactly like
false, so an older/cached config can never accidentally leak raw values.How it works
observeFullEvaluationDataon the downloaded flag-configuration (UFC) model (top-level boolean, defaults tofalsewhen missing).FlagEvalLoggingHook) snapshots the gateway's consent value (FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()) at the moment the flag is evaluated and stores it asFlagEvalEvent.observeFullEvaluationData. Aggregation and serialization then read that per-event snapshot and never consult the gateway — so the hashed-vs-raw decision is pinned to the configuration active when the evaluation happened.CURRENT_CONFIGcan be overwritten by a later Remote Config update between when an evaluation happened and when the batch is drained and flushed. Reading the gateway at flush would retroactively apply a different environment's consent to already-collected evaluations. (This was a real bug caught by system-tests: a targeting key came back hashed even though the active UFC saidtrue.) Snapshotting on the evaluation thread closes that window in both directions.sha256_; the context field is dropped so it's absent from the JSON (notnull, not{}).SHA-256 hash
The hash is unsalted SHA-256 over the raw UTF-8 bytes (no trimming/case/Unicode normalization), emitted as lowercase hex. Unsalted is a deliberate cross-SDK contract requirement: every SDK must produce the identical digest for a given targeting key so hashed values line up across languages and against the backend. It matches the canonical test vector shared across all SDKs:
Performance
Consent is read once per evaluation on the hook path — a single
AtomicReference.getsnapshotted onto the event (no allocation, no per-span hot path) — and folded with a boolean AND on bucket merge. Hashing runs at flush cadence, bounded by the number of distinct evaluation buckets, and uses aThreadLocal<MessageDigest>(no per-callMessageDigest.getInstance).Testing
true,false(as a@ValueSourcematrix), absent (defaults tofalse), and explicitnull(rejected / fail-closed).true,false, and absent →false) at enqueue time;FlagEvalEventtests cover the new field and the fail-closed default on the convenience constructors.🤖 Generated with Claude Code