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: TracingConfig, SentryConfig and MetricsConfig #1595

Merged
merged 1 commit into from
Aug 4, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Aug 4, 2024

PR Type

Enhancement, Bug fix


Description

  • Refactored configuration loading and initialization, including renaming functions and adding new ones.
  • Added MetricsConfig and SentryConfig structs and their implementations.
  • Updated RpcServerConfig fields to include rpc_ prefix and adjusted related code.
  • Moved and refactored tracing and metrics initialization code.
  • Removed obsolete files and cleaned up module exports.

Changes walkthrough 📝

Relevant files
Enhancement
16 files
run_with_importer.rs
Update RPC server address field name                                         

src/bin/run_with_importer.rs

  • Renamed config.rpc_server.address to config.rpc_server.rpc_address.
  • +1/-1     
    config.rs
    Refactor configuration loading and initialization               

    src/config.rs

  • Added imports for MetricsConfig and SentryConfig.
  • Renamed load_dotenv to load_dotenv_file.
  • Added load_env_aliases function.
  • Replaced individual Sentry and metrics configurations with
    SentryConfig and MetricsConfig.
  • Renamed init_runtime to init_tokio_runtime.
  • +25/-12 
    rpc_config.rs
    Rename RPC server configuration fields                                     

    src/eth/rpc/rpc_config.rs

    • Renamed fields in RpcServerConfig to include rpc_ prefix.
    +6/-6     
    rpc_server.rs
    Update RPC server field usage                                                       

    src/eth/rpc/rpc_server.rs

    • Updated field names to match changes in RpcServerConfig.
    +4/-4     
    globals.rs
    Refactor global initialization                                                     

    src/globals.rs

  • Refactored to use config::load_dotenv_file and
    config::load_env_aliases.
  • Updated initialization to use new SentryConfig and MetricsConfig.
  • +14/-31 
    metrics_config.rs
    Add MetricsConfig struct                                                                 

    src/infra/metrics/metrics_config.rs

    • Added new MetricsConfig struct and its implementation.
    +67/-0   
    metrics_init.rs
    Remove metrics_init.rs                                                                     

    src/infra/metrics/metrics_init.rs

  • Removed metrics_init.rs and moved its functionality to
    metrics_config.rs.
  • +0/-57   
    mod.rs
    Update metrics module                                                                       

    src/infra/metrics/mod.rs

    • Updated module to use MetricsConfig.
    +2/-2     
    mod.rs
    Clean up infra module                                                                       

    src/infra/mod.rs

    • Removed unused imports and updated module exports.
    +0/-3     
    sentry.rs
    Remove sentry.rs                                                                                 

    src/infra/sentry.rs

  • Removed sentry.rs and moved its functionality to sentry_config.rs.
  • +0/-24   
    mod.rs
    Add SentryConfig module                                                                   

    src/infra/sentry/mod.rs

    • Added sentry_config module and updated exports.
    +3/-0     
    sentry_config.rs
    Add SentryConfig struct                                                                   

    src/infra/sentry/sentry_config.rs

    • Added new SentryConfig struct and its implementation.
    +36/-0   
    mod.rs
    Refactor tracing module                                                                   

    src/infra/tracing/mod.rs

    • Refactored tracing module structure and updated exports.
    +11/-10 
    tracing_config.rs
    Enhance TracingConfig and initialization                                 

    src/infra/tracing/tracing_config.rs

  • Added new fields and methods to TracingConfig.
  • Refactored tracing initialization.
  • +210/-5 
    tracing_services.rs
    Refactor tracing services                                                               

    src/infra/tracing/tracing_services.rs

  • Moved tracing services to a new file and updated their
    implementations.
  • +5/-196 
    main.rs
    Update RPC server address field name in main                         

    src/main.rs

  • Renamed config.rpc_server.address to config.rpc_server.rpc_address.
  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    github-actions bot commented Aug 4, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Missing Logging
    The initialization of the RpcServerConfig is missing logging for the configuration details. Ensure that the initialization logs all relevant configurations.

    Missing Error Handling
    The load_dotenv_file and load_env_aliases functions do not handle potential errors from environment variable operations. Consider adding error handling to improve robustness.

    Missing Metrics Initialization Logging
    The init function in MetricsConfig should log the initialization of each metric to provide better visibility.

    Missing Error Field in Logging
    When logging the failure to create the Sentry exporter, the error should be included in a field named reason.

    Missing Span Fields
    The init function in TracingConfig should record identifiers as span fields to improve traceability.

    Copy link

    github-actions bot commented Aug 4, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure that the metrics exporter is only initialized once to prevent potential conflicts or multiple initializations

    Add a check to ensure that the metrics exporter is only initialized once to prevent
    potential conflicts or multiple initializations.

    src/infra/metrics/metrics_config.rs [40]

    -init_metrics_exporter(self.metrics_exporter_address);
    +static INIT_METRICS_EXPORTER: Once = Once::new();
    +INIT_METRICS_EXPORTER.call_once(|| {
    +    init_metrics_exporter(self.metrics_exporter_address);
    +});
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential issue by ensuring that the metrics exporter is only initialized once, which is crucial for preventing conflicts or multiple initializations.

    9
    Replace unwrap calls with expect to provide descriptive error messages

    The unwrap calls in the opentelemetry_tracer function can cause the program to panic
    if the MetadataKey or value parsing fails. Consider using expect with a descriptive
    error message or handling the error more gracefully.

    src/infra/tracing/tracing_config.rs [194]

    -protocol_metadata.insert(MetadataKey::from_str(key).unwrap(), value.parse().unwrap());
    +protocol_metadata.insert(MetadataKey::from_str(key).expect("Invalid metadata key"), value.parse().expect("Invalid metadata value"));
     
    Suggestion importance[1-10]: 9

    Why: This suggestion improves error handling by providing more context in case of failure, which is crucial for debugging and maintaining robust code. The existing code can cause the program to panic without a clear reason, so this change is highly beneficial.

    9
    Handle potential errors from console_server.serve().await more explicitly by logging and returning a result

    The spawn_named function call inside the tokio_console_layer match arm should handle
    potential errors from the console_server.serve().await call more explicitly,
    possibly by logging the error and returning a result.

    src/infra/tracing/tracing_config.rs [138-142]

     spawn_named("console::grpc-server", async move {
         if let Err(e) = console_server.serve().await {
             tracing::error!(reason = ?e, address = %tokio_console_address, "failed to create tokio-console server");
    +        return Err(e);
         };
    +    Ok(())
     });
     
    Suggestion importance[1-10]: 8

    Why: Explicitly handling potential errors from console_server.serve().await improves the robustness of the code by ensuring that errors are logged and propagated correctly. This change enhances error visibility and handling.

    8
    Performance
    Clone the config once and reuse the cloned instance to improve performance and readability

    Instead of cloning the config multiple times, consider cloning it once and reusing
    the cloned instance. This can improve performance and readability.

    src/bin/run_with_importer.rs [29-32]

    -config.clone().candidate_peers.clone(),
    -Some(config.clone()),
    -config.rpc_server.rpc_address,
    -config.grpc_server_address,
    +let cloned_config = config.clone();
    +cloned_config.candidate_peers.clone(),
    +Some(cloned_config),
    +cloned_config.rpc_server.rpc_address,
    +cloned_config.grpc_server_address,
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves performance and readability by reducing the number of times config is cloned, which is a good practice in Rust.

    8
    Best practice
    Use constants for environment variable keys to avoid potential issues with variable names

    To avoid potential issues with environment variable names, consider using constants
    for the environment variable keys in load_env_aliases.

    src/config.rs [60-66]

    -env_alias("EXECUTOR_CHAIN_ID", "CHAIN_ID");
    -env_alias("EXECUTOR_EVMS", "EVMS");
    -env_alias("EXECUTOR_EVMS", "NUM_EVMS");
    -env_alias("EXECUTOR_REJECT_NOT_CONTRACT", "REJECT_NOT_CONTRACT");
    -env_alias("EXECUTOR_STRATEGY", "STRATEGY");
    -env_alias("TRACING_LOG_FORMAT", "LOG_FORMAT");
    -env_alias("TRACING_URL", "TRACING_COLLECTOR_URL");
    +const EXECUTOR_CHAIN_ID: &str = "EXECUTOR_CHAIN_ID";
    +const CHAIN_ID: &str = "CHAIN_ID";
    +const EXECUTOR_EVMS: &str = "EXECUTOR_EVMS";
    +const NUM_EVMS: &str = "NUM_EVMS";
    +const EXECUTOR_REJECT_NOT_CONTRACT: &str = "EXECUTOR_REJECT_NOT_CONTRACT";
    +const EXECUTOR_STRATEGY: &str = "EXECUTOR_STRATEGY";
    +const TRACING_LOG_FORMAT: &str = "TRACING_LOG_FORMAT";
    +const TRACING_URL: &str = "TRACING_URL";
     
    +env_alias(EXECUTOR_CHAIN_ID, CHAIN_ID);
    +env_alias(EXECUTOR_EVMS, EXECUTOR_EVMS);
    +env_alias(EXECUTOR_EVMS, NUM_EVMS);
    +env_alias(EXECUTOR_REJECT_NOT_CONTRACT, EXECUTOR_REJECT_NOT_CONTRACT);
    +env_alias(EXECUTOR_STRATEGY, EXECUTOR_STRATEGY);
    +env_alias(TRACING_LOG_FORMAT, LOG_FORMAT);
    +env_alias(TRACING_URL, TRACING_COLLECTOR_URL);
    +
    Suggestion importance[1-10]: 7

    Why: Using constants for environment variable keys is a best practice that can help avoid potential issues with variable names and improve code maintainability.

    7
    Use structured logging for errors in tracing_subscriber::registry().try_init()

    The tracing_subscriber::registry().try_init() call can fail, and the error is
    currently only printed. Consider logging this error using a more structured logging
    approach or handling it more gracefully.

    src/infra/tracing/tracing_config.rs [162-163]

    -println!("failed to create tracing registry | reason={:?}", e);
    +tracing::error!(reason = ?e, "failed to create tracing registry");
     Err(e.into())
     
    Suggestion importance[1-10]: 7

    Why: Using structured logging for errors provides better context and integration with logging systems, making it easier to diagnose issues. This is a good practice for maintaining clear and informative logs.

    7
    Maintainability
    Extract the configuration of the RpcServer into a separate function to improve readability and maintainability

    To improve readability and maintainability, consider extracting the configuration of
    the RpcServer into a separate function.

    src/eth/rpc/rpc_server.rs [121-125]

    -let server = RpcServer::builder()
    -    .set_rpc_middleware(rpc_middleware)
    -    .set_http_middleware(http_middleware)
    -    .set_id_provider(RandomStringIdProvider::new(8))
    -    .max_connections(rpc_config.rpc_max_connections)
    -    .build(rpc_config.rpc_address)
    -    .await?;
    +fn configure_rpc_server(rpc_config: &RpcServerConfig, rpc_middleware: Middleware, http_middleware: Middleware) -> RpcServer {
    +    RpcServer::builder()
    +        .set_rpc_middleware(rpc_middleware)
    +        .set_http_middleware(http_middleware)
    +        .set_id_provider(RandomStringIdProvider::new(8))
    +        .max_connections(rpc_config.rpc_max_connections)
    +        .build(rpc_config.rpc_address)
    +        .await
    +        .expect("failed to build RPC server")
    +}
     
    +let server = configure_rpc_server(&rpc_config, rpc_middleware, http_middleware);
    +
    Suggestion importance[1-10]: 6

    Why: Extracting the configuration into a separate function can improve code readability and maintainability, but it is not a critical change.

    6
    Enhancement
    Simplify redundant clone calls

    The config.clone().candidate_peers.clone() call is redundant and can be simplified
    to config.candidate_peers.clone().

    src/main.rs [22]

    -config.clone().candidate_peers.clone(),
    +config.candidate_peers.clone(),
     
    Suggestion importance[1-10]: 6

    Why: Simplifying redundant clone calls improves code readability and efficiency. While this is a minor enhancement, it contributes to cleaner and more maintainable code.

    6

    @dinhani-cw dinhani-cw merged commit 7a6372c into main Aug 4, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the config branch August 4, 2024 02:54
    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