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

metrics: metrify cache slot reads #1912

Merged
merged 2 commits into from
Dec 12, 2024
Merged

metrics: metrify cache slot reads #1912

merged 2 commits into from
Dec 12, 2024

Conversation

carneiro-cw
Copy link
Contributor

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

PR Type

Enhancement


Description

  • Introduced a new label constant CACHE for better categorization of cache-related operations
  • Implemented metrics for cache slot reads using the timed function to measure performance
  • Added metrics increment for storage read slot operations, specifically for cache reads
  • Enhanced the read_slot function to provide more detailed performance insights
  • Improved observability of the cache system in the Stratus storage implementation

Changes walkthrough 📝

Relevant files
Enhancement
stratus_storage.rs
Add metrics for cache slot reads                                                 

src/eth/storage/stratus_storage.rs

  • Added a new label constant CACHE for cache-related operations
  • Implemented metrics for cache slot reads using the timed function
  • Incremented storage read slot metrics with cache-specific information
  • +4/-1     

    💡 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

    Performance Monitoring
    The PR introduces metrics for cache slot reads, which is good for performance monitoring. However, it's important to ensure that the added instrumentation doesn't introduce significant overhead, especially in hot paths.

    Error Handling
    The PR modifies the cache read logic, but it's not clear how errors are handled if the timed function fails. Consider reviewing the error handling strategy for this operation.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Prevent early return from function to ensure all necessary logic is executed

    The return statement inside the if let block will cause an early return from the
    entire read_slot function, potentially bypassing important logic. Consider breaking
    the label and returning the slot at the end of the function instead.

    src/eth/storage/stratus_storage.rs [204-208]

     if let Some(slot) = timed(|| self.cache.get_slot(address, index)).with(|m| {
         metrics::inc_storage_read_slot(m.elapsed, label::CACHE, point_in_time, true);
     }) {
    -    return Ok(slot);
    +    break 'query slot;
     };
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential issue where an early return could bypass important logic. Changing to 'break' with the label ensures all necessary code is executed, improving the function's reliability and correctness.

    8

    @carneiro-cw carneiro-cw enabled auto-merge (squash) December 12, 2024 15:01
    @carneiro-cw carneiro-cw disabled auto-merge December 12, 2024 15:01
    @carneiro-cw carneiro-cw changed the title enha: metrify cache slot reads metrics: metrify cache slot reads Dec 12, 2024
    @carneiro-cw carneiro-cw enabled auto-merge (squash) December 12, 2024 15:01
    @carneiro-cw carneiro-cw disabled auto-merge December 12, 2024 15:12
    @carneiro-cw carneiro-cw enabled auto-merge (squash) December 12, 2024 15:13
    @carneiro-cw carneiro-cw merged commit a50187d into main Dec 12, 2024
    34 checks passed
    @carneiro-cw carneiro-cw deleted the cache_read_slot_metric branch December 12, 2024 15:19
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-900199182

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1122.00, Min: 636.00, Avg: 988.44, StdDev: 60.09
    TPS Stats: Max: 1136.00, Min: 819.00, Avg: 958.64, StdDev: 64.01

    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