-
Notifications
You must be signed in to change notification settings - Fork 254
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
Support for aligning Slack message grouping intervals #1473
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,12 +91,21 @@ def validate_exactly_one_defined(cls, values: Dict): | |
class GroupingParams(BaseModel): | ||
group_by: GroupingAttributeSelectorListT = ["cluster"] | ||
interval: int = 15*60 # in seconds | ||
aligned: bool = False | ||
notification_mode: Optional[NotificationModeParams] | ||
|
||
@root_validator | ||
def validate_notification_mode(cls, values: Dict): | ||
if values is None: | ||
return {"summary": SummaryNotificationModeParams()} | ||
def validate_interval_alignment(cls, values: Dict): | ||
if values["aligned"]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if values is none? wont this throw? i would handle that case first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's marked as required, so the only possible values should be |
||
if values["interval"] < 24 * 3600: | ||
if (24 * 3600) % values["interval"]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure i understand, we allow the interval to be seconds but we dont support seconds, shouln't we change it to minutes or have it round up to the nearest minute instead of failing ? (inorder to support backward compatibility) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There will be no backwards compatibility problems, as prior users don't use the interval aligning mechanism (it's not been released yet). Also, we support aligning on thresholds shorter than a minutes here without problem. For example, if the user specs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it |
||
raise ValueError(f'Unable to properly align time interval of {values["interval"]} seconds') | ||
else: | ||
# TODO do we also want to support automatically aligning intervals longer than | ||
# a day? Using month/year boundaries? This would require additionally handling | ||
# leap years and daytime saving, just as we handle leap seconds in | ||
# NotificationSummary.calculate_interval_boundaries | ||
raise ValueError(f"Automatically aligning time intervals longer than 24 hours is not supported") | ||
return values | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import pytest | ||
|
||
from datetime import datetime, timezone | ||
|
||
from robusta.core.sinks.sink_base import NotificationSummary | ||
|
||
|
||
utc = timezone.utc | ||
|
||
|
||
class TestNotificationSummary: | ||
@pytest.mark.parametrize("input_dt,expected_output", [ | ||
(datetime(2024, 6, 25, 12, 15, 33, tzinfo=utc), (1719273600, 1719360000)), | ||
(datetime(2024, 6, 30, 17, 22, 19, tzinfo=utc), (1719705600, 1719792000)), | ||
(datetime(2024, 12, 3, 10, 59, 59, tzinfo=utc), (1733184000, 1733270400)), | ||
(datetime(2024, 12, 31, 16, 42, 28, tzinfo=utc), (1735603200, 1735689600)), | ||
]) | ||
def test_get_day_boundaries(self, input_dt, expected_output): | ||
assert NotificationSummary.get_day_boundaries(input_dt) == expected_output |
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.
what if not self.summary_table?
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.
If
end_ts
is not set it means that the whole object is not initialized, and sosummary_table
is also not set. I basically simplified the condition here.