fix(rpc): Fix more of the events tests #81934
Conversation
wmak
commented
Dec 10, 2024
wmak
commented
- This fixes a bug with searching on fields with contexts where we weren't passing context
- eg. if you did project:some-slug, the context to transform wasn't being passed
- This fixes a bug where we weren't searching sentry_tags correctly
- This fixes a bug with searching on fields with contexts where we
weren't passing context
- eg. if you did project:some-slug, the context to transform wasn't
being passed
- This fixes a bug where we weren't searching sentry_tags correctly
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #81934 +/- ##
==========================================
+ Coverage 80.36% 80.42% +0.05%
==========================================
Files 7277 7284 +7
Lines 321426 321308 -118
Branches 20973 20922 -51
==========================================
+ Hits 258326 258421 +95
+ Misses 62689 62484 -205
+ Partials 411 403 -8 |
wmak
changed the title
| @@ -430,11 +435,21 @@ def device_class_context_constructor(params: SnubaParams) -> VirtualColumnContex | |||
| ) | |||
|
|
|||
|
|
|||
| def module_context_constructor(params: SnubaParams) -> VirtualColumnContext: | |||
| value_map = {key: key for key in SPAN_MODULE_CATEGORY_VALUES} | |||
There was a problem hiding this comment.
What's the point of this map if it just maps to itself?
There was a problem hiding this comment.
Need the transform to other for unknown values (need to update the proto to change the default value which is why there's xfailed tests)
src/sentry/search/eap/spans.py
Outdated
| elif len(terms) == 0: | ||
| return None, [] |
There was a problem hiding this comment.
nit: maybe move this condition to the beginning?
| resolved_rhs = self._resolve_boolean_conditions(rhs) if rhs else None | ||
| resolved_lhs, contexts_lhs = self._resolve_boolean_conditions(lhs) | ||
| resolved_rhs, contexts_rhs = self._resolve_boolean_conditions(rhs) | ||
| contexts = contexts_lhs + contexts_rhs |
There was a problem hiding this comment.
Is there a need to dedupe contexts here?
There was a problem hiding this comment.
My thought is that its on the caller to call the dedupe function once, instead of it potentially being automatically being called multiple times within the Resolver, open to dedupe here as well if you think its important
| if column.startswith("sentry_tags"): | ||
| field = f"sentry.{field}" |
There was a problem hiding this comment.
Is this for backwards compatibility? To support syntax like sentry_tags[foo]?
There was a problem hiding this comment.
Yeah, i can xskip tests if we want to remove that though, i'm just trying to get CI passing so we have 1:1 functionality right now
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
- This fixes a bug with searching on fields with contexts where we weren't passing context - eg. if you did project:some-slug, the context to transform wasn't being passed - This fixes a bug where we weren't searching sentry_tags correctly
