-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add rate limiting and metrics to hedging #4860
Conversation
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
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.
Looks great!
Just a note about the metric naming, but otherwise 👌
|
||
func registerMetrics() { | ||
totalHedgeRequests = promauto.NewCounter(prometheus.CounterOpts{ | ||
Name: "hedged_requests_total", |
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.
Will these be prefixed with loki_
?
Name: "hedged_requests_total", | |
Name: "hedged_requests_total", | |
Namespace: "loki", |
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.
No I did not prefix them because I figure we might move this to dskit.
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 prefix them until we move it to dskit
. All of our other metrics have loki_
prefixes.
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena [email protected]
Checklist
CHANGELOG.md
about the changes.