-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
rfc(decision): Escalating Forecasts Merged Issues #95
Merged
NisanthanNanthakumar
merged 3 commits into
main
from
rfc/escalatingforecastsmergedissues
May 25, 2023
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
- Start Date: 2023-05-23 | ||
- RFC Type: decision | ||
- RFC PR: https://github.com/getsentry/rfcs/pull/95 | ||
- RFC Status: draft | ||
|
||
# Summary | ||
|
||
We need to decide how to handle the Issue States and Escalating Forecasts for Merged issues. | ||
|
||
## What is Merged Issues | ||
|
||
If you have similar-looking Error Issues that have not been grouped together automatically and you want to reduce noise, you can do so by merging them. A user can also un-merge the issues. | ||
|
||
Sentry does not infer any new grouping rules from how you merge issues. Future events will be added to the merged set of issues by the same criteria as they would've been added to the individual issues that are now in the merged set. | ||
|
||
## What is Issue States & Escalating Forecasts | ||
|
||
![issue state diagram.png](0080-images/issue_state_diagram.png) | ||
|
||
When a user Archives (until escalating) an issue, we generate a forecast using the past 7 day's event volume. If the event volume exceeds the forecasted value, then we mark the issue as Escalating and surface it in the Issue Stream. | ||
|
||
# Motivation | ||
|
||
1. We need a plan on how to combine/split the Issue States and Escalating Forecasts when we merge/un-merge issues. | ||
|
||
# Background | ||
|
||
The reason this decision or document is required. This section might not always exist. | ||
|
||
Code where we handle merge: https://github.com/getsentry/sentry/blob/master/src/sentry/issues/merge.py#L19 | ||
|
||
Code where we handle un-merge: https://github.com/getsentry/sentry/blob/master/src/sentry/tasks/unmerge.py#L465 | ||
|
||
Snuba records which Issues are merged. The primary Issue is the one that has the most events. The other Issues will get merged into the primary Issue. | ||
|
||
# Proposed Option | ||
|
||
## What should we do with the GroupStatus, GroupInboxReason and the EscalatingForecasts? | ||
|
||
### Merge | ||
|
||
We should delete the individual Issues EscalatingForecasts and create a new forecast for the combined Issue. On merge, we should inherit on the GroupStatus and GroupInboxReason of the primary Issue. | ||
|
||
### Un-Merge | ||
|
||
We should delete the merged Issue's EscalatingForecasts and create a new forecast for the separated Issues. On un-merge, we should assign the GroupStatus and GroupInboxReason of the merged Issue to the separated Issues. | ||
|
||
|
||
## Auto-Archive | ||
|
||
After an issue has been in the `Unresolved(ongoing)` state for 14 days, it will be automatically moved to the `Archived(until_escalating)` state. | ||
|
||
### Merge | ||
|
||
Since we are inheriting the GroupStatus of the primary Issue, we can use its' time in `Unresolved(ongoing)` state for the Auto-Archive feature. | ||
|
||
### Un-Merge | ||
|
||
On un-merge, the GroupStatus of the separated Issues will be of the merged Issue. We should start a new timer for the newly split Issues, but maintain the timer for the primary Issue (the Issue that accumulates all the other Issues). | ||
|
||
# Drawbacks | ||
|
||
Why should we not do this? What are the drawbacks of this RFC or a particular option if | ||
multiple options are presented. | ||
|
||
We need to determine how often events are getting merged and un-merged by users. A high volume could lead to high load to Snuba when we generate new Escalating Forecasts for the newly merged/un-merged Issues. | ||
|
||
# Unresolved questions |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we should have stats on this somewhere in datadog already. I don't think it'll be a huge issue to recalculate on merge, because even if we merge 1000 issues into a single issue, it's still just generating a single forecast.
I suppose that on unmerge is the larger risk, since we might trigger a large number of new forecasts from a single issue. Maybe the way to keep things safe here is to just keep the number of workers on the forecast queue low enough that it won't hurt snuba too much.
One other potential issue is that I think it takes a while for a merge to go through to clickhouse and to rewrite the group ids. So we'd need to delay the task, but I don't know that we have any specific confirmation on when the merge has completed (maybe sns can advise here)