feat(metrics): Implement and capture the metadata cache read count metric. - #5093
thrivikram-karur-g wants to merge 17 commits into
Conversation
…vikram-metadata-read-cache-simple-implementation
…vikram-metadata-read-cache-simple-implementation
github-actions
Bot
added
the
remind-reviewers
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces detailed metadata cache metrics tracking (such as cache hits, misses, TTL expirations, and positive/negative entry statuses) across gcsfuse. It updates the stat cache, directory inodes, and file inodes to record these metrics, and adds comprehensive end-to-end tests. A critical correctness issue was identified in internal/fs/inode/dir.go where using || instead of && in the negative entry check could lead to false ENOENT errors and data invisibility.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5093 +/- ##
==========================================
- Coverage 86.79% 86.77% -0.03%
==========================================
Files 176 177 +1
Lines 18868 18936 +68
==========================================
+ Hits 16377 16431 +54
- Misses 2490 2504 +14
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…vikram-metadata-read-cache-simple-implementation
thrivikram-karur-g
added
the
execute-integration-tests
|
Passed Integration tests. |
thrivikram-karur-g
added
execute-perf-test
…vikram-metadata-read-cache-simple-implementation
|
Perf tests also have been run |
|
Updated the perf results run on the PR with the latest changes. |
thrivikram-karur-g
added
execute-integration-tests
|
Re-Ran the integration tests with the updated PR changes and they passed. |
|
|
||
| require.True(t, foundMetric, "metric %s not found", metricName) | ||
| require.Fail(t, "Data point for attributes %v not found in %s metric", attrs, metricName) | ||
| require.Failf(t, "data point not found", "no data point matching attributes %v in metric %s", attrs, metricName) |
There was a problem hiding this comment.
what's the reason behind making these changes?
| LookUp(name string, now time.Time) (hit bool, m *gcs.MinObject) | ||
|
|
||
| // LookUpDetail returns the current object entry along with entry status and lookup detail. | ||
| LookUpDetail(name string, now time.Time) (hit bool, m *gcs.MinObject, entryStatus metrics.EntryStatus, detail metrics.LookupDetail) |
There was a problem hiding this comment.
I find EntryKind or EntryType better than EntryStatus. Status should represent things like whether the entry is expired or not.
| } | ||
|
|
||
| if req.FetchOnlyFromCache { | ||
| } else if req.FetchOnlyFromCache { |
There was a problem hiding this comment.
Why change if to else if? It adds more cognitive load. Same comment elsewhere.
There was a problem hiding this comment.
will keep variables out of if scope
| return | ||
| } | ||
|
|
||
| func (b *fastStatBucket) lookUpDetail(name string) (hit bool, m *gcs.MinObject, entryStatus metrics.EntryStatus, detail metrics.LookupDetail) { |
There was a problem hiding this comment.
Let's rename it LookupFileDetail
| // 3. Negative entry check: | ||
| // Both lookups must be cache hits (no cacheMiss errors) with no results found for us to | ||
| // conclude the entry does not exist. If only one candidate is a negative hit, the other | ||
| // candidate may still exist in GCS, so we must fall through and query GCS. |
There was a problem hiding this comment.
If the GCS call is still happening - should it be recorded as a cache-hit or a miss?
| // This is a no-op when the context carries no outcome, which is the case when | ||
| // metrics are disabled or for unit tests that drive LookUpChild directly. | ||
| // | ||
| // Not safe for concurrent use. All call sites are on LookUpChild's own |
There was a problem hiding this comment.
This looks fragile. While currently it might work, it's a maintenance hazard. I understand that performance could be a concern in which case, we should see how to fix that. But without that, it could lead to hard-to-find bugs.
|
Hi @vadlakondaswetha, 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 participants
Description
Implementing the metadata_cache/read_count metric to capture the hits and misses with detailed reason for miss or hit as a counter metric to understand the metadata_cache performance.
metadata_cache/read_countwas already defined inmetrics/metrics.yamlbut nothing everrecorded it. This PR wires up emission and export.
Summary of the code changes:
dirInode.LookUpChild— withcache_hit,entry_status(positive/negative) andlookup_detail(found/ttl_expired/not_found).stat_cache.goderives the classification,fast_stat_bucket.gocarries the miss reasonup via
CacheMissError,dir.goemits,otelexporters.goallowlists the prefix.Perf test results on the PR:
we do not see any regression and all bandwidth fluctuations seems normal and within healthy range.
Link to the issue in case of a bug fix.
b/512002923
Testing details
Executed the below manual test plan for checking the metrics count updates for the metadata_cache/read_count metric.
Ran GCSFuse with the below command.
Start GCSFuse with Prometheus metric reporting enabled on port
8080:Query the exposed Prometheus metrics endpoint to inspect the metadata_cache/read_count counter and its attributes:
bash
curl -s http://localhost:8080/metrics | grep "metadata_cache/read_count"
Verified manually the below test cases for corresponding change in metric counts queried from the prometheus end point.
stat <MOUNT_DIR>/nonexistent_file.txtmetadata_cache_read_count_total{cache_hit="false",lookup_detail="not_found"} +1stat <MOUNT_DIR>/nonexistent_dir/metadata_cache_read_count_total{cache_hit="false",lookup_detail="not_found"} +1stat <MOUNT_DIR>/existing_file.txt2.
stat <MOUNT_DIR>/existing_file.txt{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="true",entry_status="positive",lookup_detail="found"} +1stat <MOUNT_DIR>/existing_dir/2.
stat <MOUNT_DIR>/existing_dir/{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="true",entry_status="positive",lookup_detail="found"} +1stat <MOUNT_DIR>/nonexistent_file.txt2.
stat <MOUNT_DIR>/nonexistent_file.txt{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="true",entry_status="negative",lookup_detail="found"} +1stat <MOUNT_DIR>/nonexistent_dir/2.
stat <MOUNT_DIR>/nonexistent_dir/{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="true",entry_status="negative",lookup_detail="found"} +1stat <MOUNT_DIR>/existing_file.txt2.
sleep 125 && stat <MOUNT_DIR>/existing_file.txt{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="false",entry_status="positive",lookup_detail="ttl_expired"} +1stat <MOUNT_DIR>/existing_dir/2.
sleep 125 && stat <MOUNT_DIR>/existing_dir/{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="false",entry_status="positive",lookup_detail="ttl_expired"} +1stat <MOUNT_DIR>/nonexistent_file.txt2.
sleep 125 && stat <MOUNT_DIR>/nonexistent_file.txt{cache_hit="false",lookup_detail="not_found"} +12nd stat:
{cache_hit="false",entry_status="negative",lookup_detail="ttl_expired"} +1ls --color=never <MOUNT_DIR>/metadata_cache_read_count(ReadDir does not issue per-entry stat calls)Any backward incompatible change? If so, please explain.
N/A