fix(processing): Use correct comparison interval #73062
Conversation
github-actions
bot
added
the
Scope: Backend
| @@ -48,7 +48,7 @@ def __repr__(self): | |||
|
|
|||
|
|
|||
| class DataAndGroups(NamedTuple): | |||
| data: EventFrequencyConditionData | None | |||
| data: EventFrequencyConditionData | |||
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
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.
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.
^ 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.
+1 on a test here, we need something that reproduces this bug without the fix
There was a problem hiding this comment.
ok wrote the test and confirmed it fails with the previous code but works now
schew2381
force-pushed
the
seiji/fix/use-correct-comparison-interval
branch
from
b6cd6bc
to
6d559a7
Compare
| comparison_interval = condition_inst.intervals[ | ||
| condition_data.get("comparisonInterval", "5m") |
There was a problem hiding this comment.
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.
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