Repository navigation
Conversation
DominikB2014
left a comment
There was a problem hiding this comment.
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.
|
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. |
|
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. |
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.