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

chore: put rocks metrics collection behind a feature flag #1922

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

carneiro-cw
Copy link
Contributor

@carneiro-cw carneiro-cw commented Dec 17, 2024

PR Type

Enhancement


Description

  • Introduced a new feature flag "rocks_metrics" for RocksDB-specific metrics collection
  • Updated all occurrences of the "metrics" feature flag to "rocks_metrics" in RocksDB-related files
  • Modified conditional compilation directives to use the new feature flag
  • Adjusted imports and dependencies to align with the new feature flag
  • Maintained backwards compatibility by making "rocks_metrics" depend on "metrics"
  • This change allows for more granular control over metrics collection, specifically for RocksDB

Changes walkthrough 📝

Relevant files
Configuration changes
rocks_cf.rs
Update feature flag for RocksDB metrics                                   

src/eth/storage/permanent/rocks/rocks_cf.rs

  • Changed feature flag from "metrics" to "rocks_metrics" for metrics
    collection
  • +2/-2     
    rocks_config.rs
    Adjust feature flag for RocksDB statistics                             

    src/eth/storage/permanent/rocks/rocks_config.rs

  • Updated feature flag from "metrics" to "rocks_metrics" for RocksDB
    statistics
  • +1/-1     
    rocks_permanent.rs
    Update feature flag for metrics export                                     

    src/eth/storage/permanent/rocks/rocks_permanent.rs

  • Modified feature flag from "metrics" to "rocks_metrics" for exporting
    metrics
  • +1/-1     
    rocks_state.rs
    Refactor RocksDB state metrics feature flag                           

    src/eth/storage/permanent/rocks/rocks_state.rs

  • Changed feature flag from "metrics" to "rocks_metrics" throughout the
    file
  • Updated conditional compilation directives
  • Modified imports to accommodate the new feature flag
  • +10/-8   
    Cargo.toml
    Add new RocksDB metrics feature flag                                         

    Cargo.toml

    • Added new feature flag "rocks_metrics" that depends on "metrics"
    +3/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Performance Impact
    The new feature flag might affect the performance metrics collection for RocksDB. Ensure that this change doesn't inadvertently impact the overall performance monitoring capabilities of the system.

    Logging Consistency
    The error logging for metrics export is now conditional. Ensure that this doesn't lead to inconsistent error reporting across different feature flag configurations.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Align feature flags for consistent metrics functionality across the codebase

    Ensure that the metrics feature is enabled when rocks_metrics is enabled. Consider
    adding a compile-time check or adjusting the feature dependencies in Cargo.toml to
    enforce this relationship.

    src/eth/storage/permanent/rocks/rocks_state.rs [55-56]

    -#[cfg(feature = "metrics")]
    +#[cfg(feature = "rocks_metrics")]
     use crate::infra::metrics;
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies an inconsistency in feature flag usage and proposes a fix that aligns with the PR's changes. This is important for maintaining code consistency and preventing potential runtime errors.

    8
    Ensure proper feature flag dependencies for consistent functionality

    Modify the rocks_metrics feature definition to include metrics as a dependency,
    ensuring that enabling rocks_metrics automatically enables metrics.

    Cargo.toml [205]

     # Enable runtime rocksdb metrics collection.
    -rocks_metrics = ["metrics"]
    +rocks_metrics = ["metrics", "dep:metrics-exporter-prometheus"]
    Suggestion importance[1-10]: 7

    Why: The suggestion proposes a valid improvement to the feature flag dependencies in Cargo.toml, ensuring that enabling 'rocks_metrics' also enables 'metrics'. This change enhances code consistency and prevents potential configuration issues.

    7

    @carneiro-cw carneiro-cw merged commit 4bec71a into main Dec 17, 2024
    35 checks passed
    @carneiro-cw carneiro-cw deleted the rocks_metrics_ff branch December 17, 2024 19:52
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-1110601443

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1243.00, Min: 723.00, Avg: 1137.15, StdDev: 48.75
    TPS Stats: Max: 1239.00, Min: 987.00, Avg: 1103.34, StdDev: 54.93

    Plot: View Plot

    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.

    2 participants