Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ele 3682 alerts grouping #1716

Merged
merged 23 commits into from
Oct 14, 2024
Merged

Ele 3682 alerts grouping #1716

merged 23 commits into from
Oct 14, 2024

Conversation

MikaKerman
Copy link
Contributor

@MikaKerman MikaKerman commented Oct 6, 2024 •
edited by github-actions bot
Loading

null

@linear Linear
Copy link

linear bot commented Oct 6, 2024

ELE-3682 Alerts Grouping

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 6, 2024

👋 @MikaKerman
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch 4 times, most recently from dbef87b to 38c709f Compare October 7, 2024 10:48
@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch from 883026f to 6ee5466 Compare October 10, 2024 07:50
@MikaKerman MikaKerman marked this pull request as ready for review October 10, 2024 07:52
@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch from 7abcef2 to ad400ee Compare October 10, 2024 08:59
@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch from ad400ee to 35fb51b Compare October 10, 2024 09:12
NoyaArie
NoyaArie reviewed Oct 13, 2024
View reviewed changes
Copy link
Contributor

@NoyaArie NoyaArie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left a few small comments

elementary/config/config.py Outdated
self.group_alerts_threshold = self._first_not_none(
group_alerts_threshold,
slack_config.get("group_alerts_threshold"),
100,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider extracting the 100 to a constant (and use it in the cli as well :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

elementary/monitor/data_monitoring/alerts/integrations/base_integration.py Outdated
else:
flattened_alerts.append(alert)

if len(flattened_alerts) > threshold:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(flattened_alerts) > threshold:
if len(flattened_alerts) >= threshold:

when there are 100 alerts, they should send in a single message, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

elementary/monitor/data_monitoring/alerts/integrations/base_integration.py
Union[
TestAlertModel,
ModelAlertModel,
SourceFreshnessAlertModel,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SourceFreshnessAlertModel,
SourceFreshnessAlertModel,
GroupedByTableAlerts,

no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the alert is an AlertsGroup, the inner alerts will be generated. This changes the behavior of GroupedByTableAlerts, but I checked that the method's caller can handle it as well.

elementary/elementary/monitor/data_monitoring/alerts/data_monitoring_alerts.py

Line 259 in 161330d

if isinstance(alert, AlertsGroup):

elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py Outdated
for alert in alerts:
text = f":{bullet_icon}: {alert.summary}"
if alert.report_url:
text = " - ".join([text, f"<{alert.report_url}|View Details>"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the url in your alerts, I think the report_url tasks to the dashboard, is this what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch from 9b493f5 to 621e3fe Compare October 14, 2024 10:01
@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch 2 times, most recently from 2584cd7 to 5f1d7a7 Compare October 14, 2024 11:54
NoyaArie
NoyaArie approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@NoyaArie NoyaArie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@MikaKerman MikaKerman force-pushed the ele-3682-alerts-grouping branch from 5f1d7a7 to a074335 Compare October 14, 2024 15:39
@MikaKerman MikaKerman merged commit b7e70f7 into master Oct 14, 2024
3 checks passed
@MikaKerman MikaKerman deleted the ele-3682-alerts-grouping branch October 14, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoyaArie NoyaArie NoyaArie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MikaKerman @NoyaArie