Skip to content

Parse observeFullEvaluationData and hash targeting_key in flagevaluations events - #12042

Open
vjfridge wants to merge 14 commits into
leo.romanovsky/ffl-2446-evp-flagevaluation-javafrom
vickie/FFL-2790-protect-pii-with-observeFullEvaluationData
Open

Parse observeFullEvaluationData and hash targeting_key in flagevaluations events#12042
vjfridge wants to merge 14 commits into
leo.romanovsky/ffl-2446-evp-flagevaluation-javafrom
vickie/FFL-2790-protect-pii-with-observeFullEvaluationData

Conversation

@vjfridge

@vjfridge vjfridge commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

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-java is 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.

Server says observeFullEvaluationData targeting key evaluation context
true sent as-is (raw) included (raw)
false or absent (privacy-preserving default) SHA-256 hashed, sha256_<hex> omitted entirely

"Absent" is treated exactly like false, so an older/cached config can never accidentally leak raw values.

How it works

  • New field observeFullEvaluationData on the downloaded flag-configuration (UFC) model (top-level boolean, defaults to false when missing).
  • Consent is captured at evaluation time, on the evaluation thread, and carried on the event. DDEvaluator snapshots the active UFC's observeFullEvaluationData once per evaluation and stamps it onto the ProviderEvaluation metadata; the OpenFeature hook (FlagEvalLoggingHook) reads that metadata value and stores it as FlagEvalEvent.observeFullEvaluationData. Aggregation and serialization then read that per-event snapshot and never consult the live gateway — so the hashed-vs-raw decision is pinned to the configuration active when the evaluation happened.
    • Why not read it at drain/flush? CURRENT_CONFIG can 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 said true.) Snapshotting on the evaluation thread closes that window in both directions.
    • Bucket separation. observeFullEvaluationData is part of the aggregation bucket key (FullKey / DegradedKey), so events with different consent values never merge into the same bucket. Consent-off and consent-on evaluations for the same subject land in distinct buckets and are emitted with their respective policies. An AND-fold on the bucket's consent field is kept as defense-in-depth in case the key ever drifts from the field.
  • When hashing, the targeting key is run through the shared ULeb128Encoder.hashTargetingKey helper and prefixed with sha256_; the context field is dropped so it's absent from the JSON (not null, not {}).
  • Evaluation logic is unchanged.

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:

"jane.doe@datadoghq.com" -> sha256_b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b

Performance

Consent is read once per evaluation on the hook path — a single AtomicReference.get snapshotted onto the event (no allocation, no per-span hot path). Hashing runs at flush cadence, bounded by the number of distinct evaluation buckets, and uses a ThreadLocal<MessageDigest> (no per-call MessageDigest.getInstance).

Testing

  • Hash helper matches the canonical PII vector; preserves whitespace/case; is lowercase 64-char hex.
  • Config parsing covers true, false (as a @ValueSource matrix), absent (defaults to false), and explicit null (rejected / fail-closed).
  • Serialization covers both raw and hashed paths, asserting on the raw wire bytes that the hashed value is present, the raw PII value never appears, and no per-event context is emitted.
  • Consent-capture at evaluation time: hook tests assert the event's consent value is read from the evaluation metadata stamped by DDEvaluator at evaluation time (true, false, absent → false), and that the hook ignores the live gateway even when it disagrees with the metadata (ignoresGatewayConsentEvenWhenItDisagreesWithMetadata). FlagEvalEvent tests cover the new field and the fail-closed default on the convenience constructors.
  • Consent-timing regression guards (both directions): a bucket whose event was snapshotted with consent off stays hashed even if a later RC update turns the gateway on before drain/flush; and symmetrically, an event snapshotted with consent on stays raw even if the gateway is later turned off — proving neither aggregation nor flush consults the gateway.
  • Bucket separation: mixed-consent evaluations for the same subject land in distinct buckets (mixedConsentEvaluationsForSameSubjectLandInDistinctBuckets), and same-consent evaluations merge as before (sameConsentEvaluationsForSameSubjectMergeIntoOneBucket).

🤖 Generated with Claude Code

…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>
@vjfridge vjfridge added tag: ai generated Largely based on code generated by an AI or LLM comp: openfeature OpenFeature type: feature Enhancements and improvements labels Jul 22, 2026 — with ddtool CLI
@datadog-prod-us1-5

