Early initialize lazy static metrics to prime them #6657
Conversation
Since all metrics are `lazy_static!`, it means thay are not primed (set to 0) immediately, but at a point they are first accessed. This means, that they do not appear in the `node/metrics` endpoint until they were accessed for the first time. This PR forcibly initializes the lazy static metrics everywhere, where they might not be used early.
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
transport/protocol/src/lib.rs (2)
163-175: Consider adding error handling for metric initializationWhile the early initialization of metrics aligns with the PR objectives, the
lazy_static::initializecalls could panic if metric registration fails (e.g., due to duplicate metric names). Consider wrapping the initialization in atracing::warn!to gracefully handle failures in production.#[cfg(all(feature = "prometheus", not(test)))] { // Initialize the lazy statics here - lazy_static::initialize(&METRIC_RECEIVED_ACKS); - lazy_static::initialize(&METRIC_SENT_ACKS); - lazy_static::initialize(&METRIC_TICKETS_COUNT); - lazy_static::initialize(&METRIC_PACKET_COUNT); - lazy_static::initialize(&METRIC_PACKET_COUNT_PER_PEER); - lazy_static::initialize(&METRIC_REPLAYED_PACKET_COUNT); - lazy_static::initialize(&METRIC_REJECTED_TICKETS_COUNT); - lazy_static::initialize(&METRIC_QUEUE_SIZE); - lazy_static::initialize(&METRIC_MIXER_AVERAGE_DELAY); + for metric in [ + &METRIC_RECEIVED_ACKS, + &METRIC_SENT_ACKS, + &METRIC_TICKETS_COUNT, + &METRIC_PACKET_COUNT, + &METRIC_PACKET_COUNT_PER_PEER, + &METRIC_REPLAYED_PACKET_COUNT, + &METRIC_REJECTED_TICKETS_COUNT, + &METRIC_QUEUE_SIZE, + &METRIC_MIXER_AVERAGE_DELAY, + ] { + if let Err(e) = std::panic::catch_unwind(|| lazy_static::initialize(metric)) { + tracing::warn!("Failed to initialize metric: {:?}", e); + } + } }
Line range hint
449-458: Consider using integer arithmetic for mixer delay calculationThe current implementation uses floating-point arithmetic for calculating the average delay, which could lead to precision issues over time. Consider using integer arithmetic or a more robust averaging algorithm.
- let weight = 1.0f64 / cfg.metric_delay_window as f64; - METRIC_MIXER_AVERAGE_DELAY.set( - (weight * random_delay.as_millis() as f64) - + ((1.0f64 - weight) * METRIC_MIXER_AVERAGE_DELAY.get()), - ); + // Use exponential moving average with integer arithmetic + let window = cfg.metric_delay_window as u128; + let current_avg = METRIC_MIXER_AVERAGE_DELAY.get() as u128; + let new_delay = random_delay.as_millis(); + let new_avg = ((current_avg * (window - 1) + new_delay) / window) as f64; + METRIC_MIXER_AVERAGE_DELAY.set(new_avg);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
transport/protocol/Cargo.toml(1 hunks)transport/protocol/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- transport/protocol/Cargo.toml
🔇 Additional comments (1)
transport/protocol/src/lib.rs (1)
163-175: LGTM! The changes effectively address the metrics initialization issue.
The implementation successfully ensures that metrics are initialized early and visible in the node/metrics endpoint from the start. The code is well-structured with proper feature isolation and comprehensive metric coverage.
Since all metrics are
lazy_static!, it means they are not primed (set to 0) immediately, but at a point they are first accessed.This means, that they do not appear in the
node/metricsendpoint until they were accessed for the first time.This PR forcibly initializes the lazy static metrics everywhere, where they might not be used early.