fix: Omit non-symbol context attributes and protect against cyclic values - #423
Draft
kinyoklion wants to merge 1 commit into
Draft
fix: Omit non-symbol context attributes and protect against cyclic values#423kinyoklion wants to merge 1 commit into
kinyoklion wants to merge 1 commit into
Conversation
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
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.
Summary
Attribute references store every path component as a symbol, so a context attribute with a non-symbol name (for example
"street" => ...instead ofstreet:) can never be addressed: it is invisible to evaluation and bucketing, and a private-attribute reference can never redact it. Today the event filter still copies such attributes into analytics event payloads, with one extra defect: a top-level string-keyed attribute is emitted with anullvalue, because the attribute name comes from raw storage but the value is fetched throughget_value, which symbolizes the name and misses the entry.This PR makes
ContextFilter:_meta.redactedAttributes, since no reference removed them. This also removes the top-levelnullemission described above.AtomicBooleansince the formatter runs on flush workers), so the condition is discoverable without log spam.SystemStackError. The filter now tracks the chain of hashes from the attribute root down each branch and omits a value that points back into its own chain. The same object appearing in different branches (a DAG) is still copied everywhere it appears.Behavior change note: string-keyed attribute data that was previously sent in event payloads (intact when nested,
nullat top level) is now omitted.Performance: same-process A/B against
mainon representative context shapes shows 0.99-1.05x on flat and redact-all paths and ~0.90x on deeply nested contexts (the cycle-protection chain costs one small array per nested hash).Regression tests cover: top-level and nested omission, subtree omission, no
redactedAttributesreporting for omitted names, composition with private-attribute redaction, log-once behavior, the previousSystemStackErroron cycles, and DAG preservation. All new tests fail against the previous implementation.