Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: split metrics module #845

Merged
merged 1 commit into from
May 15, 2024
Merged

refactor: split metrics module #845

merged 1 commit into from
May 15, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner May 15, 2024 14:32
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, due to the extensive changes across multiple files involving refactoring and reorganization of metrics-related code, which requires a detailed understanding of the metrics system and its integration.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The renaming of LabelValue to MetricLabelValue in storage_point_in_time.rs might cause issues if there are other parts of the codebase that still reference the old name but were not updated in this PR.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/primitives/storage_point_in_time.rs
suggestion      

Ensure that all references to LabelValue are updated to MetricLabelValue across the entire codebase to avoid reference errors. [important]

relevant lineuse crate::infra::metrics::MetricLabelValue;

relevant filesrc/infra/metrics/metrics_init.rs
suggestion      

Consider handling the potential error from builder.set_buckets(&BUCKET_FOR_DURATION) more gracefully instead of using unwrap(), to prevent possible panics at runtime. [important]

relevant linebuilder = builder.set_buckets(&BUCKET_FOR_DURATION).unwrap();

relevant filesrc/infra/metrics/metrics_macros.rs
suggestion      

Add error handling for the metrics::counter! and metrics::histogram! macro invocations to manage potential failures gracefully. [important]

relevant linemetrics::counter!(stringify!([]), n, labels);

relevant filesrc/infra/metrics/metrics_macros.rs
suggestion      

Consider adding logging or tracing within the metric recording functions to aid in debugging and monitoring metric recording failures or unexpected behavior. [medium]

relevant linepub fn [

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Error handling
Implement error handling for metrics initialization to enhance robustness

Add error handling for the builder.install() method to gracefully handle potential
initialization failures without panicking.

src/infra/metrics/metrics_init.rs [58]

-builder.install().expect("failed to start metrics");
+if let Err(e) = builder.install() {
+    tracing::error!("Failed to start metrics: {}", e);
+    return;
+}
 
Suggestion importance[1-10]: 8

Why: This suggestion correctly addresses a critical aspect of error handling by recommending a non-panicking approach if the metrics initialization fails, which enhances the robustness of the application.

8
Enhancement
Implement conversion from a reference to an Option> to MetricLabelValue

Implement From<&Option<Cow<'static, str>>> for MetricLabelValue to handle cases where the
input might be a reference to an Option, which is common in Rust codebases.

src/infra/metrics/metrics_types.rs [47-54]

-impl From<Option<Cow<'static, str>>> for MetricLabelValue {
-    fn from(value: Option<Cow<'static, str>>) -> Self {
+impl From<&Option<Cow<'static, str>>> for MetricLabelValue {
+    fn from(value: &Option<Cow<'static, str>>) -> Self {
         match value {
-            Some(str) => Self::Some(str.into_owned()),
+            Some(str) => Self::Some(str.to_string()),
             None => Self::None,
         }
     }
 }
 
Suggestion importance[1-10]: 7

Why: The suggestion correctly identifies an enhancement for the MetricLabelValue implementation, improving flexibility in handling references to Option<Cow<'static, str>>.

7
Best practice
Improve the naming convention of metric constants for better clarity and conflict avoidance

Use a more descriptive naming convention for the generated metric constants to avoid
potential naming conflicts and increase code readability.

src/infra/metrics/metrics_macros.rs [17]

-pub const [<METRIC_ $name:upper>]: &str = stringify!([<stratus_ $name>]);
+pub const [<METRIC_STRATUS_ $name:upper>]: &str = stringify!([<stratus_ $name>]);
 
Suggestion importance[1-10]: 6

Why: The suggestion is valid and improves readability and avoids potential conflicts by suggesting a more descriptive naming convention for metric constants.

6

@dinhani-cw dinhani-cw merged commit 187a8bb into main May 15, 2024
26 checks passed
@dinhani-cw dinhani-cw deleted the metrics branch May 15, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant