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

Align S3 Gateway Prometheus Histogram Buckets with lakeFS API Buckets #7622

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

guy-har
Copy link
Contributor

@guy-har guy-har commented Apr 3, 2024

Description

The Prometheus histogram buckets currently used for monitoring the lakeFS API (api_requests_total) are specifically tailored to the lakeFS service's response times, providing valuable insights into API performance. However, the histogram for the S3 gateway (gateway_request_duration_seconds) utilizes the default Prometheus buckets, which may not accurately reflect the performance characteristics of the S3 gateway interactions.

Aligning the S3 gateway's Prometheus histogram buckets with those defined for the lakeFS API can enhance monitoring accuracy and offer better insights into S3 gateway performance. This adjustment would allow for a more consistent and informative analysis of request durations across both interfaces.

Copy link

github-actions bot commented Apr 3, 2024

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

@guy-har guy-har requested a review from a team April 3, 2024 06:50
Copy link

github-actions bot commented Apr 3, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

@guy-har can you please add a description to the PR or link to an issue with a description of the changes

@guy-har guy-har changed the title Edit buckets for s3 gateway prometheus histogram Align S3 Gateway Prometheus Histogram Buckets with lakeFS API Buckets Apr 3, 2024
@guy-har guy-har added include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached labels Apr 3, 2024
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks!

Help: "request durations for lakeFS storage gateway",
Name: "gateway_request_duration_seconds",
Help: "request durations for lakeFS storage gateway",
Buckets: []float64{0.01, 0.05, 0.1, 0.2, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60},
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of the intervals, especially the middle octave 0.1, 0.2, 0.25, 0.5, 1.0. The bucket-to-bucket size ratios are really weird!

limit 0.01 0.05 0.1 0.2 0.25 0.5 1 2.5 5 10 30 60
next limit / this limit - 5.0 2.0 2.0 1.25 2.0 2.0 2.5 2.0 2.0 3.0 2.0

The jumps 0.1 -> 0.2 -> 0.25 -> 0.5 are pretty poorly spaced: 2x, 1.25x, 2x. I would get rid of 0.2.

I don't care that the spacing on the buckets >10s is weird, it's not as different and the numbers there will barely matter (until they do).

Suggested change
Buckets: []float64{0.01, 0.05, 0.1, 0.2, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60},
Buckets: []float64{0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arielshaqed, I changed both here and in the API, PTAL

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks!

@guy-har guy-har merged commit 4adfb3f into master Apr 3, 2024
35 checks passed
@guy-har guy-har deleted the chore/s3-gateway-promethous-buckets branch April 3, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants