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.

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

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

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

joeyorlando
Copy link
Contributor

Which issue(s) this PR fixes

Fixes https://github.com/grafana/oncall-private/issues/2376

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@joeyorlando joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Dec 11, 2023
@joeyorlando joeyorlando requested a review from a team December 11, 2023 16:00
Comment on lines 1220 to -1226
self.hard_delete()
# Update alert group state metric cache
self._update_metrics(organization_id=user.organization_id, previous_state=initial_state, state=None)
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

Copy link
Member

@Ferril Ferril left a comment

Choose a reason for hiding this comment

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

LGTM

@joeyorlando joeyorlando merged commit 16ba87b into dev Dec 11, 2023
21 checks passed
@joeyorlando joeyorlando deleted the jorlando/fix-delete-alert-group-bug branch December 11, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants