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

feat: compute rpc active connections metrics from jsonrpsee logs #1590

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

dinhani-cw
Copy link
Contributor

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

PR Type

Enhancement, Configuration changes


Description

  • Simplified the rpc_requests_active gauge definition by removing labels.
  • Added temporary metrics collection from tracing events, including a new event_to_metrics function to parse and set metrics.
  • Modified the TracingLog struct to use JsonValue for fields instead of SerializeFieldMap.
  • Updated the RUST_LOG configuration to include jsonrpsee-server=debug for better logging visibility.

Changes walkthrough 📝

Relevant files
Enhancement
metrics_definitions.rs
Simplified `rpc_requests_active` gauge definition               

src/infra/metrics/metrics_definitions.rs

  • Modified rpc_requests_active gauge to remove labels.
+1/-1     
tracing.rs
Added metrics collection from tracing events                         

src/infra/tracing/tracing.rs

  • Added temporary metrics collection from events.
  • Introduced event_to_metrics function to parse and set metrics.
  • Modified TracingLog struct to use JsonValue for fields.
  • +27/-3   
    Configuration changes
    stratus.env.local
    Updated logging configuration for jsonrpsee-server             

    config/stratus.env.local

  • Updated RUST_LOG configuration to include jsonrpsee-server=debug.
  • +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 1, 2024

    PR Reviewer Guide 🔍

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

    Logging and Monitoring
    The event_to_metrics function uses metrics::set_rpc_requests_active without logging the operation. Ensure that the metrics update is logged for better observability.

    Dynamic Fields in Tracing
    Avoid using dynamic fields in tracing events. Instead of parsing and using dynamic fields, consider adding these fields directly to the tracing event.

    Unwrap Usage
    The event_to_metrics function contains multiple unwrap-like operations using and_then, strip_prefix, and split_once. Consider using expect to provide context in case of failure.

    Copy link

    github-actions bot commented Aug 1, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add missing labels to the rpc_requests_active gauge for consistency

    The rpc_requests_active gauge should include the same labels (client, method) as
    before to ensure consistency and granularity in metrics.

    src/infra/metrics/metrics_definitions.rs [8]

    -gauge rpc_requests_active{},
    +gauge rpc_requests_active{client, method},
     
    Suggestion importance[1-10]: 9

    Why: This suggestion ensures consistency and granularity in metrics by including the same labels (client, method) as before. This is crucial for maintaining the integrity of the metrics data.

    9
    Best practice
    Add error logging for parsing failures in the event_to_metrics function

    The event_to_metrics function should handle potential errors more gracefully by
    logging them instead of silently returning.

    src/infra/tracing/tracing.rs [346]

    -let Ok(current) = current.parse::<u64>() else { return };
    +let Ok(current) = current.parse::<u64>() else {
    +    eprintln!("Failed to parse current connections count");
    +    return;
    +};
     
    Suggestion importance[1-10]: 8

    Why: Adding error logging for parsing failures improves the robustness of the code by providing visibility into potential issues, which is a best practice.

    8
    Maintainability
    Use a more descriptive variable name after stripping the prefix in the event_to_metrics function

    The event_to_metrics function should use a more descriptive name for the message
    variable after stripping the prefix to avoid confusion.

    src/infra/tracing/tracing.rs [342]

    -let Some(message) = message.strip_prefix("Accepting new connection ") else {
    +let Some(connection_info) = message.strip_prefix("Accepting new connection ") else {
     
    Suggestion importance[1-10]: 7

    Why: Using a more descriptive variable name enhances code readability and maintainability, making it easier for future developers to understand the code.

    7
    Enhancement
    Split the RUST_LOG environment variable into multiple lines for better readability

    The RUST_LOG environment variable should be split into multiple lines for better
    readability and maintainability.

    config/stratus.env.local [1]

    -RUST_LOG=info,stratus::eth::rpc::rpc_subscriptions::rx=off,stratus::eth::consensus::rx=off,stratus::eth::consensus=off,jsonrpsee-server=debug
    +RUST_LOG=info,\
    +stratus::eth::rpc::rpc_subscriptions::rx=off,\
    +stratus::eth::consensus::rx=off,\
    +stratus::eth::consensus=off,\
    +jsonrpsee-server=debug
     
    Suggestion importance[1-10]: 6

    Why: Splitting the RUST_LOG environment variable into multiple lines improves readability and maintainability, although it is a minor enhancement.

    6

    @dinhani-cw dinhani-cw enabled auto-merge (squash) August 1, 2024 20:44
    @dinhani-cw dinhani-cw merged commit 533e9f4 into main Aug 1, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the log-to-metrics branch August 1, 2024 20:45
    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