datadog-prod-us1-5 Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
Patch Coverage: 94.12%
Overall Coverage: 58.03% (+0.01%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 1b034cc | Docs | Datadog PR Page | Give us feedback!

vjfridge and others added 3 commits July 23, 2026 15:58
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>
@vjfridge

vjfridge commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

system-tests validation (L3) — Java SDK ✅

Ran the three new observeFullEvaluationData PII-protection tests from system-tests#7316 locally against this branch (java@1.65.0-SNAPSHOT+81af153ca7).

All three pass:

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.

@vjfridge

vjfridge commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

ffe-dogfooding validation (L2) - Java SDK ✅

I ran this branch's dd-java-agent + dd-openfeature build through our OpenFeature dogfooding harness against a staging UFC, exercising all three behaviors of the PII-protection contract:

Protected — observeFullEvaluationData = false (default):

  • targeting_key = sha256_ + 64-char lowercase-hex SHA-256 digest (71 chars total)
  • context.evaluation omitted
  • consistent across every emitted flagevaluation event

Full — observeFullEvaluationData = true (opt-in):

  • targeting_key raw (verbatim subject id)
  • context.evaluation present with the full evaluation context

Kill switch — DD_FLAGGING_EVALUATION_COUNTS_ENABLED = false:

  • zero flagevaluation events emitted regardless of observeFullEvaluationData (SDK logs Flag evaluation EVP writer disabled)

Notes:

  • Pointing the app at environments with observeFullEvaluationData false vs true produced exactly the expected payload shapes.
  • Hashing was applied even to FLAG_NOT_FOUND (runtime-default) evaluations — the privacy posture is enforced independent of evaluation outcome. 👍
  • The SDK posts to the singular /api/v2/flagevaluation EVP route, as expected for the flagevaluation track.

Validation harness changes: ffe-dogfooding#100 — adds the per-SDK PII posture dashboard badge and fixes the singular /api/v2/flagevaluation intake route.

Verdict: the Java SDK is performing as expected for the PII-protection contract — default hashing, opt-in full data, and kill switch all behave correctly. Cross-SDK hash equality is covered in the companion system-tests work.

vjfridge and others added 2 commits July 24, 2026 20:06
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>
@vjfridge
vjfridge marked this pull request as ready for review July 28, 2026 14:37
@vjfridge
vjfridge requested a review from a team as a code owner July 28, 2026 14:37
@vjfridge
vjfridge requested review from pavlokhrebto and typotter and removed request for a team July 28, 2026 14:37

@datadog-prod-us1-5 datadog-prod-us1-5 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datadog Autotest: PASS

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.

Was this helpful? React 👍 or 👎

📊 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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +116 to +117
final boolean observeFullEvaluationData =
FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had a good goal to seal the full evaluation metatadata to the configuration at the time of evaluation. The configuration you are accessing is mutable by other threads. Read it earlier than the hook, in the evaluation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, returning the flag via evaluation metadata seems like a good way to make sure the flag is consistent with evaluation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @leoromanovsky and @dd-oleksii for the +1s here! Storing the observeFullEvaluationData value with the evaluation metadata is much cleaner.

Addressed in 933cf3c ("Bind observeFullEvaluationData consent to the evaluator's configuration"), with follow-ups in 98df3ba and b6d9d60.

DDEvaluator.evaluate() now stamps observe_full_evaluation_data onto every returned ProviderEvaluation, sourced from the UFC snapshotted at the top of evaluate(). The hook reads consent from that metadata and no longer queries the gateway. FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled() was deleted so the race can't be reintroduced.

Semantics:

  • Every DD-produced evaluation carries the key, including PROVIDER_NOT_READY (stamped false).
  • Missing key = non-DD provider → hook falls back to false (fail-closed).

Comment on lines 52 to 54
final boolean observeFullEvaluationData = event.observeFullEvaluationData;
final Map<String, Object> prunedAttrs = pruneContext(event.contextAttributes());
final String ctxKey = canonicalContextKey(prunedAttrs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full aggregation key includes the evaluation context. This behavior is correct when observeFullEvaluationData=true.

For example, these evaluations have different contexts:

  targeting key: alice@example.com
  context 1: {plan: pro, request_id: req-1}
  context 2: {plan: pro, request_id: req-2}
  context 3: {plan: pro, request_id: req-3}

When full evaluation data is enabled, the SDK emits each context. Separate aggregation buckets are necessary.

When observeFullEvaluationData=false, the SDK omits the evaluation context. All three evaluations then have the same visible identity:

  {
    "flag": {"key": "checkout"},
    "variant": {"key": "enabled"},
    "targeting_key": "sha256_<alice-hash>"
  }

The current implementation still uses request_id in the aggregation key. It creates three buckets, although the emitted context is identical.

The protected path also performs unnecessary operations. It:

  • copies the evaluation context
  • flattens the evaluation context
  • sorts and prunes its fields
  • creates a canonical context key
  • stores the context until the next flush
  • removes the context during serialization.

High-cardinality fields can create many unnecessary buckets. Examples include request IDs, timestamps, and correlation IDs.

The per-flag limit is 10,000 full buckets. After this limit, the SDK moves later evaluations to degraded aggregation. Degraded events omit both the context and targeting key.

When observeFullEvaluationData=false, the SDK must:

  • keep the targeting key in the aggregation key
  • omit the evaluation context from the aggregation key
  • avoid evaluation-context processing
  • avoid storing the evaluation context.

Different subjects must remain in separate buckets:

  alice@example.com -> sha256_<alice>
  bob@example.com   -> sha256_<bob>

Different contexts for the same subject must use one protected bucket.

👉 Overall this feels like a "P1" to me; see if it is straight-forward to resolve.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @leoromanovsky! This is a great catch I did not think about and will definitely save us form degrading evaluations unnecessarily.

What are we doing now?

Per your example of

targeting key: alice@example.com
  context 1: {plan: pro, request_id: req-1}
  context 2: {plan: pro, request_id: req-2}
  context 3: {plan: pro, request_id: req-3}

the new behavior is:

Case A: observeFullEvaluationData = true for all three

The three ctxKeys are distinct because request_id differs:

  • Event 1 → ctxKey₁ = canonical serialization of {plan:pro, request_id:req-1}
  • Event 2 → ctxKey₂ = canonical serialization of {plan:pro, request_id:req-2}
  • Event 3 → ctxKey₃ = canonical serialization of {plan:pro, request_id:req-3}

At flush, includeRawContext = isFullTier && observeFullEvaluationData is true, so each bucket serializes the full pruned context:

row 1: {targeting_key: "alice@example.com", context: {plan:pro, request_id:req-1}}
row 2: {targeting_key: "alice@example.com", context: {plan:pro, request_id:req-2}}
row 3: {targeting_key: "alice@example.com", context: {plan:pro, request_id:req-3}}

Results in three genuinely different raw evaluations, three rows on the wire 👍

Case B: observeFullEvaluationData = false for all three

In the hook (before enqueue):

if (observeFullEvaluationData) { ... } else {
  // no snapshotAttrs, no supplier — event carries Collections.emptyMap()
}

Each event enqueues with attrs = {} and the three FullKeys are:

These three are byte-for-byte identical, so they all hash to the same bucket.

Final state: one full-tier bucket, count = 3.

At flush, includeRawContext = isFullTier && observeFullEvaluationData is false, and resolveTargetingKey hashes:

row 1: {targeting_key: "sha256_b4698f9b...9e51b", count: 3, first_ms: 1000, last_ms: 3000}

Results in one row on the wire. All three raw contexts (which contained the same PII-adjacent request_id values you'd want protected) are gone. They were never allocated, never sorted, never stored.

Implementation in three commits

  • 9e5253b — aggregator skips context work on the protected path.
  • 79c8009 — hook skips context capture on the hot path when consent is off
  • c4b9a82 — observeFullEvaluationData is now part of FullKey / DegradedKey

@leoromanovsky
leoromanovsky requested review from dd-oleksii and leoromanovsky and removed request for typotter July 28, 2026 23:07

@leoromanovsky leoromanovsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two conditions to resolve please.

Comment on lines +56 to +57
* FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled()}) so the consent decision is pinned
* to the instant the flag was evaluated, not to whatever configuration happens to be active when

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking

Comment on lines 52 to 54
final boolean observeFullEvaluationData = event.observeFullEvaluationData;
final Map<String, Object> prunedAttrs = pruneContext(event.contextAttributes());
final String ctxKey = canonicalContextKey(prunedAttrs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full aggregation key includes the evaluation context. This behavior is correct when observeFullEvaluationData=true.

For example, these evaluations have different contexts:

  targeting key: alice@example.com
  context 1: {plan: pro, request_id: req-1}
  context 2: {plan: pro, request_id: req-2}
  context 3: {plan: pro, request_id: req-3}

When full evaluation data is enabled, the SDK emits each context. Separate aggregation buckets are necessary.

When observeFullEvaluationData=false, the SDK omits the evaluation context. All three evaluations then have the same visible identity:

  {
    "flag": {"key": "checkout"},
    "variant": {"key": "enabled"},
    "targeting_key": "sha256_<alice-hash>"
  }

The current implementation still uses request_id in the aggregation key. It creates three buckets, although the emitted context is identical.

The protected path also performs unnecessary operations. It:

  • copies the evaluation context
  • flattens the evaluation context
  • sorts and prunes its fields
  • creates a canonical context key
  • stores the context until the next flush
  • removes the context during serialization.

High-cardinality fields can create many unnecessary buckets. Examples include request IDs, timestamps, and correlation IDs.

The per-flag limit is 10,000 full buckets. After this limit, the SDK moves later evaluations to degraded aggregation. Degraded events omit both the context and targeting key.

When observeFullEvaluationData=false, the SDK must:

  • keep the targeting key in the aggregation key
  • omit the evaluation context from the aggregation key
  • avoid evaluation-context processing
  • avoid storing the evaluation context.

Different subjects must remain in separate buckets:

  alice@example.com -> sha256_<alice>
  bob@example.com   -> sha256_<bob>

Different contexts for the same subject must use one protected bucket.

👉 Overall this feels like a "P1" to me; see if it is straight-forward to resolve.

Comment on lines +116 to +117
final boolean observeFullEvaluationData =
FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had a good goal to seal the full evaluation metatadata to the configuration at the time of evaluation. The configuration you are accessing is mutable by other threads. Read it earlier than the hook, in the evaluation.

Comment on lines +125 to +128
public static boolean isObserveFullEvaluationDataEnabled() {
final ServerConfiguration current = CURRENT_CONFIG.get();
return current != null && current.observeFullEvaluationData;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: given that the CURRENT_CONFIG is atomic, the result of this function is outdated as soon as it returns. Snapshotting "at evaluation time" is not enough to ensure they are in-sync. Ideally, the caller gets the flag from the configuration they are going to use for evaluation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for raising @dd-oleksii! I reworked to follow y'alls advice and isObserveFullEvaluationDataEnabled() was removed in 933cf3c. The hook now reads consent from evaluation metadata collected by DDEvaluator.evaluate() against the exact UFC it used. No caller reads the gateway for consent anymore.

vjfridge and others added 5 commits July 29, 2026 12:22
Address PR #12042 review feedback (Codex P1, leoromanovsky, dd-oleksii): the
FlagEvalLoggingHook was reading observeFullEvaluationData from
FeatureFlaggingGateway.isObserveFullEvaluationDataEnabled() at hook-fire time,
which races against a Remote Config swap of CURRENT_CONFIG that happens after
DDEvaluator.evaluate() captured its own ServerConfiguration reference. That
race can retroactively mark an evaluation performed without consent as
consented and leak the raw targeting key / context.

DDEvaluator now stamps the boolean directly from the ServerConfiguration it
used, onto every ProviderEvaluation via ImmutableMetadata under key
"dd.observe_full_evaluation_data". The hook reads consent from that metadata
and no longer queries the gateway. Missing metadata (PROVIDER_NOT_READY or a
non-DD provider) → false, the privacy-preserving default.

The gateway's isObserveFullEvaluationDataEnabled() accessor is removed since
its only real caller was the hook and re-adding it would re-open the race.

Adds regression tests: hook honours consent metadata (true/false/absent) and
ignores a gateway value that disagrees; evaluator stamps the correct boolean
on the FLAG_NOT_FOUND path and omits metadata when it holds no config.

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the previous commit: the private error() and resolveVariant()
helpers only ever read one field off the ServerConfiguration
(observeFullEvaluationData), so pass the boolean directly instead of the whole
config. Keeps the internal API narrow and removes the incidental coupling
these helpers had to the UFC.

While here, PROVIDER_NOT_READY now stamps consent as the privacy-preserving
false rather than omitting the metadata. Same on-the-wire outcome the hook
would have produced, but the invariant "every DD-produced evaluation carries
dd.observe_full_evaluation_data" is now unconditional, which is easier to
reason about. The two error() overloads collapse to one (the (String) null
casts at call sites disappear along with them).

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The key is only ever read by FlagEvalLoggingHook one line later — it never
lands on the wire, so it doesn't need the "dd." namespacing that
"dd.eval.timestamp_ms" has (that key is re-emitted onto spans).

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The race-vs-CURRENT_CONFIG backstory is captured in the previous commits'
messages; the code only needs the forward-looking invariants (metadata is
source of truth, missing key = false, DD-produced evaluations always stamp).

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address PR #12042 review from leoromanovsky (escalated Codex P2 → P1): on the
protected path (observeFullEvaluationData=false) the serializer drops the
evaluation context, but the aggregator was still running it through
pruneContext + canonicalContextKey and keying every full-tier bucket on it. A
high-cardinality field on the evaluation context (request_id, timestamp,
correlation id) would fragment buckets that emit byte-identical wire rows,
blow out PER_FLAG_CAP (10k) inside one flush window, and force subsequent
evaluations into the degraded tier — which drops the targeting key entirely.

On the protected path aggregate() now uses ctxKey="" and stores
prunedAttrs=null, so different contexts for the same subject collapse into one
bucket. The targeting key stays in the aggregation identity, so different
subjects still hash to different buckets. The consent-on path is unchanged.

Regression tests: protected path collapses differing contexts for one subject;
protected path still separates distinct subjects; full path still splits on
context. Existing tests that exercise pruneContext / context-differentiation
were updated to use consent=on (that's the code path they actually cover).

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vjfridge and others added 3 commits July 30, 2026 13:13
Companion to the aggregator fix: with observeFullEvaluationData=false the
evaluation context is dropped on emit and no longer influences aggregation,
so there is no reason to snapshot it on the evaluation thread. The hook now
branches on consent up front — the protected path enqueues an event with an
empty materialized attrs map (no map copy of the OpenFeature context, no
Supplier<Map> allocation, no lambda instance), while the consent-on path is
unchanged.

Grep confirms the only production consumer of FlagEvalEvent.contextAttributes
/ FlagEvalEvent.attrs is FlagEvaluationAggregator.aggregate, which already
skips them on the protected path.

Regression test: mutating the EvaluationContext after finallyAfter returns
still yields empty attrs on the enqueued event — proves the hook never
snapshotted it. Two existing tests that exercise the snapshot mechanism were
switched to pass consent-on metadata (that's the code path they cover).

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bucket keys should cover every dimension the emitter will branch on. The
serializer branches on observeFullEvaluationData (hashes the targeting key
and drops the context when off), so two evaluations that differ only in
consent produce different wire rows and must not share a bucket.

Before this change they could: same subject, same flag, same empty context
would land under the same FullKey regardless of consent, and the AND-fold
would silently downgrade a consent-on evaluation to the protected wire shape
because a nearby consent-off event merged into its bucket first. No PII leak
(fail-closed direction), but arrival-order-dependent semantics and a lost
raw-context row.

Add observeFullEvaluationData to FullKey / DegradedKey (equals + hashCode).
The AND-fold on bucket.observeFullEvaluationData stays as defensive belt-
and-suspenders; every event merging into a bucket now carries the matching
consent value by construction.

Regression test: two events identical except for consent land in two full-
tier buckets, one consent-on and one consent-off. Updated the previous
"fold to false on mixed consent" test to reflect the new invariant.

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test asserts that context attributes flow through the logging hook,
but the mock metadata omitted the observe-full-evaluation-data flag, so
the hook took the privacy-preserving path and dropped context.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: openfeature OpenFeature tag: ai generated Largely based on code generated by an AI or LLM type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants