Skip to content

feat(metrics): allow meter provider to be passed in NewGRPCClient#3729

Merged
kislaykishore merged 18 commits intomasterfrom
use-WithMeterProvider
Dec 4, 2025
Merged

feat(metrics): allow meter provider to be passed in NewGRPCClient#3729
kislaykishore merged 18 commits intomasterfrom
use-WithMeterProvider

Conversation

@cojenco
Copy link
Copy Markdown
Collaborator

@cojenco cojenco commented Aug 21, 2025

Description

  • Allow meter provider to be passed in to the Go SDK NewGRPCClient, using the WithMeterProvider client option in the Go Storage SDK
  • Update monitoring prom tests for gRPC metrics

Depends on googleapis/google-cloud-go#12668

Link to the issue in case of a bug fix.

Testing details

  1. Manual - Tested
  2. Unit tests - NA
  3. Integration tests - Added

Manually tested on GKE and GCE. Integration tests are ready once googleapis/google-cloud-go#12668 is launched

$ go test ./tools/integration_tests/monitoring/... --integrationTest -v 
=== RUN   TestPromGrpcMetricsSuite
{"timestamp":{"seconds":1755816146,"nanos":897136316},"severity":"INFO","message":"Building GCSFuse from source in the dir: /tmp/gcsfuse_readwrite_test_4022898957 ..."}
{"timestamp":{"seconds":1755816146,"nanos":897185786},"severity":"INFO","message":"Building build_gcsfuse at /tmp/gcsfuse_integration_tests2729044397/build_gcsfuse"}
{"timestamp":{"seconds":1755816151,"nanos":90209635},"severity":"INFO","message":"Building gcsfuse into /tmp/gcsfuse_readwrite_test_4022898957"}
=== RUN   TestPromGrpcMetricsSuite/TestStorageClientGrpcMetrics
--- PASS: TestPromGrpcMetricsSuite (33.62s)
    --- PASS: TestPromGrpcMetricsSuite/TestStorageClientGrpcMetrics (3.24s)
=== RUN   TestPromOTELSuite
{"timestamp":{"seconds":1755816180,"nanos":519715471},"severity":"INFO","message":"Building GCSFuse from source in the dir: /tmp/gcsfuse_readwrite_test_27243481 ..."}
{"timestamp":{"seconds":1755816180,"nanos":519801451},"severity":"INFO","message":"Building build_gcsfuse at /tmp/gcsfuse_integration_tests2133238605/build_gcsfuse"}
{"timestamp":{"seconds":1755816184,"nanos":711409334},"severity":"INFO","message":"Building gcsfuse into /tmp/gcsfuse_readwrite_test_27243481"}
=== RUN   TestPromOTELSuite/TestFsOpsErrorMetrics
=== RUN   TestPromOTELSuite/TestListMetrics
=== RUN   TestPromOTELSuite/TestReadMetrics
=== RUN   TestPromOTELSuite/TestStatMetrics
--- PASS: TestPromOTELSuite (36.87s)
    --- PASS: TestPromOTELSuite/TestFsOpsErrorMetrics (1.60s)
    --- PASS: TestPromOTELSuite/TestListMetrics (1.62s)
    --- PASS: TestPromOTELSuite/TestReadMetrics (1.67s)
    --- PASS: TestPromOTELSuite/TestStatMetrics (1.62s)
PASS
ok      github.com/googlecloudplatform/gcsfuse/v3/tools/integration_tests/monitoring    70.543s

@cojenco cojenco requested a review from kislaykishore August 21, 2025 22:58
@cojenco cojenco mentioned this pull request Aug 21, 2025
Merged
kislaykishore
kislaykishore reviewed Aug 25, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kislaykishore kislaykishore left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I'll prepare a list of performance tests to run.

kislaykishore
kislaykishore reviewed Aug 26, 2025
View reviewed changes
@cojenco cojenco requested a review from kislaykishore August 28, 2025 00:20
@cojenco
Copy link
Copy Markdown
Collaborator Author

cojenco commented Sep 2, 2025

Overall, looks good. I'll prepare a list of performance tests to run.

@kislaykishore I have addressed the comments and added a flag. When you have a moment, could you take another pass and share the list of performance tests?

Also, since the Go storage changes are now in main, would we be able to run the perf and integration tests through CI/CD using those GitHub tags you mentioned?

@danielduhh
Copy link
Copy Markdown

danielduhh commented Nov 7, 2025

Hi, is this blocked by something?

@cojenco cojenco marked this pull request as ready for review November 12, 2025 23:35
@cojenco cojenco requested review from a team and Tulsishah as code owners November 12, 2025 23:35
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 12, 2025

/gemini summary

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 12, 2025

/gemini review

@github-actions github-actions bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Nov 12, 2025
@kislaykishore kislaykishore added execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Nov 13, 2025
@cojenco
Copy link
Copy Markdown
Collaborator Author

cojenco commented Nov 13, 2025

This PR depends on Go storage SDK versions >= v1.57.0, which introduces WithMeterProvider clientOption that is required for this change.

Go SDK has been reverted to v1.56.3 in GCSFuse go.mod due to seeing widespread network call failures blocking GCSFuse release.

The 3 failed checks error with undefined: experimental.WithMeterProvider and should be resolved once Go SDK is upgraded to versions >= v1.57.0

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 14, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.33%. Comparing base (43b0c85) to head (c5b455b).
⚠️ Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
cmd/legacy_main.go 60.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3729      +/-   ##
==========================================
+ Coverage   83.27%   83.33%   +0.05%     
==========================================
  Files         153      153              
  Lines       18613    18634      +21     
==========================================
+ Hits        15500    15528      +28     
+ Misses       2541     2538       -3     
+ Partials      572      568       -4
Flag Coverage Δ
unittests 83.33% <84.61%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cojenco cojenco added execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests and removed execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Nov 14, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 17, 2025

Hi @kislaykishore, @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

2 similar comments
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 18, 2025

Hi @kislaykishore, @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 19, 2025

Hi @kislaykishore, @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

kislaykishore
kislaykishore previously approved these changes Nov 19, 2025
View reviewed changes
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 20, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

8 similar comments
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 21, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 24, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 25, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 26, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 27, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 28, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 1, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 2, 2025

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@cojenco cojenco requested a review from kislaykishore December 3, 2025 05:39
@kislaykishore
Copy link
Copy Markdown
Collaborator

kislaykishore commented Dec 3, 2025

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Dec 3, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a feature to enable gRPC metrics by passing an OpenTelemetry meter provider to the gRPC client, controlled by the new enable-grpc-metrics flag. The changes are well-structured, with corresponding updates to configuration, command-line flags, and application logic. The addition of unit and integration tests is commendable and ensures the feature's correctness. I have one minor suggestion regarding a TODO comment to improve clarity for future development.

kislaykishore
kislaykishore requested changes Dec 3, 2025
View reviewed changes
@kislaykishore kislaykishore removed the execute-perf-test Execute performance test in PR label Dec 4, 2025
@kislaykishore kislaykishore merged commit a6ed753 into master Dec 4, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kislaykishore kislaykishore kislaykishore approved these changes

@Tulsishah Tulsishah Awaiting requested review from Tulsishah Tulsishah is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

execute-integration-tests Run only integration tests remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cojenco @danielduhh @kislaykishore