fix(processing): Separate second query of percentage comparison #73099
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #73099 +/- ##
==========================================
- Coverage 78.01% 78.00% -0.01%
==========================================
Files 6630 6632 +2
Lines 295916 296082 +166
Branches 50971 50989 +18
==========================================
+ Hits 230848 230967 +119
- Misses 58732 58778 +46
- Partials 6336 6337 +1
|
| if len(query_values) == 2: | ||
| calculated_value = percent_increase(calculated_value, query_values[1]) | ||
|
|
||
| target_value = float(condition_data["value"]) |
There was a problem hiding this comment.
Previously we converted this to a string then float, not sure if that's important though
There was a problem hiding this comment.
@ceorourke do you remember why we originally did that here?
There was a problem hiding this comment.
I think it was just to match this https://github.com/getsentry/sentry/blob/master/src/sentry/rules/conditions/event_frequency.py - what's the data type initially? A float?
There was a problem hiding this comment.
It's a string but I think we can just convert that directly, like here
schew2381
force-pushed
the
seiji/fix/separate-percent-comparison-queries
branch
from
e96bf12
to
accf24d
Compare
schew2381
force-pushed
the
seiji/fix/separate-percent-comparison-queries
branch
from
c62a186
to
edbfdf9
Compare
| """ | ||
| end = timezone.now() - interval | ||
| if comparison_interval: | ||
| end = end - comparison_interval |
There was a problem hiding this comment.
If we're already passing in the end - why do we pass in the offset?
Can we just calculate the new end when invoking this helper?
There was a problem hiding this comment.
We could but I liked having this function handle all the time calculations instead of doing it in get_rate_bulk and get_rate
| with self.disable_consistent_snuba_mode(duration): | ||
| result = self.query(event, start, end, environment_id=environment_id) | ||
| if comparison_type == ComparisonType.PERCENT: | ||
| # TODO: Figure out if there's a way we can do this less frequently. All queries are | ||
| # automatically cached for 10s. We could consider trying to cache this and the main | ||
| # query for 20s to reduce the load. | ||
| start, end = self.get_comparison_start_end(comparison_interval, duration) | ||
| start, end = self.get_comparison_start_end( |
There was a problem hiding this comment.
RE: comments above
1 - lets make this kwargs
2 - lets pre-calucate the end here
| query, but the second percent query is separate. | ||
| """ | ||
|
|
||
| def ordered_callthrough(descending=False): |
There was a problem hiding this comment.
🤔 can we call this mock_get_condition_group? or toss side_effect/callback somewhere in teh name of this?
There was a problem hiding this comment.
Also - does this need to be a separate helper?
Why are we modifying the args being passed into this method? This means we aren't properly capturing how the implementation would behave.
This feels smelly 🤔
There was a problem hiding this comment.
We're modifying the order of the list of rules being passed into the function. We need to do this to test the behavior when count and percent conditions can share a query, so that ordering doesn't matter.
Originally they come in a random order, so we test for both orderings (count first then percent and vice versa)
schew2381
force-pushed
the
seiji/fix/separate-percent-comparison-queries
branch
from
4a5c976
to
edbfdf9
Compare
schew2381
force-pushed
the
seiji/fix/separate-percent-comparison-queries
branch
from
c42b4d8
to
b4fc09e
Compare
Fixes ALRT-101