fix: remove path label for cache store #4336
Conversation
sunng87
requested review from
evenyag,
v0y4g3r,
waynexia and
a team
as code owners
WalkthroughThe changes primarily revolve around enhancing 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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/common/datasource/src/object_store/fs.rs (1 hunks)
- src/common/datasource/src/object_store/s3.rs (1 hunks)
- src/datanode/src/store.rs (1 hunks)
- src/mito2/src/access_layer.rs (1 hunks)
- src/mito2/src/cache/write_cache.rs (2 hunks)
- src/object-store/src/layers/prometheus.rs (16 hunks)
- src/object-store/src/util.rs (2 hunks)
Additional comments not posted (15)
src/common/datasource/src/object_store/fs.rs (1)
34-34: Ensure proper instantiation ofPrometheusMetricsLayer.The
PrometheusMetricsLayeris instantiated with thenewmethod and atrueargument. This change seems correct and consistent with the objective of adding apath_labelparameter.Action Required: Verify that the boolean argument (
true) is intended and correctly aligns with the desired behavior for thepath_label.Verification successful
Verified: Proper instantiation of
PrometheusMetricsLayer.The boolean argument
truecorrectly aligns with the desired behavior for thepath_labelparameter in thePrometheusMetricsLayer::newmethod.
src/common/datasource/src/object_store/s3.rssrc/common/datasource/src/object_store/fs.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `PrometheusMetricsLayer::new(true)` to ensure correctness. # Test: Search for the usage of `PrometheusMetricsLayer::new` to confirm that the boolean argument is intended. rg --type rust 'PrometheusMetricsLayer::new'Length of output: 363
Script:
#!/bin/bash # Search for the definition of the `PrometheusMetricsLayer::new` method to inspect its implementation. ast-grep --lang rust --pattern 'impl PrometheusMetricsLayer { fn new($_) { $$$ } }'Length of output: 85
Script:
#!/bin/bash # Search for the definition of the `PrometheusMetricsLayer` struct or class to locate its implementation. rg --type rust 'struct PrometheusMetricsLayer'Length of output: 126
Script:
#!/bin/bash # Search for the `new` method within the `src/object-store/src/layers/prometheus.rs` file. rg 'fn new' src/object-store/src/layers/prometheus.rs -A 10Length of output: 661
src/common/datasource/src/object_store/s3.rs (1)
97-97: Ensure proper instantiation ofPrometheusMetricsLayer.The
PrometheusMetricsLayeris instantiated with thenewmethod and atrueargument. This change seems correct and consistent with the objective of adding apath_labelparameter.Action Required: Verify that the boolean argument (
true) is intended and correctly aligns with the desired behavior for thepath_label.Verification successful
Proper instantiation of
PrometheusMetricsLayerverified.The boolean argument
trueinPrometheusMetricsLayer::new(true)correctly initializes thepath_labelfield, ensuring the desired behavior.
src/common/datasource/src/object_store/s3.rssrc/common/datasource/src/object_store/fs.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `PrometheusMetricsLayer::new(true)` to ensure correctness. # Test: Search for the usage of `PrometheusMetricsLayer::new` to confirm that the boolean argument is intended. rg --type rust 'PrometheusMetricsLayer::new'Length of output: 363
Script:
#!/bin/bash # Description: Search for the definition of the `PrometheusMetricsLayer::new` method to understand the role of the boolean parameter. # Locate the definition of `PrometheusMetricsLayer::new`. rg --type rust 'fn new' | rg 'PrometheusMetricsLayer'Length of output: 53
Script:
#!/bin/bash # Description: Search for all occurrences of `PrometheusMetricsLayer` to locate its definition and understand the role of the boolean parameter. # Search for any mention of `PrometheusMetricsLayer` to trace its definition. rg --type rust 'PrometheusMetricsLayer'Length of output: 751
Script:
#!/bin/bash # Description: Inspect the implementation of `PrometheusMetricsLayer` in `src/object-store/src/layers/prometheus.rs` to find the `new` method and understand the boolean parameter. # Search for the new method implementation within prometheus.rs rg --type rust 'fn new' src/object-store/src/layers/prometheus.rsLength of output: 122
Script:
#!/bin/bash # Description: Inspect the surrounding code and comments in `src/object-store/src/layers/prometheus.rs` to understand the role of `path_label` in the `new` method. # Display the surrounding lines of the `new` method implementation. rg --type rust -A 10 -B 10 'fn new' src/object-store/src/layers/prometheus.rsLength of output: 1173
src/datanode/src/store.rs (1)
63-63: Ensure correct usage ofwith_instrument_layers.The
with_instrument_layersfunction is called with an additional boolean argument (true). This change seems correct and consistent with the objective of adding apath_labelparameter to thePrometheusMetricsLayer.Action Required: Verify that the boolean argument (
true) is intended and correctly aligns with the desired behavior for thepath_label.Verification successful
Usage of
with_instrument_layersis correct.The boolean argument (
true) passed towith_instrument_layersis intended and correctly aligns with the desired behavior for thePrometheusMetricsLayer.
src/object-store/src/util.rs: Thepath_labelparameter is used to initialize thePrometheusMetricsLayer.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `with_instrument_layers` with the additional boolean argument to ensure correctness. # Test: Search for the usage of `with_instrument_layers` to confirm that the boolean argument is intended. rg --type rust 'with_instrument_layers'Length of output: 626
Script:
#!/bin/bash # Retrieve the implementation details of the `with_instrument_layers` function ast-grep --lang rust --pattern 'pub fn with_instrument_layers($_, $_) -> $_ { $$$ }'Length of output: 1056
Script:
#!/bin/bash # Retrieve the implementation details of the `PrometheusMetricsLayer::new` function ast-grep --lang rust --pattern 'impl PrometheusMetricsLayer { pub fn new($_) -> $_ { $$$ } }'Length of output: 439
src/object-store/src/util.rs (2)
141-141: Ensure proper instantiation ofPrometheusMetricsLayer.The
PrometheusMetricsLayeris instantiated with thenewmethod and apath_labelargument. This change seems correct and consistent with the objective of adding apath_labelparameter.Action Required: Verify that the
path_labelargument is correctly passed and aligns with the desired behavior for thePrometheusMetricsLayer.Verification successful
Verification Successful: Proper instantiation of
PrometheusMetricsLayerThe
PrometheusMetricsLayeris instantiated with the correctpath_labelargument in all instances.
src/object-store/src/util.rssrc/mito2/src/access_layer.rssrc/datanode/src/store.rsThe
path_labelargument is correctly passed and aligns with the desired behavior for thePrometheusMetricsLayer.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `PrometheusMetricsLayer::new(path_label)` to ensure correctness. # Test: Search for the usage of `PrometheusMetricsLayer::new` to confirm that the `path_label` argument is correctly passed. rg --type rust 'PrometheusMetricsLayer::new'Length of output: 363
Script:
#!/bin/bash # Extract the surrounding lines in src/object-store/src/util.rs to verify the context of `PrometheusMetricsLayer::new(path_label)` rg --type rust -A 5 -B 5 'PrometheusMetricsLayer::new\(path_label\)' src/object-store/src/util.rsLength of output: 467
Script:
#!/bin/bash # Extract the full definition of the `with_instrument_layers` function from src/object-store/src/util.rs ast-grep --lang rust --pattern 'pub fn with_instrument_layers($_, $_) -> $_ { $$$ }' # Search for invocations of the `with_instrument_layers` function within the codebase rg --type rust 'with_instrument_layers'Length of output: 1683
151-151: Ensure proper layering ofPrometheusMetricsLayer.The
PrometheusMetricsLayeris added to theObjectStorewith thepath_labelparameter. This change seems correct and consistent with the objective of adding apath_labelparameter.Action Required: Verify that the
PrometheusMetricsLayeris correctly layered and aligns with the desired behavior for thePrometheusMetricsLayer.Verification successful
Verified: The
PrometheusMetricsLayeris correctly layered and aligns with the desired behavior.
- Implementation and usage of
PrometheusMetricsLayerare consistent and correct across different files (fs.rs,s3.rs, andutil.rs).- The
path_labelparameter is used appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the layering of `PrometheusMetricsLayer` to ensure correctness. # Test: Search for the usage of `PrometheusMetricsLayer` in layering to confirm that it is correctly layered. rg --type rust 'PrometheusMetricsLayer'Length of output: 751
Script:
#!/bin/bash # Extract the implementation of PrometheusMetricsLayer from src/object-store/src/layers/prometheus.rs echo "Implementation of PrometheusMetricsLayer in src/object-store/src/layers/prometheus.rs:" rg --type rust -A 10 'pub struct PrometheusMetricsLayer' src/object-store/src/layers/prometheus.rs # Extract the context of usage in src/common/datasource/src/object_store/fs.rs echo -e "\nUsage in src/common/datasource/src/object_store/fs.rs:" rg --type rust -A 10 '.layer\(object_store::layers::PrometheusMetricsLayer::new\(true\)\)' src/common/datasource/src/object_store/fs.rs # Extract the context of usage in src/common/datasource/src/object_store/s3.rs echo -e "\nUsage in src/common/datasource/src/object_store/s3.rs:" rg --type rust -A 10 '.layer\(object_store::layers::PrometheusMetricsLayer::new\(true\)\)' src/common/datasource/src/object_store/s3.rs # Extract the context of usage in src/object-store/src/util.rs echo -e "\nUsage in src/object-store/src/util.rs:" rg --type rust -A 10 '.layer\(PrometheusMetricsLayer::new\(path_label\)\)' src/object-store/src/util.rsLength of output: 1868
src/mito2/src/access_layer.rs (3)
211-218: LGTM!The code changes correctly handle the
path_labelparameter in thenew_fs_storefunction.
220-223: LGTM!The code changes correctly handle the
path_labelparameter in thenew_fs_object_storefunction.
225-227: LGTM!The new function
new_fs_cache_storecorrectly sets thepath_labelparameter tofalse.src/mito2/src/cache/write_cache.rs (1)
Line range hint
89-97: LGTM!The code changes correctly use the
new_fs_cache_storefunction to create the local store in thenew_fsfunction.src/object-store/src/layers/prometheus.rs (6)
87-89: LGTM!The addition of the
path_labelfield to thePrometheusMetricsLayerstruct is correct.
91-95: LGTM!The
newmethod correctly initializes thePrometheusMetricsLayerstruct with thepath_labelparameter.
115-117: LGTM!The addition of the
path_labelfield to thePrometheusAccessstruct is correct.
119-126: LGTM!The
get_path_labelmethod correctly handles thepath_labellogic based on the value of thepath_labelfield.
Line range hint
97-107: LGTM!The
layermethod correctly includes thepath_labelfield when creating aPrometheusAccess.
Line range hint
151-478: LGTM!The methods in
PrometheusAccesscorrectly use theget_path_labelmethod to handle thepath_labellogic.
|
@evenyag not sure if we want to turn off path label for IntermediateManager as well. edit: confirmed we need to do that |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4336 +/- ##
==========================================
- Coverage 85.18% 84.90% -0.28%
==========================================
Files 1060 1060
Lines 189069 189076 +7
==========================================
- Hits 161057 160535 -522
- Misses 28012 28541 +529 |
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/mito2/src/sst/index/intermediate.rs (2 hunks)
Additional comments not posted (1)
src/mito2/src/sst/index/intermediate.rs (1)
22-23: LGTM! The use ofnew_fs_cache_storealigns with the PR objectives.The function
init_fsnow correctly usesnew_fs_cache_storeto disable the path label for the cache, addressing the high cardinality issue.Also applies to: 40-41
sunng87
force-pushed
the
fix/prom-cardinality
branch
from
059c883
to
9025573
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 (2)
- src/mito2/src/access_layer.rs (1 hunks)
- src/mito2/src/sst/index/intermediate.rs (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/mito2/src/access_layer.rs
- src/mito2/src/sst/index/intermediate.rs
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/mito2/src/access_layer.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/mito2/src/access_layer.rs
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 fixes high cardinality on path label of opendal metrics. It's an issue introduce in #4277 . Because our file system cache shares same also uses this api, we get fragmented path label due to those usage. This path turns off path label for the cache.
Checklist
Summary by CodeRabbit
new_fs_object_storetonew_fs_cache_storeand adjusted its implementation to include additional instrumentation.