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(katana): separate metrics address and port into 2 args #2537

Merged
merged 2 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions bin/katana/src/cli/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,13 @@
//! for more info.

use std::collections::HashSet;
use std::net::{IpAddr, SocketAddr};
use std::net::IpAddr;
use std::path::PathBuf;

use alloy_primitives::U256;
use anyhow::{Context, Result};
use clap::{Args, Parser};
use console::Style;
use dojo_utils::parse::parse_socket_address;
use katana_core::backend::config::{Environment, StarknetConfig};
use katana_core::constants::{
DEFAULT_ETH_L1_GAS_PRICE, DEFAULT_INVOKE_MAX_STEPS, DEFAULT_SEQUENCER_ADDRESS,
Expand All @@ -27,7 +26,7 @@
use katana_core::service::messaging::MessagingConfig;
use katana_node::config::db::DbConfig;
use katana_node::config::dev::DevConfig;
use katana_node::config::metrics::MetricsConfig;
use katana_node::config::metrics::{MetricsConfig, DEFAULT_METRICS_ADDR, DEFAULT_METRICS_PORT};
use katana_node::config::rpc::{
ApiKind, RpcConfig, DEFAULT_RPC_ADDR, DEFAULT_RPC_MAX_CONNECTIONS, DEFAULT_RPC_PORT,
};
Expand Down Expand Up @@ -83,12 +82,6 @@
#[arg(help = "Output logs in JSON format.")]
pub json_log: bool,

/// Enable Prometheus metrics.
///
/// The metrics will be served at the given interface and port.
#[arg(long, value_name = "SOCKET", value_parser = parse_socket_address, help_heading = "Metrics")]
pub metrics: Option<SocketAddr>,

#[arg(long)]
#[arg(requires = "rpc_url")]
#[arg(value_name = "BLOCK_NUMBER")]
Expand All @@ -105,20 +98,46 @@
pub messaging: Option<MessagingConfig>,

#[command(flatten)]
#[command(next_help_heading = "Server options")]
pub metrics: MetricsOptions,

#[command(flatten)]
pub server: ServerOptions,

#[command(flatten)]
#[command(next_help_heading = "Starknet options")]
pub starknet: StarknetOptions,

#[cfg(feature = "slot")]
#[command(flatten)]
#[command(next_help_heading = "Slot options")]
pub slot: SlotOptions,
}

#[derive(Debug, Args, Clone)]
#[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,

Check warning on line 122 in bin/katana/src/cli/node.rs

View check run for this annotation

Codecov / codecov/patch

bin/katana/src/cli/node.rs#L122

Added line #L122 was not covered by tests

/// 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,

Check warning on line 129 in bin/katana/src/cli/node.rs

View check run for this annotation

Codecov / codecov/patch

bin/katana/src/cli/node.rs#L129

Added line #L129 was not covered by tests

/// 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,

Check warning on line 136 in bin/katana/src/cli/node.rs

View check run for this annotation

Codecov / codecov/patch

bin/katana/src/cli/node.rs#L136

Added line #L136 was not covered by tests
}
Comment on lines +115 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Ohayo, sensei! Possible Typos and Clarity Improvements in MetricsOptions

  1. Typo in value_name for metrics_addr

    The value_name for metrics_addr is 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")]
  2. Consider Renaming the metrics Field to Improve Clarity

    Using self.metrics.metrics may cause confusion due to the repetitive naming. Consider renaming the boolean field metrics in MetricsOptions to enabled or enable_metrics to 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_config method:

     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.

Suggested change
#[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,
}


#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Server options")]
pub struct ServerOptions {
#[arg(short, long)]
#[arg(default_value_t = DEFAULT_RPC_PORT)]
Expand All @@ -142,6 +161,7 @@
}

#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Starknet options")]
pub struct StarknetOptions {
#[arg(long)]
#[arg(default_value = "0")]
Expand All @@ -163,7 +183,6 @@
pub disable_validate: bool,

#[command(flatten)]
#[command(next_help_heading = "Environment options")]
pub environment: EnvironmentOptions,

#[arg(long)]
Expand All @@ -173,6 +192,7 @@
}

#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Environment options")]
pub struct EnvironmentOptions {
#[arg(long)]
#[arg(help = "The chain ID.")]
Expand Down Expand Up @@ -208,6 +228,7 @@

#[cfg(feature = "slot")]
#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Slot options")]
pub struct SlotOptions {
#[arg(hide = true)]
#[arg(long = "slot.controller")]
Expand Down Expand Up @@ -364,7 +385,11 @@
}

fn metrics_config(&self) -> Option<MetricsConfig> {
self.metrics.map(|addr| MetricsConfig { addr })
if self.metrics.metrics {
Some(MetricsConfig { addr: self.metrics.metrics_addr, port: self.metrics.metrics_port })

Check warning on line 389 in bin/katana/src/cli/node.rs

View check run for this annotation

Codecov / codecov/patch

bin/katana/src/cli/node.rs#L389

Added line #L389 was not covered by tests
} else {
None
}
Comment on lines +388 to +392
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
}

}
}

Expand Down
18 changes: 16 additions & 2 deletions crates/katana/node/src/config/metrics.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
use std::net::SocketAddr;
use std::net::{IpAddr, Ipv4Addr, SocketAddr};

/// Metrics server default address.
pub const DEFAULT_METRICS_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST);
/// Metrics server default port.
pub const DEFAULT_METRICS_PORT: u16 = 9100;

/// Node metrics configurations.
#[derive(Debug, Clone)]
pub struct MetricsConfig {
/// The address to bind the metrics server to.
pub addr: SocketAddr,
pub addr: IpAddr,
/// The port to bind the metrics server to.
pub port: u16,
}

impl MetricsConfig {
/// Returns the [`SocketAddr`] for the metrics server.
pub fn socket_addr(&self) -> SocketAddr {
SocketAddr::new(self.addr, self.port)
}

Check warning on line 21 in crates/katana/node/src/config/metrics.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/node/src/config/metrics.rs#L19-L21

Added lines #L19 - L21 were not covered by tests
}
2 changes: 1 addition & 1 deletion crates/katana/node/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
/// This method will start all the node process, running them until the node is stopped.
pub async fn launch(self) -> Result<LaunchedNode> {
if let Some(ref cfg) = self.metrics_config {
let addr = cfg.addr;
let addr = cfg.socket_addr();

Check warning on line 107 in crates/katana/node/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/node/src/lib.rs#L107

Added line #L107 was not covered by tests
let mut reports = Vec::new();

if let Some(ref db) = self.db {
Expand Down
Loading