feat: add path prefix label to obejct storage metrics #4277
Conversation
WalkthroughThe recent changes enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/object-store/src/layers/prometheus.rs (19 hunks)
- src/object-store/src/util.rs (2 hunks)
Additional comments not posted (9)
src/object-store/src/util.rs (1)
126-142: LGTM! Verify the function usage in the codebase.The function logic is correct and includes comments explaining its behavior and safety.
However, ensure that the
extract_parent_pathfunction is correctly used in the codebase.src/object-store/src/layers/prometheus.rs (8)
131-145: LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
create_dirmethod is correctly used in the codebase.Verification successful
Verified: The
create_dirmethod is correctly used in the codebase.The method
create_dirinsrc/object-store/src/layers/prometheus.rsmatches the new signature and is correctly used within the file. There are no other instances ofcreate_dirthat need updating.
src/object-store/src/layers/prometheus.rs: Correct usage of the newcreate_dirmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `create_dir` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'create_dir'Length of output: 1842
288-294: LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
presignmethod is correctly used in the codebase.Verification successful
Verification Successful: The
presignmethod usage matches the new signature.
- The method definition and its usage are consistent with the new signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `presign` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'presign'Length of output: 247
306-320: LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
blocking_create_dirmethod is correctly used in the codebase.Verification successful
LGTM! The function usage has been verified and is correct.
- The
blocking_create_dirfunction is only used within thesrc/object-store/src/layers/prometheus.rsfile.- There are no other calls to this function elsewhere in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `blocking_create_dir` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'blocking_create_dir'Length of output: 275
Script:
#!/bin/bash # Description: Verify all function calls to `blocking_create_dir` across the entire codebase to ensure they match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg 'blocking_create_dir'Length of output: 263
234-240: LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
deletemethod is correctly used in the codebase.Verification successful
Verification successful!
The
deletefunction insrc/object-store/src/layers/prometheus.rshas been correctly updated and all its usages match the new signature.
- Lines: 234-240 in
src/object-store/src/layers/prometheus.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `delete` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'delete'Length of output: 65202
Script:
#!/bin/bash # Description: Verify the specific `delete` function usage in prometheus.rs matches the new signature. # Search for the `delete` function definition in prometheus.rs to get its signature. ast-grep --lang rust --pattern 'async fn delete(&self, path: &str, args: OpDelete) -> Result<RpDelete>' src/object-store/src/layers/prometheus.rs # Search for calls to the `delete` function within prometheus.rs to ensure they match the new signature. rg --type rust 'delete\(' src/object-store/src/layers/prometheus.rsLength of output: 497
252-258: LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
listmethod is correctly used in the codebase.Verification successful
The modification in the
listmethod ofprometheus.rscorrectly uses thepath_labelparameter for Prometheus metrics. No further issues were found.
- The
listmethod insrc/object-store/src/layers/prometheus.rscorrectly integrates thepath_labelparameter.- Verified the invocation of the
listmethod withinprometheus.rs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `list` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'list'Length of output: 97347
Script:
#!/bin/bash # Description: Verify all function calls to `list` in `src/object-store/src/layers/prometheus.rs` match the new signature. # Test: Search for the specific function signature and its invocations. rg --type rust 'list\(&self, path: &str, args: OpList\)' -A 5Length of output: 1991
Script:
#!/bin/bash # Description: Verify all invocations of the `list` method in the codebase. # Test: Search for invocations of the `list` method. rg --type rust '\.list\('Length of output: 1364
Script:
#!/bin/bash # Description: Verify invocations of the `list` method in `src/object-store/src/layers/prometheus.rs` to ensure correct usage of the `path_label` parameter. # Test: Search for invocations of the `list` method in `prometheus.rs`. rg --type rust '\.list\(' src/object-store/src/layers/prometheus.rsLength of output: 127
Line range hint
157-180:
LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
readmethod is correctly used in the codebase.
Line range hint
187-210:
LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
writemethod is correctly used in the codebase.
217-223: LGTM! Verify the function usage in the codebase.The method logic is correct and includes the new
path_labelparameter for Prometheus metrics.However, ensure that the
statmethod is correctly used in the codebase.
sunng87
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4277 +/- ##
==========================================
- Coverage 84.94% 84.67% -0.28%
==========================================
Files 1061 1061
Lines 188105 188201 +96
==========================================
- Hits 159795 159352 -443
- Misses 28310 28849 +539 |
|
The official |
|
According to the patch comment, we forked it to avoid a panic with metrics registration. #2861 Checked upstream it doesn't seem to be addressed after our fork. Also our solution, using |
Yes, I realized that the layer will create a So we can't create multiple |
I created an issue for this. apache/opendal#4854 Maybe @Xuanwo can give us some suggestions. |
Thanks! I'm also thinking allow users to set metrics or reuse them in someway. I will give it look. |
|
Actually I found the Prometheus library has a few runtime panics which I think can be avoided by design. For example, if you provide labels doesn't match the declaration, it ends up with a runtime panics, which is very dangerous for some corner cases. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/object-store/src/util.rs (1)
126-141: Document function assumptions clearly.Document the assumptions about the path pattern more clearly. This will help future developers understand the function's behavior and limitations.
-// this implementation tries to extract at most 3 levels of parent path +// This implementation tries to extract at most 3 levels of parent path based on the assumption that the path follows the pattern `<data|index>/catalog/schema/table_id/...`.Tools
GitHub Check: Check typos and docs
[warning] 138-138:
"insuffient" should be "insufficient".
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/object-store/src/util.rs (2 hunks)
Additional context used
GitHub Check: Check typos and docs
src/object-store/src/util.rs
[warning] 138-138:
"insuffient" should be "insufficient".
Additional comments not posted (1)
src/object-store/src/util.rs (1)
200-222: Test cases look good.The test cases for
extract_parent_pathare comprehensive and cover various scenarios.
sunng87
force-pushed
the
feat/s3-access-stats
branch
from
0e09055
to
7daefb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/object-store/src/util.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/object-store/src/util.rs
Would you like to submit an issue to upstream and link back here? |
It provides a method |
@Xuanwo I can submit a patch for a workaround by adding a method to set the impl PrometheusLayer {
fn metrics(mut self, metrics: Arc<PrometheusMetrics>) -> Self {
self.metrics = Some(metrics);
self
}
}But this may not be elegant enough. |
|
The CI should be fixed in main branch🥲 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/object-store/src/layers/prometheus.rs (19 hunks)
Additional comments not posted (10)
src/object-store/src/layers/prometheus.rs (10)
131-145: LGTM!The
create_dirfunction correctly includes thepath_labelparameter for Prometheus metrics.
Line range hint
157-180:
LGTM!The
readfunction correctly includes thepath_labelparameter for Prometheus metrics.
Line range hint
187-210:
LGTM!The
writefunction correctly includes thepath_labelparameter for Prometheus metrics.
217-222: LGTM!The
statfunction correctly includes thepath_labelparameter for Prometheus metrics.
234-240: LGTM!The
deletefunction correctly includes thepath_labelparameter for Prometheus metrics.
252-258: LGTM!The
listfunction correctly includes thepath_labelparameter for Prometheus metrics.
288-294: LGTM!The
presignfunction correctly includes thepath_labelparameter for Prometheus metrics.
306-320: LGTM!The
blocking_create_dirfunction correctly includes thepath_labelparameter for Prometheus metrics.
333-347: LGTM!The
blocking_readfunction correctly includes thepath_labelparameter for Prometheus metrics.
374-388: LGTM!The
blocking_writefunction correctly includes thepath_labelparameter for Prometheus metrics.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/object-store/src/util.rs (1)
126-138: Enhance documentation and handle edge cases.The function
extract_parent_pathworks as intended, but consider adding more context about its assumptions and limitations. Additionally, handle edge cases explicitly, such as paths with fewer than three levels.// This logic tries to extract the parent path from the object storage operation. // The function assumes that the region path is built from the pattern `<data|index>/catalog/schema/table_id/...`. // This implementation extracts at most 3 levels of the parent path. For paths with fewer levels, it returns the full path. pub(crate) fn extract_parent_path(path: &str) -> &str { let parts: Vec<&str> = path.split('/').collect(); if parts.len() <= 3 { return path; // Return the full path if there are fewer than 3 levels. } let third_slash_index = parts.iter().take(3).map(|s| s.len() + 1).sum::<usize>() - 1; &path[..third_slash_index] }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/object-store/src/layers/prometheus.rs (19 hunks)
- src/object-store/src/util.rs (2 hunks)
Additional comments not posted (14)
src/object-store/src/layers/prometheus.rs (14)
131-137: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
Line range hint
149-172:
LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
Line range hint
179-202:
LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
209-214: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
226-232: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
244-250: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
264-268: LGTM!The method does not require the
path_labelparameter as it operates on multiple paths.
280-286: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
298-312: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
Line range hint
325-353:
LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
Line range hint
366-394:
LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
407-421: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
432-446: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
458-472: LGTM!The method correctly includes the
path_labelparameter for Prometheus metrics.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This patch add a path label to object store related prometheus metrics, so that we can observe which path has most writes/reads.
Checklist
Summary by CodeRabbit