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

Don't update alert group metrics when deleting an alert group #3544

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Don't update alert group metrics when deleting an alert group #3544

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da9a695
dont update alert group metrics when
joeyorlando Dec 11, 2023
4676f86
add test
joeyorlando Dec 11, 2023
620c5a6
update changelog
joeyorlando Dec 11, 2023
7372fff
WIP
joeyorlando Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Fix schedules invalid dates issue ([#support-escalations/issues/8084](https://github.com/grafana/support-escalations/issues/8084))
- Fix issue related to updating alert group metrics when deleting an alert group via the public API by @joeyorlando ([#3544](https://github.com/grafana/oncall/pull/3544))

## v1.3.76 (2023-12-11)

Expand Down
9 changes: 2 additions & 7 deletions engine/apps/alerts/models/alert_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,8 +1191,6 @@ def wipe_by_user(self, user: User) -> None:
def delete_by_user(self, user: User):
from apps.alerts.models import AlertGroupLogRecord

initial_state = self.state

self.stop_escalation()
# prevent creating multiple logs
# filter instead of get_or_create cause it can be multiple logs of this type due deleting error
Expand All @@ -1219,13 +1217,10 @@ def delete_by_user(self, user: User):
force_sync=True,
)

dependent_alerts = list(self.dependent_alert_groups.all())

self.hard_delete()
# Update alert group state metric cache
self._update_metrics(organization_id=user.organization_id, previous_state=initial_state, state=None)
Comment on lines 1222 to -1226
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an alternative here could be to call self._update_metrics() before calling self.hard_delete()

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to call it before, since it updates metrics async and we don't protect ourself of error it should fix.
I'd propose either call it sync here (but we change it to async everywhere by purpose) or expend parameters we pass to _update_metrics to allow it to update metrics anyway.

For now I think it's ok to remove metrics update from here, since we recalculate all metrics once a day


for dependent_alert_group in dependent_alerts: # unattach dependent incidents
# unattach dependent incidents
for dependent_alert_group in self.dependent_alert_groups.all():
dependent_alert_group.un_attach_by_delete()

def hard_delete(self):
Expand Down
2 changes: 2 additions & 0 deletions engine/apps/alerts/tasks/delete_alert_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def delete_alert_group(alert_group_pk, user_pk):
logger.debug("User not found, skipping delete_alert_group")
return

logger.debug(f"User {user} is deleting alert group {alert_group} (channel: {alert_group.channel})")

try:
alert_group.delete_by_user(user)
except SlackAPIRatelimitError as e:
Expand Down
44 changes: 44 additions & 0 deletions engine/apps/alerts/tests/test_alert_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,3 +577,47 @@ def test_filter_active_alert_groups(
assert active_alert_groups.count() == 2
assert alert_group_active in active_alert_groups
assert alert_group_active_silenced in active_alert_groups


@patch("apps.alerts.models.AlertGroup.hard_delete")
@patch("apps.alerts.models.AlertGroup.un_attach_by_delete")
@patch("apps.alerts.models.AlertGroup.stop_escalation")
@patch("apps.alerts.models.alert_group.alert_group_action_triggered_signal")
@pytest.mark.django_db
def test_delete_by_user(
mock_alert_group_action_triggered_signal,
_mock_stop_escalation,
_mock_un_attach_by_delete,
_mock_hard_delete,
make_organization_and_user,
make_alert_receive_channel,
make_alert_group,
):
organization, user = make_organization_and_user()
alert_receive_channel = make_alert_receive_channel(organization)

alert_group = make_alert_group(alert_receive_channel)

# make a few dependent alert groups
dependent_alert_groups = [make_alert_group(alert_receive_channel, root_alert_group=alert_group) for _ in range(3)]

assert alert_group.log_records.filter(type=AlertGroupLogRecord.TYPE_DELETED).count() == 0

alert_group.delete_by_user(user)

assert alert_group.log_records.filter(type=AlertGroupLogRecord.TYPE_DELETED).count() == 1
deleted_log_record = alert_group.log_records.get(type=AlertGroupLogRecord.TYPE_DELETED)

alert_group.stop_escalation.assert_called_once_with()

mock_alert_group_action_triggered_signal.send.assert_called_once_with(
sender=alert_group.delete_by_user,
log_record=deleted_log_record.pk,
action_source=None,
force_sync=True,
)

alert_group.hard_delete.assert_called_once_with()

for dependent_alert_group in dependent_alert_groups:
dependent_alert_group.un_attach_by_delete.assert_called_with()
Loading