Repository navigation
feat(openfeature): parse observeFullEvaluationData and hash targeting_key in flagevaluations events - #19554
Closed
vjfridge wants to merge 46 commits into
Closed
feat(openfeature): parse observeFullEvaluationData and hash targeting_key in flagevaluations events#19554vjfridge wants to merge 46 commits into
vjfridge wants to merge 46 commits into
Conversation
Contributor
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 1 job - 1 passed on retry 🔗 Commit SHA: a3f68d2 | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-08-28 04:51:00 Comparing candidate commit a3f68d2 in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in candidate:
Found 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 0 unstable metrics.
|
Circular import analysis
|
Codeowners resolved asResolved from the full PR diff against |
Dependency direction analysis
|
Capture immutable, bounded evaluation context before queue admission and harden writer lifecycle, shutdown, fork, and drop accounting. Add deterministic regression coverage and registered benchmark profiles for FFL-3060. Generated with Claude Code
The two snapshot-error log calls used exc_info=True. The bounded walk calls __iter__ and __getitem__ on the caller's own context object, so a caller-raised exception message and its traceback can carry customer context data. The canonicalization handler on the drain thread has the same exposure through traceback frame locals. Context data is consent-gated in the context.evaluation payload, so a leak to the log sink bypasses that gate entirely. Log the exception type name instead, which is a fixed vocabulary. Adds a test per site asserting the sensitive text does not appear in the log record and that exc_info stays unset. Co-Authored-By: Claude <noreply@anthropic.com>
…e snapshot An unsupported value or a non-string key raised TypeError from _validated_leaf and _flatten_mapping. That aborted the whole walk, so one bad field discarded every valid field around it. Ruby's bounded_context_snapshot falls through its leaf type case instead. One caller value cannot discard its neighbours. This change matches that behaviour: the offending field is skipped, a new unsupported_value truncation reason is counted, and the walk continues. The leaf check also moves from exact type identity to isinstance with unbound builtin normalization. IntEnum, StrEnum, and str subclasses used for lazy translation are common in real evaluation context. They were dropped before. Normalizing through str.__str__, int.__int__, float.__float__, and datetime.isoformat keeps a caller override of those hooks from running. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
_stop_drain_worker called worker.join() with no timeout. Aggregation invokes __eq__ and __hash__ on the caller's own canonical context key, so a caller implementation that blocks or deadlocks holds process exit open forever. Bound the wait at 5 seconds, matching dd-trace-rb and dd-trace-java. A timeout is safe. _aggregate and the periodic() snapshot both hold self._lock, so a worker that is still running writes into the post-swap maps instead of corrupting the flush. Such an event flushes late rather than never. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
_accepting_events was a plain attribute, so it survived a fork unchanged. Only _start_service sets it back to True, and a fork child never calls it: threads. _after_fork_child restarts the PeriodicThread directly. A child forked while the parent was in on_shutdown or _stop_service therefore inherited False. Its drain and flush threads ran, but every enqueue dropped as "closed" for the life of the process. Move the flag into _WriterProcessState, which forksafe.ResetObject replaces in the child, alongside the queue, aggregation maps, and counters. The child now starts with its intake open. The parent state is unaffected. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
The hook returns to the caller immediately after enqueue, so the caller is free to reuse or mutate the context object it passed. The PR relies on the snapshot being detached, but no test held that guarantee. Add two tests. The first mutates every container kind after flatten_and_prune_context: a scalar, a nested mapping, a list element, plus a key added and a key deleted. The second does the same through enqueue and asserts the queued event, including that the buffered mapping rejects writes. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…apshots The existing tests cover the same dict object and differing insertion order. Neither covers what aggregation actually depends on: two separate evaluations, each building its own snapshot object, must produce one identical key so they bucket together. Add a test over every supported leaf type — str, int, bool, float, None, datetime, nested mapping, and list — built twice in opposite field order, and a test that fifty repeated calls yield exactly one distinct key. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
enqueue runs on arbitrary caller hook threads and spans three separate locks. The O(1) queue-full check is not atomic with put_nowait, so the pre-queue and queue.Full drop paths can both run for the same offered load. The file had no concurrency test, so a lost update or a double count in either counter would not have been caught. Add two tests. The first drives 8 threads at a 64-slot queue and asserts buffered + dropped_pre_queue + dropped_queue equals the total attempted, and that both drop paths were exercised. The second races 6 producers against a drain-and-snapshot loop and asserts the same identity across the flushed, residual, and dropped counts. Both assert no caller thread blocked. Verified over five consecutive suite runs under pytest-randomly reordering. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Remove the Python-only traversal guard, preserve exact drop telemetry, and make shutdown flushing race-safe across writer workers. Generated with Claude Code
vjfridge
force-pushed
the
vickie/ffl-2964-protecting-pii-in-py-flagevaluations-track
branch
from
August 27, 2026 19:11
62f9f02 to
437f715
Compare
vjfridge
changed the base branch from
main
to
vickie/ffl-3060-bounded-evaluation-context
August 27, 2026 19:16
Generated with Claude Code
Remove six tests that assert implementation text, base classes, or the test helper itself rather than writer behavior, and merge four groups of duplicated setup into their emission-side counterparts. The cap constants keep a single assertion, restated as the cross-SDK derivation they encode: these bounds are mirrored in dd-trace-rb, dd-trace-java, and dd-trace-go, so drift makes the same customer context serialize differently per language. Rename test_drop_accounting_is_complete_no_silent_loss, whose docstring claimed a conservation check the body never performed, and point it at the concurrent test that does. 684 passed, 2 skipped on riot venv 14fc413 (py3.13), unchanged from before the trim. Generated with Claude Code
… walk The bounded traversal added in this branch regressed the hook enqueue path by 60-75% across the openfeatureflagevaluation benchmarks. Nearly all of it was per-field dispatch overhead rather than the new caps themselves. Every field, including a plain string, reached its leaf through a call to _flatten_bounded, which tests isinstance against the Mapping and Sequence ABCs before falling through to _validated_leaf and its five-branch isinstance ladder. ABC isinstance is not cheap, and an exact scalar can match neither container protocol, so that dispatch was pure overhead on the common case. Take exact str/int/float/bool/None to the output dict directly from the container loops. Subclasses still take the _flatten_bounded path, so the unbound-builtin normalization that keeps a caller __str__ or __float__ override from running is unchanged. Also hoist the per-container key allowance out of the loops; it depends only on the prefix, which is fixed for the life of one container frame. Verified behavior-identical against the pre-change walk over 4046 inputs (cap boundaries, cycles, depth limits, StrEnum/IntEnum, caller-override subclasses, raising iterators, plus 4000 randomized contexts): snapshots, truncation reasons, raised exceptions, and canonical keys all agree. flatten_and_prune_context, vs this branch / vs main: typical (10 str fields) 5.57 -> 1.45 us -74% / -63% scale (20 str fields) 10.85 -> 2.49 us -77% / -66% mixed types (10) 4.73 -> 1.44 us -69% / -56% list (10 elements) 6.72 -> 2.43 us -64% / -51% adversarial (257x257ch) 186.76 -> 32.46 us -83% / -72% Co-Authored-By: Claude <noreply@anthropic.com>
canonical_context_key runs on the drain worker, so this is off the evaluation
hot path, but it is charged per aggregated bucket and shows up in the
openfeatureflagevaluation-hook-plus-drain-scale benchmark.
Three costs removed, all per context field:
- struct.pack(">Q", n) re-parses the format string on every call. A prebound
Struct(">Q").pack skips that.
- _length_delimited allocated an intermediate prefix+data bytes object that
was immediately concatenated again by the caller. Append the length and the
payload to the parts list separately and let the single join do the work.
- sorted(attrs.keys()) materializes a keys view; sorted(attrs) iterates the
mapping directly for the same result.
_length_delimited had no remaining callers and no test or external reference,
so it is removed rather than left as dead code. The wire encoding is
unchanged, which the existing canonical-key tests cover.
canonical_context_key, vs this branch:
typical (10 fields) 3.50 -> 2.92 us -17%
scale (20 fields) 6.68 -> 5.72 us -14%
mixed types (10) 3.48 -> 2.98 us -14%
list (10 elements) 3.24 -> 2.89 us -11%
Co-Authored-By: Claude <noreply@anthropic.com>
…ce-rb The bounded context snapshot diverged from dd-trace-rb's bounded_flatten in two ways, so the same caller context produced different context.evaluation payloads and different canonical keys — meaning the same user could land in different aggregation buckets depending on the SDK. Depth was off by one. _flatten_bounded checked the depth of the container it was descending from, permitting a retained leaf at five path segments where Ruby caps it at four. The check now compares depth + 1 in both the mapping and sequence branches. Null leaves were retained. Python emitted them as fields and canonicalized them as tag "o" + "None", while Ruby drops nil leaves entirely and encodes a nil value as tag "o" with no bytes. Nulls are now omitted on both inline fast paths and in _validated_leaf, via a sentinel that drops the field without recording a truncation reason (omission is by design, not truncation). The canonical encoder's unreachable null branch is aligned for defense in depth. Verified by running both implementations over identical inputs; all vectors now agree except the known tags[0] vs tags.0 array notation, which is unchanged pending backend-owner approval under FFL-3060. Generated with Claude Code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Commit 75dbf4c ("harden bounded evaluation context handling") removed MAX_VISITED_NODES as a "Python-only traversal guard". That premise was wrong: dd-trace-rb defines MAX_VISITED_NODES and enforces it at four sites in bounded_context_snapshot, so the cap is part of the cross-SDK contract. Without it, the remaining caps do not bound total work. Null leaves are omitted rather than emitted, so output never grows, so the global field cap never trips, and the width caps are only per-container. Traversal cost is therefore width ** (depth + 1) -- about 1.1e12 nodes at the configured caps -- on a caller-supplied mapping, inline on the evaluation path before queue admission. A nested all-null context measured at over two minutes without terminating; it now completes in under a millisecond and reports max_visited_nodes. The budget rides in the existing shared traversal cell (negative means terminal), so the hot loops keep a single check instead of two. Cost is about 8% on typical contexts (1.37 -> 1.48 us) and 17% on a 256-field context. Also replaces test_more_than_old_visited_node_cap_retains_later_scalar, which was added by the removing commit and asserted that a 1,286-node context still retains a later scalar. dd-trace-rb returns {} with max_visited_nodes on that exact input, so the test asserted the opposite of the parity it cited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-rb Python reports unsupported_value for a type-rejected key or leaf; dd-trace-rb drops the same field silently and records nothing, so Ruby operators cannot see type-rejected fields. Verified against bounded_context_snapshot for unsupported value types, unsupported key types, and the nested case: all three return the field-dropped snapshot with an empty reason set. Keep the Python reason. It is additive operator visibility -- the retained key set is identical across SDKs and truncation reasons are local advisory telemetry, so it does not bucket the same caller context differently per language. Documented at the constant so the next reader does not delete it chasing reason-vocabulary parity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Match the conventions the FFE notes have converged on in review: lowercase the scope prefix, which the note's own "openfeature:" tag already implies, and drop "telemetry" as internal vocabulary. Lead with the consequence rather than the mechanism. A customer needs to know that context fields past the limits are omitted from the reported evaluation, and that flag resolution is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…context
The separator decision in _flatten_mapping keyed off len(prefix) rather than
the root flag. Only the root mapping legitimately omits the "." separator, but
a nested container reached through an empty caller key also has an empty
prefix, so it reclaimed the separator's character and dropped its own path
segment.
The result is that {"": {"a": 1}} and {"a": 1} both flattened to {"a": 1}.
Two distinct caller contexts then produced the same canonical key and merged
into one aggregation bucket. That breaks the guarantee the canonical key exists
to provide: the key is deliberately not a hash, so distinct contexts must
always produce distinct keys.
Key the separator and the per-field key budget off root instead. The budget
change is the same bug in the other direction -- a nested container with an
empty prefix would silently reclaim the separator's character from the 256-char
key allowance.
Verified by reverting the fix against the new test: {"": {"a": 1}} flattens to
{'a': 1} instead of {'.a': 1}.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
json.dumps defaults to allow_nan=True, which emits bare NaN, Infinity, and -Infinity tokens. None of those are valid JSON under RFC 8259. A single such value in an evaluation context therefore made the whole request body undecodable at the intake, discarding every event batched with it -- not just the event that carried the value. A caller reaches this without doing anything unusual. numpy.float64 arrives as NaN whenever a context passes through a dataframe with a missing cell, and a computed ratio yields inf on a zero denominator. Guard the float leaf at all three sites that can admit one: * the inline fast path in _flatten_mapping, split out from the int/bool case so integer leaves keep paying nothing for the check * the inline fast path in _flatten_sequence, which has its own leaf handling * _validated_leaf, which is where float subclasses such as numpy.float64 land Each drops the single field and records unsupported_value, matching how every other rejected leaf behaves. The ValueError raised in _validated_leaf is already an enumerated case in the caller's handler, so it drops one field rather than escaping. Also set allow_nan=False in _json_dumps as a backstop. If a non-finite value ever bypasses the snapshot, _encode_payload_event catches ValueError and counts one serialization_error drop, so the cost is one event instead of the payload. Verified by reverting the two inline guards against the new tests: all three parametrized cases and the list-element case fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iter The writer's own drain worker already had DRAIN_WORKER_JOIN_TIMEOUT, but the provider's shutdown path called join() with no argument. PeriodicService.join forwards its timeout straight to Thread.join, so None means block forever. The final flush runs inside the worker's on_shutdown, which talks to the agent. A hung agent connection there would hold process exit open until the orchestrator's kill timer fired, turning a telemetry problem into a visibly slow shutdown for the application. Reuse the writer's existing constant rather than introducing a second timeout, so both joins stay bounded by the same value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…note Two gaps remained after the previous pass. The note described the effect but not the limits, so a customer could not tell whether their context was affected without reading the source. State the cap values: 256 fields, 256-character keys and string values, 256 entries per list or nested object, and 4 levels of nesting. The change to which value *types* are reported was invisible. It is a separate concern from the caps and it is the part that silently drops data a customer was previously getting: Decimal, UUID, and custom objects used to be converted to strings and are now omitted. Move that to upgrade:, where a behavior change that needs action belongs, and give the workaround -- convert to a string in the evaluation context before calling the client. Keep "flag resolution itself is unaffected" in both sections. It is the line that stops a reader worrying this changes which variant they receive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Set the eight absolute execution-time gates from corrected measurements on the pinned x86 runner. Each threshold keeps approximately 10-16% headroom over GitLab job 1992838683. Co-Authored-By: Claude <noreply@anthropic.com>
Adds the accepted design for hashing targeting_key and omitting context.evaluation by default in the EVP flagevaluation track, gated by the new top-level UFC boolean observeFullEvaluationData. Mirrors the Java pilot (dd-trace-java#12042) and the Go reference (dd-trace-go#5151). Co-Authored-By: Claude <noreply@anthropic.com>
Twelve tasks, TDD-shaped, mirroring the design spec section-by-section. Each task is bite-sized (2-5 minutes per step), fully self-contained code, with the exact scripts/run-tests invocation and expected pass/fail output. Co-Authored-By: Claude <noreply@anthropic.com>
…contract Unsalted SHA-256 over raw UTF-8 bytes with the sha256_ prefix. Cross-SDK contract: every SDK reproduces the canonical vector byte-for-byte so hashed targeting keys join across languages. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
…rule Follows CLAUDE.md 'Add imports at the top of files, never inside functions'. No functional change. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
Previously the test source relied on raw multi-byte UTF-8 sequences for the NFC-vs-NFD non-normalization proof. Editor tools that normalize UTF-8 on save could silently collapse them. Explicit \u escapes remove the risk. No behavior change. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
…pshot _FfeSnapshot lets a reader observe a consistent (config, consent) pair even if Remote Config swaps the snapshot mid-evaluation. Legacy _set_ffe_config callers pass a bare Configuration and get consent-off (fail closed). FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
- AIDEV-NOTE on _get_ffe_config warns new callers to use _get_ffe_snapshot instead, so grabbing just the config cannot reintroduce the consent race. - Symmetric try/finally teardown in test_default_is_none matches the other three TestFfeSnapshot tests. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
The consent value sits at the top level of the UFC, a sibling of environment, never nested inside it. Absent, null, and wrong-typed values all fail closed to False. Stored alongside the parsed native Configuration as an atomic snapshot. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
The provider snapshots consent from the FFE snapshot each evaluation runs against and stamps it on flag_metadata under the cross-SDK key observe_full_evaluation_data on every return path. Downstream must read consent from metadata, never from live config. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
DataDogProvider does not read env vars at class-definition time — it reads them in __init__ via a fresh OpenFeatureConfig() instance, so monkeypatch still works after hoisting. Complies with CLAUDE.md "no in-function imports". FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
…hen off The EVP hook reads observe_full_evaluation_data from FlagResolutionDetails metadata (never from live config), fails closed on missing or non-bool values, and skips evaluation-context capture entirely when consent is off. Prevents PII attribute dicts from living in the writer queue. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
Full-tier bucket key carries observe_full_evaluation_data so mixed-consent evaluations never merge. Consent-off aggregations drop context from both key and entry (bucket key must carry only dimensions that survive serialization). Fast-path merge AND-folds consent as defense in depth. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
The AND-fold branch in _aggregate is unreachable today because consent is part of the full-tier key. The test previously suggested it was guarding the branch reachability; in fact it guards Python's "consent-off wins" semantics. Rename and update the docstring to be honest about what the test documents. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
FlagEvaluationWriter.periodic() now branches on entry.observe_full_evaluation_data when serializing full-tier events. Consent-off: hashed sha256_ targeting_key, context field absent. Consent-on: raw targeting_key and context.evaluation. Hashing runs once per bucket at flush cadence, matching dd-trace-java#12042 and dd-trace-go#5151. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
The Java pilot shipped a version of this feature that read consent from live config at flush time; system-tests caught it and unit tests had not. This test evaluates under one consent value, swaps the RC snapshot before flush, and asserts the flushed event still reflects the value at evaluation time. Both directions covered. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
DoLog: for each consent value, the flushed-event JSON is identical across
do_log ∈ {True, False} (excluding wall-clock fields). Guards against a
future refactor that gates PII behavior on DoLog, which the RFC forbids.
Kill switch: DD_FLAGGING_EVALUATION_COUNTS_ENABLED=false disables the EVP
track entirely regardless of observeFullEvaluationData. Kill switch always
wins over consent. FFL-2964.
Co-Authored-By: Claude <noreply@anthropic.com>
Documents the default-hashing behavior, the sha256_ wire prefix, the observeFullEvaluationData per-environment opt-in, and the unchanged DD_FLAGGING_EVALUATION_COUNTS_ENABLED kill switch. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
Follows CLAUDE.md 'Add imports at the top of files, never inside functions'. Import as _fw_module because the test mutates GLOBAL_CAP on the module object. No functional change. FFL-2964. Co-Authored-By: Claude <noreply@anthropic.com>
Generated with Claude Code
Generated with Claude Code
…racts Two functions named hash_targeting_key existed in ddtrace/internal/openfeature/, one in _flageval_pii.py returning the 71-char sha256_-prefixed flagevaluation wire value and one in _span_enrichment.py returning the bare 64-char span-tag value. Both are frozen cross-SDK contracts and both have signature (str) -> str, so importing the wrong one produced no type error and silently emitted the wrong value, breaking the unique-subject join this work exists to preserve. Extract the digest into targeting_key_digest and have both call it. Public names and behavior are unchanged: the span-enrichment helper still returns a bare digest and still raises on non-string input, and hash_targeting_key still guards its input and returns "" for empty, non-string, or non-UTF-8-encodable values. Also record in the docstring that the digest is a pseudonym rather than an anonymization, since a deterministic cross-SDK join forbids a salt. The new import points at a leaf module that imports only hashlib, so it adds no cycle; the detector still reports the same 3 pre-existing ones. Co-Authored-By: Claude <noreply@anthropic.com>
Use the approved consent metadata key, preserve explicit empty targeting keys, omit malformed keys, and key protected aggregation by the serialized hash identity. Generated with Claude Code
vjfridge
force-pushed
the
vickie/ffl-2964-protecting-pii-in-py-flagevaluations-track
branch
from
August 28, 2026 04:40
437f715 to
a3f68d2
Compare
Base automatically changed from
vickie/ffl-3060-bounded-evaluation-context
to
main
September 8, 2026 20:04
Contributor
Author
|
Closed in favor of #20354 |
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Sep 22, 2026
## Description Implements protected-by-default OpenFeature `flagevaluation` telemetry. - Hash valid non-empty targeting keys as `sha256_<lowercase hex>` over exact UTF-8 bytes. Preserve explicit empty strings; omit missing or invalid keys without dropping evaluation counts. - Omit evaluation context by default. Only exact root `observeFullEvaluationData: true` permits raw valid targeting keys and a bounded immutable context snapshot. - Emit only approved OpenFeature error codes in **both** privacy modes. Full-data consent never permits raw evaluator/exception text. - Bind consent to the configuration used for the evaluation; carry it through private metadata, queueing, aggregation, and serialization. Full-tier identity includes consent; degraded identity excludes it and always omits key/context. Both tiers retain defensive AND-folds. - Preserve asynchronous bounded delivery, existing OTel metrics/exposures, `DoLog` independence, the kill switch, and the span helper's bare digest format. - Count invalid-key omissions separately from dropped rows and caught hook failures. Telemetry-sink failures cannot interrupt delivery. Supersedes the stale implementation in #19554 and builds on #19881. Context: [FFL-2964](https://datadoghq.atlassian.net/browse/FFL-2964), [FFL-2529](https://datadoghq.atlassian.net/browse/FFL-2529), [approved PII RFC](https://docs.google.com/document/d/19VIf4B9p-zsAL2uWSvdzil59DZlGjQm4yudAMwJBqLs/edit), [shared implementation guidance](DataDog/ffe-codegen-tools#9). ## Testing Validation refreshed on 2026-09-18 for `1bfd2eab9d36c935999c261bbade7c37a5f4f9cc`. - OpenFeature: **848 passed, 2 skipped** in each CPython 3.11 dependency selection (pinned 0.8 and latest). - Six isolated fault-injection checks detect bypassed context guards, queue aliasing, missing full-tier folding, unsafe serializer errors, and missing hook error-code forwarding. - Full mypy: **1,139 source files passed**. Targeted style, spelling, security, error-log, suitespec, and SLO ownership checks passed. The broad `scripts/lint checks` entrypoint hit generated cache files in its Cython scan; source-scoped checks were run separately. - Exact ARM64 wheels compared with main `63de8eb2d675e2ebe8428f058b01249674ab5079`: dogfooding **18/18 HTTP 200 and 18 evaluations on both sides**, no corrupt rows. Installed candidate writer checksum matches source. - Installed-wheel error-only canaries: candidate passes both consent modes and both tiers, preserving default counts; main leaks the canary in all four cases. - System tests: candidate **11 focused passed; 46 full passed, 2 skipped, 4 existing XPASS**. Main full has the two expected privacy failures. Its initial focused degradation failure passed in the full run and isolated rerun; candidate passed first try. - All ten benchmark means meet SLOs. Protected enqueue/aggregation measure **18.27/12.20 µs**, with **24/16 µs** thresholds. Legacy full-data scenarios retain their original work and limits. Full aggregate-typical is **28.75 vs 27.65 µs (+3.96%)**, close to its unchanged 29 µs limit; do not interpret protected improvements as same-mode optimization. - Five review lenses plus an adversarial pass and a read-only Codex sweep completed. The SRE finding about telemetry failure losing a flush was fixed and rechecked. No remaining actionable local findings. See the latest two **Note to self** comments for harness commits, wheel hashes, commands, and first-run/retry counts. New remote CI must finish independently of this local evidence. ## Risks / Additional Notes This intentionally changes the default wire shape. Deterministic unsalted hashes preserve cross-SDK subject joins but remain susceptible to dictionary attacks for low-entropy keys. Protected mode does not read/copy context attributes; hashing remains in the background worker. `flagevaluation.targeting_key.omitted{reason:invalid}` counts rejected inputs that reach aggregation, not transmitted rows or dropped evaluations. `flagevaluation.hook.errors` counts caught hook failures separately from writer-owned drops. All 16 review threads have replies. Three contract/design discussions remain open for acknowledgement: code-only errors in full mode, retaining the degraded defensive fold, and the approved `__dd_` metadata prefix. Empty-string preservation already matches the approved amendment and remains unchanged. [FFL-2964]: https://datadoghq.atlassian.net/browse/FFL-2964?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: vickie.fridge <vickie.fridge@datadoghq.com>
brettlangdon
pushed a commit
that referenced
this pull request
Sep 23, 2026
## Description Implements protected-by-default OpenFeature `flagevaluation` telemetry. - Hash valid non-empty targeting keys as `sha256_<lowercase hex>` over exact UTF-8 bytes. Preserve explicit empty strings; omit missing or invalid keys without dropping evaluation counts. - Omit evaluation context by default. Only exact root `observeFullEvaluationData: true` permits raw valid targeting keys and a bounded immutable context snapshot. - Emit only approved OpenFeature error codes in **both** privacy modes. Full-data consent never permits raw evaluator/exception text. - Bind consent to the configuration used for the evaluation; carry it through private metadata, queueing, aggregation, and serialization. Full-tier identity includes consent; degraded identity excludes it and always omits key/context. Both tiers retain defensive AND-folds. - Preserve asynchronous bounded delivery, existing OTel metrics/exposures, `DoLog` independence, the kill switch, and the span helper's bare digest format. - Count invalid-key omissions separately from dropped rows and caught hook failures. Telemetry-sink failures cannot interrupt delivery. Supersedes the stale implementation in #19554 and builds on #19881. Context: [FFL-2964](https://datadoghq.atlassian.net/browse/FFL-2964), [FFL-2529](https://datadoghq.atlassian.net/browse/FFL-2529), [approved PII RFC](https://docs.google.com/document/d/19VIf4B9p-zsAL2uWSvdzil59DZlGjQm4yudAMwJBqLs/edit), [shared implementation guidance](DataDog/ffe-codegen-tools#9). ## Testing Validation refreshed on 2026-09-18 for `1bfd2eab9d36c935999c261bbade7c37a5f4f9cc`. - OpenFeature: **848 passed, 2 skipped** in each CPython 3.11 dependency selection (pinned 0.8 and latest). - Six isolated fault-injection checks detect bypassed context guards, queue aliasing, missing full-tier folding, unsafe serializer errors, and missing hook error-code forwarding. - Full mypy: **1,139 source files passed**. Targeted style, spelling, security, error-log, suitespec, and SLO ownership checks passed. The broad `scripts/lint checks` entrypoint hit generated cache files in its Cython scan; source-scoped checks were run separately. - Exact ARM64 wheels compared with main `63de8eb2d675e2ebe8428f058b01249674ab5079`: dogfooding **18/18 HTTP 200 and 18 evaluations on both sides**, no corrupt rows. Installed candidate writer checksum matches source. - Installed-wheel error-only canaries: candidate passes both consent modes and both tiers, preserving default counts; main leaks the canary in all four cases. - System tests: candidate **11 focused passed; 46 full passed, 2 skipped, 4 existing XPASS**. Main full has the two expected privacy failures. Its initial focused degradation failure passed in the full run and isolated rerun; candidate passed first try. - All ten benchmark means meet SLOs. Protected enqueue/aggregation measure **18.27/12.20 µs**, with **24/16 µs** thresholds. Legacy full-data scenarios retain their original work and limits. Full aggregate-typical is **28.75 vs 27.65 µs (+3.96%)**, close to its unchanged 29 µs limit; do not interpret protected improvements as same-mode optimization. - Five review lenses plus an adversarial pass and a read-only Codex sweep completed. The SRE finding about telemetry failure losing a flush was fixed and rechecked. No remaining actionable local findings. See the latest two **Note to self** comments for harness commits, wheel hashes, commands, and first-run/retry counts. New remote CI must finish independently of this local evidence. ## Risks / Additional Notes This intentionally changes the default wire shape. Deterministic unsalted hashes preserve cross-SDK subject joins but remain susceptible to dictionary attacks for low-entropy keys. Protected mode does not read/copy context attributes; hashing remains in the background worker. `flagevaluation.targeting_key.omitted{reason:invalid}` counts rejected inputs that reach aggregation, not transmitted rows or dropped evaluations. `flagevaluation.hook.errors` counts caught hook failures separately from writer-owned drops. All 16 review threads have replies. Three contract/design discussions remain open for acknowledgement: code-only errors in full mode, retaining the degraded defensive fold, and the approved `__dd_` metadata prefix. Empty-string preservation already matches the approved amendment and remains unchanged. [FFL-2964]: https://datadoghq.atlassian.net/browse/FFL-2964?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: vickie.fridge <vickie.fridge@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change protects PII in the EVP
flagevaluationtrack fordd-trace-py. Server-side flag evaluations, by default, do not send raw subject identifiers or evaluation context to Datadog.Server SDK upgrades must not send subject PII to Datadog by accident. 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. The PR makes full data collection an explicit per-environment opt-in.
Tracked by FFL-2964, under the server-SDK fan-out FFL-2784. The Java pilot is dd-trace-java#12042. The direct peer is dd-trace-go#5151.
What does this PR do?
A new UFC boolean
observeFullEvaluationDataselects the wire shape.observeFullEvaluationDatatargeting_keyon the wirecontext.evaluationon the wiretruefalseor absent (the default)sha256_+ 64-char lowercase hex (71 chars total)User impact:
flagevaluationstrack. The environment must opt-in first.targeting_keyas the raw subject must accept thesha256_prefix on the default path.DD_FLAGGING_EVALUATION_COUNTS_ENABLEDkill switch is not changed. This kill switch disables the whole track when set tofalse.Key design points:
observeFullEvaluationDatais at the root of the UFC, a sibling ofenvironment. The read path in_native.pygets the value directly from the Remote Config dict before the native parser sees it. Absent, explicitnull, and wrong-typed values all fail closed tofalse.observe_full_evaluation_data. No component downstream of the evaluator reads live configuration. A Remote Config update between evaluation and flush cannot apply another environment's policy. The Java pilot hit this bug. A regression test guards both directions._FfeSnapshotis aNamedTuplethat bundles the native config and its consent value. A single module-level reference makes the pair visible atomically.No new tracer environment variables. No dependency changes. No changes to the Rust native layer.
Testing
Unit tests: done.
New tests in
tests/openfeature/test_flageval_pii.py:jane.doe@datadoghq.com→sha256_b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b. Byte-identical to dd-trace-java'sULeb128EncoderTestand dd-trace-go'sTestHashTargetingKeyCanonicalVector.\uescapes so editor auto-normalization cannot collapse them.observeFullEvaluationDataon theenvironmentobject is not read. This blocks the FFL-2784 placement drift."true","false",1,0,[],{}, andNoneall set consent tofalse.Nonemetadata values all set consent tofalse.concern:consent-off-bucket-keying.doLog ∈ {True, False}. The RFC requires this proof.DD_FLAGGING_EVALUATION_COUNTS_ENABLED=falseprevents construction of the writer and hook, regardless ofobserveFullEvaluationData.System-tests (L3): out of scope for this PR. Python's
system-testsmanifest still readstests/ffe/test_flag_eval_evp.py: missing_feature (FFL-2446). Base track dd-trace-py#18613 merged without a manifest flip. Resolution needs a companionsystem-testsPR that turns the file on and marks the threeTest_FFE_EVP_Flagevaluation_ObserveFullData_*rows asmissing_feature (FFL-2964). This is a known gap tracked in the source-cluster README.L2 (dogfooding): not run. No companion
ffe-dogfoodingchange in this PR.Risks
observeFullEvaluationDatais defensive by default. An environment that expects to see the rawtargeting_keyin queries must opt-in through the flagging app UI. Customers that readtargeting_keyon the default path seesha256_-prefixed values. Dashboards need updates.TestHashTargetingKey::test_does_not_normalizeguards this.system-testscoverage in CI yet. Python's manifest keeps the file off. This PR does not activate it. A separate PR must activate the file and mark only the three PII-specific rows asmissing_feature (FFL-2964).Additional Notes
hashlib.sha256. Thedatadog-ffecrate atlibdatadog@v38.0.0still handles evaluation, and no Rust change is required.libdatadog#2117is a PHP-only change.rfc:gdoc:19VIf4B9p-zsAL2uWSvdzil59DZlGjQm4yudAMwJBqLs. Java pilot: dd-trace-java#12042. Direct peer: dd-trace-go#5151.🤖 Generated with Claude Code