-
Notifications
You must be signed in to change notification settings - Fork 413
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 prometheus metrics support #1859
base: master
Are you sure you want to change the base?
Conversation
9772ba2
to
77d5d33
Compare
MaxHeaderBytes: 1 << 20, | ||
} | ||
|
||
go func() { |
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.
Though this should be lightweight but we should do run perf testing of GCSfuse with promotheus server running and also ensure that this doesn't take extra memory and CPU
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.
Hi @sethiay, thanks for the review! I updated the PR to move the mount flag to config file. Do you happen to have a guidance to do the GCSFuse perf test and memory/CPU resource tests?
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.
Hey @songjiaxun We have presubmit performance tests (I have enabled for this PR with the labels) that do some basic performance testing without capturing CPU and memory. That is good to ensure that performance doesn't degrade incase some business logic is changed or dependencies are upgraded. However, this change requires more testing and monitoring CPU and memory, so I recommend running all the tests pipeline that we have and monitor the memory, CPU and performance numbers for the tests.
2610032
to
d21c254
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1859 +/- ##
==========================================
- Coverage 73.49% 73.32% -0.17%
==========================================
Files 96 96
Lines 10257 10301 +44
==========================================
+ Hits 7538 7553 +15
- Misses 2384 2414 +30
+ Partials 335 334 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7adebe3
to
1afeaac
Compare
```text | ||
# HELP file_cache_read_bytes_count The cumulative number of bytes read from file cache along with read type - Sequential/Random | ||
# TYPE file_cache_read_bytes_count counter | ||
file_cache_read_bytes_count{read_type="Random"} 0 |
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.
This might be coming from somewhere else, but could we prefix all metrics names with "gcsfuse_", for example
gcsfuse_file_cache_read_bytes_count ?
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'd like to keep this PR focusing on the prometheus integration. I can create a follow-up PR to modify the metrics name, but I recommend GCSFuse team make such changes because it will introduce compatibility issues as the current test pipelines may rely on these metrics names.
7d4bc23
to
4e9589a
Compare
Description
Add Prometheus metrics support. Add a new flag
prometheus-port
for users to specify prometheus metrics endpoint port.Link to the issue in case of a bug fix.
NA
Testing details
go run tools/build_gcsfuse/main.go . . v3.0.0
../bin/gcsfuse --config-file <config_file_path> <bucket-name> <mount-path>
.curl http://localhost:8080/metrics