Skip to content

feat(api): Add optional endpoint duration budgets - #129107

Open
kcons wants to merge 3 commits into
masterfrom
kcons/too_slow
Open

kcons wants to merge 3 commits into
masterfrom
kcons/too_slow

Conversation

@kcons

@kcons kcons commented Oct 28, 2026

Copy link
Copy Markdown
Member

It's difficult for a team that owns dozens of endpoints to do good latency monitoring.
Each method at each endpoint can vary in how much latency can be acceptable, and it's very easy for rare unacceptable cases to hide in aggregates.

This introduces a new approach: endpoints can specify how slow is clearly too slow, and we report the duration over budget, unsampled, tagged with team and other key metrics.

This means that if an endpoint has some pathological behavior for only a fraction of orgs, and only with certain filters applied, we can easily identify that, and track how severe it is. You can track the aggregate unacceptable user-inflicted delay per endpoint that your team is responsible for. We can also track the rate of unacceptable latency (though that may require additional changes to be doable by-owner).

The real core of this is having a place to encode what we'd count as clearly too slow; it's an odd thing to embed in the Endpoint class vs in monitoring, but maintaining hundreds of values in monitoring configs is notably harder, and expected max latency is a product choice.

In this change, it has only been specified for a fraction of the Issues team endpoints, with a rough attempt to match existing values. Any disagreement with the specific values is valuable product and monitoring discussion, but not directly relevant to this PR.

The end goal here is to be able to set up a Terraform-maintained monitor for this that can automatically direct notification to the owning team, but prior to that we need to see it prove reliable and valuable for Issues team.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 28, 2026

@DominikB2014 DominikB2014 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Putting the duration budgets in code makes sense to me, routes, classnames can change, causing things to become out of sync. I think we should have the 3.0 be some default instead of duplicating it.

My only concern is changing the budgets requires a full redeploy, if we could pull in the budget into the code, that would solve that. But despite that tradeoff, i still like to keep it in all in code to avoid having to go to another place just to set the values. As long as any alerts attached to the durations can be muted, so we don't get spammed until a push a budget change/code change, this all seems reasonable to me.

@DominikB2014
DominikB2014 requested a review from a team October 29, 2026 18:49
@sentaur-athena

Copy link
Copy Markdown
Member

This is a good idea to launch and see how it affects issues endpoint. Just as an experiment. I agree with @DominikB2014 that we need better tooling to modify this better but IMO you can launch as is and start the experimentation.
Maybe if this is issues only for now rename the field in the base class so no one else accidentally get included?

@kcons

kcons commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

I debated adding a means of specifying value overrides via options or something for faster tweaking, but as noted, I think it's just easier to work around it by muting that particular instance while you fix the threshold. The general premise is that durations should be chosen conservatively and probably based on data (these were in fact chosen based on historical data, some are 3s if the p99 is reliably below that, others are 10s because they are known to be slow and saying "the current latency state of affairs is unacceptable" is very out of scope for this change.

I switched to a constant for the 3.0 since repeating the arbitrary value was a bit excessive, and this was if we eventually make the default 3.0 (or whatever) rather than None, it's very clear where we've made a specific choice and where we can use the default.
I also considered having multiple values for different use cases, but defining those would be retrofitting perf standards that we don't really have defined, so I opted not to.

@kcons
kcons marked this pull request as ready for review October 30, 2026 23:58
@kcons
kcons requested review from a team as code owners October 30, 2026 23:58

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants