Skip to content
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

Merged
merged 2 commits into from
May 13, 2024

Conversation

fulmicoton
Copy link
Contributor

This PR also makes it possible to set specific buckets for histograms.

Cloes #4932

@fulmicoton fulmicoton force-pushed the issue/4932-response-latency-metric branch 4 times, most recently from dbaf32a to 9b08e84 Compare May 13, 2024 07:28
This PR also makes it possible to set specific buckets for
histograms.

Cloes #4932
@fulmicoton fulmicoton force-pushed the issue/4932-response-latency-metric branch from 9b08e84 to c540694 Compare May 13, 2024 07:44
@fulmicoton fulmicoton marked this pull request as ready for review May 13, 2024 07:55
@fulmicoton fulmicoton requested a review from trinity-1686a May 13, 2024 07:55
Copy link
Contributor

@trinity-1686a trinity-1686a left a 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(),
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did both

Comment on lines 39 to 40
"request_duration_seconds",
"Response time in millisecs",
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ill switch to secs.

@fulmicoton fulmicoton force-pushed the issue/4932-response-latency-metric branch from 002c664 to 118a3b6 Compare May 13, 2024 08:52
@fulmicoton fulmicoton merged commit ab81a51 into main May 13, 2024
2 of 3 checks passed
@fulmicoton fulmicoton deleted the issue/4932-response-latency-metric branch May 13, 2024 08:54
fulmicoton added a commit that referenced this pull request May 14, 2024
* Added latency metrics for REST endpoint.

This PR also makes it possible to set specific buckets for
histograms.

Cloes #4932

* CR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants