feat(openfeature): bound evaluation context reported with flag evaluations - #19881
Open
vjfridge wants to merge 24 commits into
Open
feat(openfeature): bound evaluation context reported with flag evaluations#19881vjfridge wants to merge 24 commits into
vjfridge wants to merge 24 commits into
Conversation
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
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 2 jobs - 2 passed on retry 🔗 Commit SHA: 2443fd9 | Docs | View more details | Give us feedback! |
Dependency direction analysis
|
vjfridge
added
the
changelog/no-changelog
BenchmarksBenchmark execution time: 2026-08-28 05:25:18 Comparing candidate commit 2443fd9 in PR branch Found 4 performance improvements and 6 performance regressions! Performance is the same for 574 metrics, 10 unstable metrics, 1 known flaky benchmarks, 17 flaky benchmarks without significant changes.
|
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
changed the title
vjfridge
changed the title
Generated with Claude Code
vjfridge
changed the title
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>
vjfridge
removed
the
changelog/no-changelog
…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>
vjfridge
changed the title
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>
The scenario was added with nine configs but no SLO entries, so it ran and reported without gating anything. A regression on the evaluation hot path would have landed silently. Thresholds are the higher of two local runs plus the ~10% margin that .gitlab/benchmarks/microbenchmarks.md prescribes. Both runs agreed within about 6% on every config, so the suite is stable enough to gate: aggregate-typical 7.2 / 7.3 us hook-enqueue-typical 8.3 / 8.6 us hook-enqueue-scale 10.4 / 11.8 us aggregate-scale 13.7 / 14.4 us hook-plus-drain-scale 29.3 / 29.9 us hook-enqueue-stress 64.0 / 67.7 us hook-enqueue-bounded-adversarial 80.5 / 83.0 us aggregate-stress 142.6 / 144.6 us Measured on ARM Docker, so the absolute numbers will shift on the pinned x86 runners. The first pipeline has no baseline containing the scenario, so a mis-set threshold surfaces as a visible gate result before it can block anyone; widen the specific entry if one trips. Verified with scripts/gen_gitlab_config.py that the generator lists the scenario in a matrix entry and that all eight thresholds survive its filter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drain every complete cycle and any calibrated residual batch so pyperf loop-count differences cannot change how much aggregation each side measures. Remove duplicate queue-full polling from the timed path. Co-Authored-By: Claude <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>
Author
Note to self: ffe-dogfoodingTested
As expected, PII protection is not complete yet: targeting keys remained raw and The initial source build exceeded the Docker memory limit and passed after retrying with one build job. Gunicorn also logged two pre-existing Result: no non-PII blocker found. The Python SDK behavior covered by this PR worked as expected. |
Author
Note to self: system-testsTested system-tests ./build.sh python -w flask-poc
./run.sh FEATURE_FLAGGING_AND_EXPERIMENTATION --library python \
-F tests/ffe/test_flag_eval_evp.pyResults:
The initial parallel wheel build exceeded Docker memory; rebuilding with one compilation job passed. No non-PII test failed. Result: safe to ship for the bounded-context and other non-PII behavior validated here. The known PII-protection gap remains. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2443fd95e5
ℹ️ 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
+7
to
+8
| and nesting is limited to 4 levels. Context data beyond these limits is omitted from the | ||
| reported evaluation; flag resolution itself is unaffected. |
There was a problem hiding this comment.
The implementation also stops after 1,280 inspected nodes (MAX_VISITED_NODES), but this note says context is omitted only beyond the listed field, width, length, and depth limits. For example, several permitted 256-entry containers containing null or unsupported leaves can consume that budget and cause a later valid field to be omitted even though fewer than 256 fields would be reported and every listed limit is respected. Include the visited-node cap so customers can understand this reporting behavior.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
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
Implements FFL-3060 by moving OpenFeature evaluation-context ownership to a bounded, immutable snapshot created synchronously before queue admission.
The snapshot boundary:
tags[0], while omittingNoneleaves;NaN,Infinity) asunsupported_value, sincejson.dumpswould otherwise emit bare tokens that are invalid JSON under RFC 8259;unsupported_value; snapshot traversal or canonicalization failures fail closed to empty context plussnapshot_error, without calling arbitrary conversion hooks;The writer also separates fork-safe queue, lifecycle, counter, and aggregation state; keeps
pre_queue_overflow,queue_overflow, andclosedaccounting distinct; and makes shutdown linearizable with accepted queue events and counters included in the final drain. Both the drain worker's join and the provider's join are bounded byDRAIN_WORKER_JOIN_TIMEOUT.Benchmark profiles and deterministic regression coverage are included, with SLO entries for all eight configs.
Testing
Repository-approved product validation on commit
3bbe2021ff:scripts/run-tests --venv 18a4a8d -- -sscripts/run-tests --venv 17b66d6scripts/lint suitespec-checkandscripts/lint error-log-checkpassedgit diff origin/main..HEAD --checkpassedThe benchmark-only follow-up commits were validated on the current head:
scripts/lint checkspassed all 24 checks after the deterministic benchmark and SLO changesscripts/gen_gitlab_config.pyconfirms the benchmark scenario is gated and all eight SLO thresholds survive the generator's filterhook-plus-drain-scaleat 64.62 us for the merge-base baseline and 30.66 us for the candidateThe three guards in this round were mutation-tested by reverting each one and confirming that the corresponding tests fail:
{"": {"a": 1}}flatten to{'a': 1}instead of{'.a': 1}Risks
The corrected x86 benchmark measurements and calibrated absolute SLOs are:
aggregate-typicalhook-enqueue-typicalhook-enqueue-scaleaggregate-scalehook-plus-drain-scalehook-enqueue-stresshook-enqueue-bounded-adversarialaggregate-stresshook-plus-drain-scalenow drains every complete 2,500-event cycle and the final partial cycle. This prevents independent pyperf loop calibration from making the baseline and candidate perform different amounts of aggregation. The corrected x86 run measured 215.990 us for the baseline and 103.885 us for the candidate, so the candidate is approximately 51.9% faster. This supersedes the apparent 63.5-65.1% regression reported for commit3bbe2021ff.The Riot Docker setup prints a worktree-path Git warning during preparation, but the selected suites complete successfully.
Additional Notes
FFL-3118 and FFL-3119 remain separate dependencies and are not modified here.