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

rfc(decision): Escalating Forecasts Merged Issues #95

Merged
merged 3 commits into from
May 25, 2023

Conversation

NisanthanNanthakumar
Copy link
Contributor

@NisanthanNanthakumar NisanthanNanthakumar commented May 23, 2023

TODO. Rendered RFC

@NisanthanNanthakumar NisanthanNanthakumar force-pushed the rfc/escalatingforecastsmergedissues branch from e3b3973 to 0a9cdc5 Compare May 23, 2023 22:20
Nisanthan Nanthakumar added 2 commits May 23, 2023 15:21
@NisanthanNanthakumar NisanthanNanthakumar requested a review from a team May 24, 2023 03:02
@NisanthanNanthakumar NisanthanNanthakumar marked this pull request as ready for review May 24, 2023 03:02
@NisanthanNanthakumar NisanthanNanthakumar changed the title rfc(decision): escalating-forecasts-merged-issues rfc(decision): Escalating Forecasts Merged Issues May 24, 2023
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

This seems well thought out to me 👍

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.
Copy link
Member

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)

@NisanthanNanthakumar
Copy link
Contributor Author

Note by @onewland from discussion in Slack:
We may need to build a fallback for recently merged/unmerged issue groups where we build the forecast from errors rather than metrics.

@NisanthanNanthakumar NisanthanNanthakumar merged commit 0d913e7 into main May 25, 2023
2 checks passed
@NisanthanNanthakumar NisanthanNanthakumar deleted the rfc/escalatingforecastsmergedissues branch May 25, 2023 04:52
@armenzg
Copy link
Member

armenzg commented May 25, 2023

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants