nits

Author: patrick-ogrady

broadcast/src/buffered/metrics.rs

@@ -22,7 +22,6 @@ impl SequencerLabel {
 }
 
 /// Metrics for the [super::Engine]
-#[derive(Default)]
 pub struct Metrics {
     /// Number of broadcasts received by peer
     pub peer: Family<SequencerLabel, Counter>,
@@ -40,32 +39,32 @@ pub struct Metrics {
 impl Metrics {
     /// Create and return a new set of metrics, registered with the given context.
     pub fn init<E: RuntimeMetrics>(context: &E) -> Self {
-        let metrics = Self::default();
-        context.register(
-            "peer",
-            "Number of broadcasts received by peer",
-            metrics.peer.clone(),
-        );
-        context.register(
-            "receive",
-            "Number of received messages by status",
-            metrics.receive.clone(),
-        );
-        context.register(
-            "subscribe",
-            "Number of `subscribe` requests by status",
-            metrics.subscribe.clone(),
-        );
-        context.register(
-            "get",
-            "Number of `get` requests by status",
-            metrics.get.clone(),
-        );
-        context.register(
-            "waiters",
-            "Number of digests being awaited",
-            metrics.waiters.clone(),
-        );
-        metrics
+        Self {
+            peer: context.register(
+                "peer",
+                "Number of broadcasts received by peer",
+                Family::default(),
+            ),
+            receive: context.register(
+                "receive",
+                "Number of received messages by status",
+                status::Counter::default(),
+            ),
+            subscribe: context.register(
+                "subscribe",
+                "Number of `subscribe` requests by status",
+                status::Counter::default(),
+            ),
+            get: context.register(
+                "get",
+                "Number of `get` requests by status",
+                status::Counter::default(),
+            ),
+            waiters: context.register(
+                "waiters",
+                "Number of digests being awaited",
+                Gauge::default(),
+            ),
+        }
     }
 }

collector/src/p2p/engine.rs

@@ -74,16 +74,10 @@ where
         let mailbox: Mailbox<P, Rq> = Mailbox::new(tx);
 
         // Create metrics
-        let outstanding = Gauge::default();
-        let requests = Counter::default();
-        let responses = Counter::default();
-        context.register(
-            "outstanding",
-            "outstanding commitments",
-            outstanding.clone(),
-        );
-        context.register("requests", "processed requests", requests.clone());
-        context.register("responses", "sent responses", responses.clone());
+        let outstanding =
+            context.register("outstanding", "outstanding commitments", Gauge::default());
+        let requests = context.register("requests", "processed requests", Counter::default());
+        let responses = context.register("responses", "sent responses", Counter::default());
 
         (
             Self {

consensus/src/aggregation/metrics.rs

@@ -21,44 +21,37 @@ pub struct Metrics {
 impl Metrics {
     /// Create and return a new set of metrics, registered with the given context.
     pub fn init<E: RuntimeMetrics>(context: &E) -> Self {
-        let tip = Gauge::default();
-        context.register("tip", "Lowest height without a certificate", tip.clone());
-        let digest = status::Counter::default();
-        context.register(
-            "digest",
-            "Number of digests returned by the automaton by status",
-            digest.clone(),
-        );
-        let acks = status::Counter::default();
-        context.register(
-            "acks",
-            "Number of Ack messages processed by status",
-            acks.clone(),
-        );
-        let certificates = Counter::default();
-        context.register(
-            "certificates",
-            "Number of certificates produced",
-            certificates.clone(),
-        );
-        let rebroadcast = status::Counter::default();
-        context.register(
+        let _ = context.register(
             "rebroadcast",
             "Number of rebroadcast attempts by status",
-            rebroadcast,
+            status::Counter::default(),
         );
-        let digest_duration = Histogram::new(histogram::Buckets::LOCAL);
-        context.register(
+        let digest_duration = context.register(
             "digest_duration",
             "Histogram of application digest durations",
-            digest_duration.clone(),
+            Histogram::new(histogram::Buckets::LOCAL),
         );
-
         Self {
-            tip,
-            digest,
-            acks,
-            certificates,
+            tip: context.register(
+                "tip",
+                "Lowest height without a certificate",
+                Gauge::default(),
+            ),
+            digest: context.register(
+                "digest",
+                "Number of digests returned by the automaton by status",
+                status::Counter::default(),
+            ),
+            acks: context.register(
+                "acks",
+                "Number of Ack messages processed by status",
+                status::Counter::default(),
+            ),
+            certificates: context.register(
+                "certificates",
+                "Number of certificates produced",
+                Counter::default(),
+            ),
             digest_duration: histogram::Timed::new(digest_duration),
         }
     }

consensus/src/marshal/coding/marshaled.rs

@@ -255,35 +255,31 @@ where
         } = cfg;
 
         let context = Arc::new(context);
-        let build_histogram = Histogram::new(Buckets::LOCAL);
-        context.register(
+        let build_histogram = context.register(
             "build_duration",
             "Histogram of time taken for the application to build a new block, in seconds",
-            build_histogram.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
         let build_duration = Timed::new(build_histogram);
 
-        let verify_histogram = Histogram::new(Buckets::LOCAL);
-        context.register(
+        let verify_histogram = context.register(
             "verify_duration",
             "Histogram of time taken for the application to verify a block, in seconds",
-            verify_histogram.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
         let verify_duration = Timed::new(verify_histogram);
 
-        let parent_fetch_histogram = Histogram::new(Buckets::LOCAL);
-        context.register(
+        let parent_fetch_histogram = context.register(
             "parent_fetch_duration",
             "Histogram of time taken to fetch a parent block in proposal, in seconds",
-            parent_fetch_histogram.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
         let proposal_parent_fetch_duration = Timed::new(parent_fetch_histogram);
 
-        let erasure_histogram = Histogram::new(Buckets::LOCAL);
-        context.register(
+        let erasure_histogram = context.register(
             "erasure_encode_duration",
             "Histogram of time taken to erasure encode a block, in seconds",
-            erasure_histogram.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
         let erasure_encode_duration = Timed::new(erasure_histogram);
 
@@ -573,12 +569,14 @@ where
                 let parent_fetch_duration = proposal_parent_fetch_duration.start(&runtime_context);
                 let parent = select! {
                     _ = tx.closed() => {
+                        parent_fetch_duration.discard();
                         debug!(reason = "consensus dropped receiver", "skipping proposal");
                         return;
                     },
                     result = parent_request => match result {
                         Ok(parent) => parent,
                         Err(_) => {
+                            parent_fetch_duration.observe_now(&runtime_context);
                             debug!(
                                 ?parent_commitment,
                                 reason = "failed to fetch parent block",
@@ -625,12 +623,14 @@ where
                 let build_duration = build_duration.start(&runtime_context);
                 let built_block = select! {
                     _ = tx.closed() => {
+                        build_duration.discard();
                         debug!(reason = "consensus dropped receiver", "skipping proposal");
                         return;
                     },
                     result = build_request => match result {
                         Some(block) => block,
                         None => {
+                            build_duration.observe_now(&runtime_context);
                             debug!(
                                 ?parent_commitment,
                                 reason = "block building failed",

consensus/src/marshal/coding/shards/metrics.rs

@@ -43,50 +43,38 @@ pub struct ShardMetrics {
 impl ShardMetrics {
     /// Create and register metrics with the given context.
     pub fn new(context: &impl MetricsTrait) -> Self {
-        let erasure_decode_duration = Histogram::new(Buckets::LOCAL);
-        let reconstructed_blocks_cache_count = Gauge::default();
-        let reconstruction_states_count = Gauge::default();
-        let shards_received = Family::<Peer, Counter>::default();
-        let blocks_reconstructed_total = Counter::default();
-        let reconstruction_failures_total = Counter::default();
-        context.register(
+        let erasure_decode_duration = context.register(
             "erasure_decode_duration",
             "Histogram of erasure decoding duration in seconds",
-            erasure_decode_duration.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
-        context.register(
-            "reconstructed_blocks_cache_count",
-            "Number of blocks in the reconstructed blocks cache",
-            reconstructed_blocks_cache_count.clone(),
-        );
-        context.register(
-            "reconstruction_states_count",
-            "Number of active reconstruction states",
-            reconstruction_states_count.clone(),
-        );
-        context.register(
-            "shards_received",
-            "Number of shards received per peer",
-            shards_received.clone(),
-        );
-        context.register(
-            "blocks_reconstructed_total",
-            "Total number of blocks successfully reconstructed",
-            blocks_reconstructed_total.clone(),
-        );
-        context.register(
-            "reconstruction_failures_total",
-            "Total number of block reconstruction failures",
-            reconstruction_failures_total.clone(),
-        );
-
         Self {
             erasure_decode_duration: histogram::Timed::new(erasure_decode_duration),
-            reconstructed_blocks_cache_count,
-            reconstruction_states_count,
-            shards_received,
-            blocks_reconstructed_total,
-            reconstruction_failures_total,
+            reconstructed_blocks_cache_count: context.register(
+                "reconstructed_blocks_cache_count",
+                "Number of blocks in the reconstructed blocks cache",
+                Gauge::default(),
+            ),
+            reconstruction_states_count: context.register(
+                "reconstruction_states_count",
+                "Number of active reconstruction states",
+                Gauge::default(),
+            ),
+            shards_received: context.register(
+                "shards_received",
+                "Number of shards received per peer",
+                Family::<Peer, Counter>::default(),
+            ),
+            blocks_reconstructed_total: context.register(
+                "blocks_reconstructed_total",
+                "Total number of blocks successfully reconstructed",
+                Counter::default(),
+            ),
+            reconstruction_failures_total: context.register(
+                "reconstruction_failures_total",
+                "Total number of block reconstruction failures",
+                Counter::default(),
+            ),
         }
     }
 }

consensus/src/marshal/core/actor.rs

@@ -317,17 +317,15 @@ where
             .unwrap_or(Height::zero());
 
         // Create metrics
-        let finalized_height = Gauge::default();
-        context.register(
+        let finalized_height = context.register(
             "finalized_height",
             "Finalized height of application",
-            finalized_height.clone(),
+            Gauge::default(),
         );
-        let processed_height = Gauge::default();
-        context.register(
+        let processed_height = context.register(
             "processed_height",
             "Processed height of application",
-            processed_height.clone(),
+            Gauge::default(),
         );
         let _ = processed_height.try_set(last_processed_height.get());
 

consensus/src/marshal/standard/deferred.rs

@@ -190,11 +190,10 @@ where
         use prometheus_client::metrics::histogram::Histogram;
 
         let context = Arc::new(context);
-        let build_histogram = Histogram::new(Buckets::LOCAL);
-        context.register(
+        let build_histogram = context.register(
             "build_duration",
             "Histogram of time taken for the application to build a new block, in seconds",
-            build_histogram.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
         let build_duration = Timed::new(build_histogram);
 
@@ -396,12 +395,14 @@ where
                 let build_duration = build_duration.start(&runtime_context);
                 let built_block = select! {
                     _ = tx.closed() => {
+                        build_duration.discard();
                         debug!(reason = "consensus dropped receiver", "skipping proposal");
                         return;
                     },
                     result = build_request => match result {
                         Some(block) => block,
                         None => {
+                            build_duration.observe_now(&runtime_context);
                             debug!(
                                 ?parent_digest,
                                 reason = "block building failed",

consensus/src/marshal/standard/inline.rs

@@ -186,11 +186,10 @@ where
     /// the shared "last built block" cache used by [`Relay::broadcast`].
     pub fn new(context: E, application: A, marshal: Mailbox<S, Standard<B>>, epocher: ES) -> Self {
         let context = Arc::new(context);
-        let build_histogram = Histogram::new(Buckets::LOCAL);
-        context.register(
+        let build_histogram = context.register(
             "build_duration",
             "Histogram of time taken for the application to build a new block, in seconds",
-            build_histogram.clone(),
+            Histogram::new(Buckets::LOCAL),
         );
         let build_duration = Timed::new(build_histogram);
 
@@ -325,12 +324,14 @@ where
                 let build_duration = build_duration.start(&runtime_context);
                 let built_block = select! {
                     _ = tx.closed() => {
+                        build_duration.discard();
                         debug!(reason = "consensus dropped receiver", "skipping proposal");
                         return;
                     },
                     result = build_request => match result {
                         Some(block) => block,
                         None => {
+                            build_duration.observe_now(&runtime_context);
                             debug!(
                                 ?parent_digest,
                                 reason = "block building failed",