-
Notifications
You must be signed in to change notification settings - Fork 415
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
Added latency metrics for REST endpoint. #4977
Conversation
dbaf32a
to
9b08e84
Compare
This PR also makes it possible to set specific buckets for histograms. Cloes #4932
9b08e84
to
c540694
Compare
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.
lgtm, just a few small changes
@@ -40,6 +42,7 @@ impl Default for SearchMetrics { | |||
"Number of seconds required to run a leaf search over a single split. The timer \ | |||
starts after the semaphore is obtained.", | |||
"search", | |||
exponential_buckets(0.002, 2.0, 8).unwrap(), |
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.
last bucket ends at half a second. while i understand smaller steps for leaf_search_split, in my experience having some split being longer than that isn't necessarily rare. imo either the start bucket or the number of bucket needs to be increased
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.
did both
"request_duration_seconds", | ||
"Response time in millisecs", |
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.
we use secs
, not seconds, elsewhere.
the comment gives the wrong unit too
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.
good point. Yeah this is tricky. We used secs in all place but one (apart from here) and prometheus docs recommends seconds
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.
Ill switch to secs.
002c664
to
118a3b6
Compare
* Added latency metrics for REST endpoint. This PR also makes it possible to set specific buckets for histograms. Cloes #4932 * CR comments
This PR also makes it possible to set specific buckets for histograms.
Cloes #4932