Fix dynamic rate limit escalation metrics #861
Conversation
| ) | ||
| } else { // Dynamic escalation was skipped (dynamic <= static) | ||
| } else if rate < cfg.Rate { | ||
| r.telemetryBuilder.RatelimitDynamicEscalations.Add(ctx, 1, |
There was a problem hiding this comment.
[For reviewers] (CC: @marclop) I am not sure about this metric with the latest changes. I was thinking to base this off of the value of the multiplier but I am not sure if that will be helpful either. I have updated the current metric based on if it is higher or lower than the configured static rate but it won’t catch if it is under degradation and starting to decrease from the static rate. We can probably do this by comparing against the previous rate rather than the static rate. Let me know what you think.
There was a problem hiding this comment.
Maybe an alternative to this metric would be to have a gauge where the final rate is published with the right dimensions (metadata keys, class, etc).
There was a problem hiding this comment.
Hmm, that sounds like a good idea. A gauge will give direct insight into the rate, do you think we should keep this one too or replace this entirely?
There was a problem hiding this comment.
I'm more inclined to remove this one and add a gauge than keep it, but if you think it adds good insight we can keep it as well. But seems redundant if we add the gauge, right?
There was a problem hiding this comment.
ready to review again once you make the changes discussed here
2 participants
Since the component now supports decreasing the rate limit, adapt the
RatelimitDynamicEscalationsmetric accordingly.