fix(gcs_reader): gcs_download_bytes_count metrics is broken by raj-prince · Pull Request #3895 · GoogleCloudPlatform/gcsfuse (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Description

Fixing duplicate instrumentation of gcs/download_bytes_count[sequential/random].
Propagating right read-type in case of new read-manager flow.
Fixing the duplicate instrumentation in old flow too.

Link to the issue in case of a bug fix.

b/443637302

Testing details

Manual - NA
Unit tests - Added
Integration tests - NA

Any backward incompatible change? If so, please explain.

[ gemini-code-assist[bot] ](/apps/gemini-code-assist)

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 effectively resolves the issue of duplicate instrumentation for gcs/download_bytes_count metrics. The core fix, which involves relocating the metric capturing logic to the startRead function, is sound and correctly ensures that metrics are recorded only once when a new GCS read is initiated. The propagation of the readType to the underlying readers is also handled well. I appreciate the thoroughness of adding new tests to cover both the old and new reader paths for sequential and random reads. My main feedback is a suggestion to refactor the new tests to reduce code duplication, which will improve maintainability.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.37%. Comparing base (404f301) to head (c2bc761).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@ ## master #3895 +/- ##

Coverage 82.40% 82.37% -0.03%
Files 145 145
Lines 22773 22769 -4
Hits 18766 18757 -9
Misses 3462 3466 +4
Partials 545 546 +1

Flag	Coverage Δ
unittests	82.37% <100.00%> (-0.03%)	⬇️

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.

[ gemini-code-assist[bot] ](/apps/gemini-code-assist)

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 effectively addresses the issue of broken gcs_download_bytes_count metrics by correcting the duplicate instrumentation and ensuring the proper propagation of the read type. The refactoring to pass the read type as part of the request rather than holding it as state in the reader is a good improvement. The addition of specific tests for sequential and random read metrics provides good coverage for the fix. I have one suggestion to improve the maintainability of the new test code.