-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(eap): Start unifying EAP utils #103182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
2e9eeaf to
ea0d4de
Compare
ea0d4de to
25751c7
Compare
| "sentry.utils.eap_producer", | ||
| Topic.SNUBA_ITEMS, | ||
| exclude_config_keys=["compression.type", "message.max.bytes"], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Producer Config Change Impacts Replay Message Behavior
The unified eap_items_producer adds exclude_config_keys=["compression.type", "message.max.bytes"] which changes behavior for code previously using sentry.replays.lib.kafka.eap_producer that didn't exclude these config keys. This could affect message compression and size limits for replay trace items.
25751c7 to
d6b58d0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103182 +/- ##
===========================================
- Coverage 80.67% 80.67% -0.01%
===========================================
Files 9241 9242 +1
Lines 393627 393598 -29
Branches 25053 25053
===========================================
- Hits 317543 317518 -25
+ Misses 75622 75618 -4
Partials 462 462 |
We've got a couple redundant-implementations of EAP utils, most notably the `encode_value` function. This PR unifies these into `sentry.utils.eap`. Eventually, I'd like this package to handle all of the encoding/ details so users can easily write/query EAP without needing to worry about things.
d6b58d0 to
a3b6609
Compare
| elif dump_arrays and isinstance(value, (list, tuple, dict)): | ||
| return AnyValue(string_value=orjson.dumps(value).decode()) | ||
| elif expand_arrays and isinstance(value, dict): | ||
| return AnyValue(**value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Conflicting Flags Silently Ignore Array Expansion
When both dump_arrays=True and expand_arrays=True are passed with a dict value, the dump_arrays check at line 49 takes precedence over the expand_arrays check at line 51, causing expand_arrays to be silently ignored for dicts. This creates ambiguous behavior where two mutually exclusive flags can both be set but only one is honored.
We've got a couple redundant-implementations of EAP utils, most notably the
encode_valuefunction.This PR unifies these into
sentry.utils.eap.