refactor(katana): separate metrics address and port into 2 args #2537
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications to the Changes
Possibly related PRs
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
crates/katana/node/src/config/metrics.rs (2)
3-6: Ohayo, sensei! These constants look great!The addition of
DEFAULT_METRICS_ADDRandDEFAULT_METRICS_PORTprovides sensible defaults for the metrics server configuration. Usinglocalhostas the default address is a safe choice.Consider adding a brief comment explaining why port 9100 was chosen as the default. This could be helpful for future maintainers.
17-21: Ohayo, sensei! The newsocket_addrmethod looks great!This method provides a convenient way to obtain a
SocketAddrfrom the separatedaddrandportfields. It's a smart addition that maintains compatibility with code expecting aSocketAddr.Consider adding a brief example in the method's documentation to show how it can be used. This could be helpful for developers using this method.
bin/katana/src/cli/node.rs (1)
Line range hint
185-187: Ohayo, sensei! Minor Grammatical Correction inlong_helpIn the
long_helpfor thechain_idargument, there's a minor grammatical error. The phrase "then it'd used as the actual chain ID" should be "then it will be used as the actual chain ID."Apply this diff to correct it:
#[arg(long_help = "The chain ID. If a raw hex string (`0x` prefix) is provided, then it\ -ed used as the actual chain ID. Otherwise, it's represented as the raw \ + will be used as the actual chain ID. Otherwise, it's represented as the raw \ ASCII values. It must be a valid Cairo short string.")]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- bin/katana/src/cli/node.rs (7 hunks)
- crates/katana/node/src/config/metrics.rs (1 hunks)
- crates/katana/node/src/lib.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
crates/katana/node/src/config/metrics.rs (3)
1-1: Ohayo, sensei! LGTM!The import statement has been updated to include
IpAddrandIpv4Addr, which aligns perfectly with the changes made in the rest of the file. Good job on keeping the imports consistent!
12-15: Ohayo, sensei! Excellent refactoring!The separation of
addrandportin theMetricsConfigstruct is a great improvement. Here's why:
- It provides more flexibility in configuration.
- It aligns well with the new constants added earlier.
- Using
IpAddrfor theaddrfield is more appropriate now that the port is separate.This change will make it easier to configure and manage the metrics server settings.
1-21: Ohayo, sensei! Overall, these changes are excellent!The refactoring of the
MetricsConfigstructure and the addition of new constants and methods have significantly improved the flexibility and usability of the metrics configuration. Here's a summary of the key improvements:
- Separation of address and port in
MetricsConfigfor more granular control.- Addition of default constants for easy configuration.
- New
socket_addrmethod for backward compatibility.These changes will make it easier for users to configure and manage metrics settings while maintaining compatibility with existing code. Great work on this refactoring, sensei!
crates/katana/node/src/lib.rs (1)
Line range hint
107-115: Ohayo sensei! The metrics endpoint initialization looks solid.The use of
cfg.socket_addr()neatly encapsulates the logic for retrieving the socket address, enhancing code clarity and maintainability. Theprometheus_exporter::servefunction is correctly invoked with the appropriate parameters, ensuring the metrics server is set up properly.bin/katana/src/cli/node.rs (3)
14-14: Ohayo, sensei! ImportingIpAddrfor Address ConfigurationThe addition of
use std::net::IpAddr;is necessary for handling IP addresses in the new metrics and server options.
29-29: Ohayo, sensei! Importing Metrics ConfigurationIncluding
MetricsConfig,DEFAULT_METRICS_ADDR, andDEFAULT_METRICS_PORTis essential for the new metrics functionality.
101-101: Ohayo, sensei! Adding Metrics Options toNodeArgsThe inclusion of
pub metrics: MetricsOptionsinNodeArgsintegrates the new metrics configuration into the application's arguments.
| if self.metrics.metrics { | ||
| Some(MetricsConfig { addr: self.metrics.metrics_addr, port: self.metrics.metrics_port }) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Adjusting metrics_config for Field Name Change
Following the renaming of the metrics field to enabled, ensure that the metrics_config method reflects this change.
Apply this diff to update the field reference:
fn metrics_config(&self) -> Option<MetricsConfig> {
- if self.metrics.metrics {
+ if self.metrics.enabled {
Some(MetricsConfig { addr: self.metrics.metrics_addr, port: self.metrics.metrics_port })
} else {
None
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.metrics.metrics { | |
| Some(MetricsConfig { addr: self.metrics.metrics_addr, port: self.metrics.metrics_port }) | |
| } else { | |
| None | |
| } | |
| if self.metrics.enabled { | |
| Some(MetricsConfig { addr: self.metrics.metrics_addr, port: self.metrics.metrics_port }) | |
| } else { | |
| None | |
| } |
| #[command(next_help_heading = "Metrics options")] | ||
| pub struct MetricsOptions { | ||
| /// Whether to enable metrics. | ||
| /// | ||
| /// For now, metrics will still be collected even if this flag is not set. This only | ||
| /// controls whether the metrics server is started or not. | ||
| #[arg(long)] | ||
| pub metrics: bool, | ||
|
|
||
| /// The metrics will be served at the given address. | ||
| #[arg(long = "metrics.addr")] | ||
| #[arg(value_name = "ADD")] | ||
| #[arg(requires = "metrics")] | ||
| #[arg(default_value_t = DEFAULT_METRICS_ADDR)] | ||
| pub metrics_addr: IpAddr, | ||
|
|
||
| /// The metrics will be served at the given port. | ||
| #[arg(long = "metrics.port")] | ||
| #[arg(value_name = "PORT")] | ||
| #[arg(requires = "metrics")] | ||
| #[arg(default_value_t = DEFAULT_METRICS_PORT)] | ||
| pub metrics_port: u16, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Possible Typos and Clarity Improvements in MetricsOptions
-
Typo in
value_nameformetrics_addrThe
value_nameformetrics_addris set to"ADD", which seems like a typo. Consider changing it to"ADDR"for clarity.Apply this diff to fix the typo:
#[arg(long = "metrics.addr")] -#[arg(value_name = "ADD")] +#[arg(value_name = "ADDR")] #[arg(requires = "metrics")]
-
Consider Renaming the
metricsField to Improve ClarityUsing
self.metrics.metricsmay cause confusion due to the repetitive naming. Consider renaming the boolean fieldmetricsinMetricsOptionstoenabledorenable_metricsto enhance readability.Apply this diff to rename the field:
pub struct MetricsOptions { - pub metrics: bool, + pub enabled: bool,Update the references accordingly in the code:
In
metrics_configmethod:fn metrics_config(&self) -> Option<MetricsConfig> { - if self.metrics.metrics { + if self.metrics.enabled {Update the
#[arg]attribute:#[arg(long)] -pub metrics: bool, +pub enabled: bool,
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[command(next_help_heading = "Metrics options")] | |
| pub struct MetricsOptions { | |
| /// Whether to enable metrics. | |
| /// | |
| /// For now, metrics will still be collected even if this flag is not set. This only | |
| /// controls whether the metrics server is started or not. | |
| #[arg(long)] | |
| pub metrics: bool, | |
| /// The metrics will be served at the given address. | |
| #[arg(long = "metrics.addr")] | |
| #[arg(value_name = "ADD")] | |
| #[arg(requires = "metrics")] | |
| #[arg(default_value_t = DEFAULT_METRICS_ADDR)] | |
| pub metrics_addr: IpAddr, | |
| /// The metrics will be served at the given port. | |
| #[arg(long = "metrics.port")] | |
| #[arg(value_name = "PORT")] | |
| #[arg(requires = "metrics")] | |
| #[arg(default_value_t = DEFAULT_METRICS_PORT)] | |
| pub metrics_port: u16, | |
| } | |
| #[command(next_help_heading = "Metrics options")] | |
| pub struct MetricsOptions { | |
| /// Whether to enable metrics. | |
| /// | |
| /// For now, metrics will still be collected even if this flag is not set. This only | |
| /// controls whether the metrics server is started or not. | |
| #[arg(long)] | |
| pub metrics: bool, | |
| /// The metrics will be served at the given address. | |
| #[arg(long = "metrics.addr")] | |
| #[arg(value_name = "ADDR")] | |
| #[arg(requires = "metrics")] | |
| #[arg(default_value_t = DEFAULT_METRICS_ADDR)] | |
| pub metrics_addr: IpAddr, | |
| /// The metrics will be served at the given port. | |
| #[arg(long = "metrics.port")] | |
| #[arg(value_name = "PORT")] | |
| #[arg(requires = "metrics")] | |
| #[arg(default_value_t = DEFAULT_METRICS_PORT)] | |
| pub metrics_port: u16, | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2537 +/- ##
==========================================
- Coverage 69.28% 69.27% -0.01%
==========================================
Files 388 389 +1
Lines 50012 50022 +10
==========================================
+ Hits 34652 34655 +3
- Misses 15360 15367 +7 ☔ View full report in Codecov by Sentry. |
* refactor(katana): separate metrics address and port into 2 args (#2537) * refactor(katana-cli): rename server args * refactor(katana-cli): explicit development related args * chore(katana-cli): rename var * fix * refactor cli * move gas price to gas price oracle options * update node bindings to new cli args * update ci * revert to free port * fmt * remove l1.provider * update * make fixed gpo sovereign * remove broken link
* refactor(katana): separate metrics address and port into 2 args (#2537) * refactor(katana-cli): rename server args * refactor(katana-cli): explicit development related args * chore(katana-cli): rename var * fix * refactor cli * move gas price to gas price oracle options * update node bindings to new cli args * update ci * revert to free port * fmt * remove l1.provider * update * make fixed gpo sovereign * remove broken link * fix * feat: add config file with manual merge for now * fix: rename cors_domain to cors_origins --------- Co-authored-by: Ammar Arif <[email protected]>
replace
--metrics 0.0.0.0:9100to--metrics --metrics.addr 0.0.0.0 --metrics.port 9100.by default, if just do
--metricsit will use127.0.0.0:9100. but other values can be specified using the new appropriately named flags