-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(processing): Use correct comparison interval #73062
Conversation
@@ -48,7 +48,7 @@ def __repr__(self): | |||
|
|||
|
|||
class DataAndGroups(NamedTuple): | |||
data: EventFrequencyConditionData | None | |||
data: EventFrequencyConditionData |
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.
Small typing fix, this is always set
@@ -142,7 +142,9 @@ def get_condition_group_results( | |||
return None | |||
|
|||
_, duration = condition_inst.intervals[unique_condition.interval] | |||
comparison_interval = condition_inst.intervals[unique_condition.interval][1] | |||
comparison_interval = condition_inst.intervals[ | |||
condition_data.get("comparisonInterval", "5m") |
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.
The default is not really necessary b/c all percent conditions should have this set, and we only use condition_interval in get_rate_bulk
if the condition is a percent condition in the first place.
However, it's good to be safe and match what we have for regular processing
comparison_interval_option = self.get_option("comparisonInterval", "5m") |
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.
yeah, nice to include the default. i think extra credit would be to have that default set as a constant. then we can reuse it for the API or other areas of code in the future.
comparison_interval = condition_inst.intervals[ | ||
condition_data.get("comparisonInterval", "5m") | ||
][1] |
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.
It'd be nice to include a unit test for this to make sure we don't run into the issue again in the future.
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.
^ this and/or a test with a rule where the comparison interval and the comparison value are different
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.
+1 on a test here, we need something that reproduces this bug without the fix
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.
ok wrote the test and confirmed it fails with the previous code but works now
b6cd6bc
to
6d559a7
Compare
comparison_interval = condition_inst.intervals[ | ||
condition_data.get("comparisonInterval", "5m") |
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.
So just keep in mind here that condition_data
comes from the first condition we see for that unique_condition
. So since we don't currently unique on different comparisonInterval
, this will still produce incorrect results if we have two rules with conditions like
% change of events in the last hour compared to 1 day ago
% change of events in the last hour compared to week day ago
Whichever of these conditions is processed first will have the comparison interval set, and the second will incorrectly compare to the previous conditions comparisonInterval
.
I assume you're going to fix this bug separately so I'm ok with merging, but just wanted to make sure you're aware.
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.
oh yes this was just the easier of the 2 bugs to tackle first
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #73062 +/- ##
==========================================
+ Coverage 76.93% 78.00% +1.07%
==========================================
Files 6614 6626 +12
Lines 295333 295804 +471
Branches 50876 50944 +68
==========================================
+ Hits 227200 230738 +3538
+ Misses 61741 58773 -2968
+ Partials 6392 6293 -99
|
The `comparison_interval` time is inclusive so the test needs to btwn 5 and 10 min. I checked and this test still fails without #73062 so all good
From Slack:
Comparison conditions are using the incorrect comparison interval (comparison interval is "how far back should we go". interval is "what period of time should we query")
Basically when deciding how far back to go for the past session, we were using 1 instead of 2
Additional PR to fix test bug: #73235
Fixes ALRT-100