From fc14a818c820bb4249a0b83ec0b5128e946a2686 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 10:59:45 +0100 Subject: [PATCH 01/12] refactor: [#1445] new macro to create metric names --- packages/metrics/src/metric/name.rs | 59 +++++++++++++---------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/packages/metrics/src/metric/name.rs b/packages/metrics/src/metric/name.rs index c904f34d3..453d5c777 100644 --- a/packages/metrics/src/metric/name.rs +++ b/packages/metrics/src/metric/name.rs @@ -14,11 +14,7 @@ impl MetricName { /// Panics if the provided name is empty. #[must_use] pub fn new(name: &str) -> Self { - assert!( - !name.is_empty(), - "Metric name cannot be empty. It must have at least one character." - ); - + assert!(!name.is_empty(), "Metric name cannot be empty."); Self(name.to_owned()) } } @@ -50,43 +46,42 @@ impl PrometheusSerializable for MetricName { } } +#[macro_export] +macro_rules! metric_name { + ("") => { + compile_error!("Metric name cannot be empty"); + }; + ($name:literal) => { + $crate::metric::name::MetricName::new($name) + }; +} + #[cfg(test)] mod tests { mod serialization_of_metric_name_to_prometheus { - use rstest::rstest; - - use crate::metric::MetricName; use crate::prometheus::PrometheusSerializable; - #[rstest] - #[case("valid name", "valid_name", "valid_name")] - #[case("leading underscore", "_leading_underscore", "_leading_underscore")] - #[case("leading colon", ":leading_colon", ":leading_colon")] - #[case("leading lowercase", "v123", "v123")] - #[case("leading uppercase", "V123", "V123")] - fn valid_names_in_prometheus(#[case] case: &str, #[case] input: &str, #[case] output: &str) { - assert_eq!(MetricName::new(input).to_prometheus(), output, "{case} failed: {input:?}"); - } - - #[rstest] - #[case("invalid start 1", "9invalid_start", "_invalid_start")] - #[case("invalid start 2", "@test", "_test")] - #[case("invalid dash", "invalid-char", "invalid_char")] - #[case("invalid spaces", "spaces are bad", "spaces_are_bad")] - #[case("invalid special chars", "a!b@c#d$e%f^g&h*i(j)", "a_b_c_d_e_f_g_h_i_j_")] - #[case("invalid slash", "my:metric/version", "my:metric_version")] - #[case("all invalid characters", "!@#$%^&*()", "__________")] - #[case("non_ascii_characters", "ñaca©", "_aca_")] - fn names_that_need_changes_in_prometheus(#[case] case: &str, #[case] input: &str, #[case] output: &str) { - assert_eq!(MetricName::new(input).to_prometheus(), output, "{case} failed: {input:?}"); + #[test] + fn valid_names_in_prometheus() { + assert_eq!(metric_name!("valid_name").to_prometheus(), "valid_name"); + assert_eq!(metric_name!("_leading_underscore").to_prometheus(), "_leading_underscore"); + assert_eq!(metric_name!(":leading_colon").to_prometheus(), ":leading_colon"); + assert_eq!(metric_name!("v123").to_prometheus(), "v123"); // leading lowercase + assert_eq!(metric_name!("V123").to_prometheus(), "V123"); // leading lowercase } #[test] - #[should_panic(expected = "Metric name cannot be empty. It must have at least one character.")] - fn empty_name() { - let _name = MetricName::new(""); + fn names_that_need_changes_in_prometheus() { + assert_eq!(metric_name!("9invalid_start").to_prometheus(), "_invalid_start"); + assert_eq!(metric_name!("@test").to_prometheus(), "_test"); + assert_eq!(metric_name!("invalid-char").to_prometheus(), "invalid_char"); + assert_eq!(metric_name!("spaces are bad").to_prometheus(), "spaces_are_bad"); + assert_eq!(metric_name!("a!b@c#d$e%f^g&h*i(j)").to_prometheus(), "a_b_c_d_e_f_g_h_i_j_"); + assert_eq!(metric_name!("my:metric/version").to_prometheus(), "my:metric_version"); + assert_eq!(metric_name!("!@#$%^&*()").to_prometheus(), "__________"); + assert_eq!(metric_name!("ñaca©").to_prometheus(), "_aca_"); } } } From 5497970f86edbb6da3ae327d29e26eabd4c6b7bc Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 11:25:01 +0100 Subject: [PATCH 02/12] refactor: [#1445] replace metric name constructor by macro --- .../src/statistics/event/handler.rs | 6 +- .../http-tracker-core/src/statistics/mod.rs | 4 +- packages/metrics/src/metric/mod.rs | 17 ++-- packages/metrics/src/metric/name.rs | 3 + packages/metrics/src/metric_collection.rs | 79 +++++++++---------- .../src/statistics/event/handler.rs | 8 +- .../udp-tracker-core/src/statistics/mod.rs | 4 +- .../src/statistics/event/handler.rs | 24 +++--- .../udp-tracker-server/src/statistics/mod.rs | 16 ++-- 9 files changed, 79 insertions(+), 82 deletions(-) diff --git a/packages/http-tracker-core/src/statistics/event/handler.rs b/packages/http-tracker-core/src/statistics/event/handler.rs index 0baec1cd9..6e37b0209 100644 --- a/packages/http-tracker-core/src/statistics/event/handler.rs +++ b/packages/http-tracker-core/src/statistics/event/handler.rs @@ -1,7 +1,7 @@ use std::net::IpAddr; use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; -use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_name; use torrust_tracker_primitives::DurationSinceUnixEpoch; use crate::event::Event; @@ -32,7 +32,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("request_kind"), LabelValue::new("announce")); stats_repository - .increase_counter(&MetricName::new(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) + .increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) .await; } Event::TcpScrape { connection } => { @@ -53,7 +53,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("request_kind"), LabelValue::new("scrape")); stats_repository - .increase_counter(&MetricName::new(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) + .increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) .await; } } diff --git a/packages/http-tracker-core/src/statistics/mod.rs b/packages/http-tracker-core/src/statistics/mod.rs index 026c435af..a5d6d37a5 100644 --- a/packages/http-tracker-core/src/statistics/mod.rs +++ b/packages/http-tracker-core/src/statistics/mod.rs @@ -7,7 +7,7 @@ pub mod setup; use metrics::Metrics; use torrust_tracker_metrics::metric::description::MetricDescription; -use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_name; use torrust_tracker_metrics::unit::Unit; const HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL: &str = "http_tracker_core_requests_received_total"; @@ -17,7 +17,7 @@ pub fn describe_metrics() -> Metrics { let mut metrics = Metrics::default(); metrics.metric_collection.describe_counter( - &MetricName::new(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), + &metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of HTTP requests received")), ); diff --git a/packages/metrics/src/metric/mod.rs b/packages/metrics/src/metric/mod.rs index edea035bb..95e35b520 100644 --- a/packages/metrics/src/metric/mod.rs +++ b/packages/metrics/src/metric/mod.rs @@ -87,11 +87,12 @@ mod tests { use super::super::*; use crate::gauge::Gauge; use crate::label::{LabelName, LabelValue}; + use crate::metric_name; use crate::sample::Sample; #[test] fn it_should_be_empty_when_it_does_not_have_any_sample() { - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let samples = SampleCollection::::default(); @@ -103,7 +104,7 @@ mod tests { fn counter_metric_with_one_sample() -> Metric { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let label_set: LabelSet = [(LabelName::new("server_binding_protocol"), LabelValue::new("http"))].into(); @@ -119,7 +120,7 @@ mod tests { #[test] fn it_should_return_zero_number_of_samples_for_an_empty_metric() { - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let samples = SampleCollection::::default(); @@ -133,11 +134,12 @@ mod tests { use super::super::*; use crate::counter::Counter; use crate::label::{LabelName, LabelValue}; + use crate::metric_name; use crate::sample::Sample; #[test] fn it_should_be_created_from_its_name_and_a_collection_of_samples() { - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let samples = SampleCollection::::default(); @@ -148,7 +150,7 @@ mod tests { fn it_should_allow_incrementing_a_sample() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let label_set: LabelSet = [(LabelName::new("server_binding_protocol"), LabelValue::new("http"))].into(); @@ -166,11 +168,12 @@ mod tests { use super::super::*; use crate::gauge::Gauge; use crate::label::{LabelName, LabelValue}; + use crate::metric_name; use crate::sample::Sample; #[test] fn it_should_be_created_from_its_name_and_a_collection_of_samples() { - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let samples = SampleCollection::::default(); @@ -181,7 +184,7 @@ mod tests { fn it_should_allow_setting_a_sample() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let name = MetricName::new("test_metric"); + let name = metric_name!("test_metric"); let label_set: LabelSet = [(LabelName::new("server_binding_protocol"), LabelValue::new("http"))].into(); diff --git a/packages/metrics/src/metric/name.rs b/packages/metrics/src/metric/name.rs index 453d5c777..41f5e7058 100644 --- a/packages/metrics/src/metric/name.rs +++ b/packages/metrics/src/metric/name.rs @@ -54,6 +54,9 @@ macro_rules! metric_name { ($name:literal) => { $crate::metric::name::MetricName::new($name) }; + ($name:ident) => { + $crate::metric::name::MetricName::new($name) + }; } #[cfg(test)] diff --git a/packages/metrics/src/metric_collection.rs b/packages/metrics/src/metric_collection.rs index d0ed96554..eb75e5c77 100644 --- a/packages/metrics/src/metric_collection.rs +++ b/packages/metrics/src/metric_collection.rs @@ -315,6 +315,7 @@ mod tests { use super::*; use crate::label::{LabelName, LabelValue}; + use crate::metric_name; use crate::sample::Sample; use crate::tests::{format_prometheus_output, sort_lines}; @@ -355,11 +356,11 @@ mod tests { MetricCollection::new( MetricKindCollection::new(vec![Metric::new( - MetricName::new("http_tracker_core_announce_requests_received_total"), + metric_name!("http_tracker_core_announce_requests_received_total"), SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set_1.clone())]), )]), MetricKindCollection::new(vec![Metric::new( - MetricName::new("udp_tracker_server_performance_avg_announce_processing_time_ns"), + metric_name!("udp_tracker_server_performance_avg_announce_processing_time_ns"), SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set_1.clone())]), )]), ) @@ -434,15 +435,9 @@ mod tests { #[test] #[should_panic(expected = "Metric names must be unique across counters and gauges")] fn it_should_not_allow_duplicate_names_across_types() { - let counter = MetricKindCollection::new(vec![Metric::new( - MetricName::new("test_metric"), - SampleCollection::new(vec![]), - )]); + let counter = MetricKindCollection::new(vec![Metric::new(metric_name!("test_metric"), SampleCollection::new(vec![]))]); - let gauge = MetricKindCollection::new(vec![Metric::new( - MetricName::new("test_metric"), - SampleCollection::new(vec![]), - )]); + let gauge = MetricKindCollection::new(vec![Metric::new(metric_name!("test_metric"), SampleCollection::new(vec![]))]); let _unused = MetricCollection::new(counter, gauge); } @@ -455,10 +450,10 @@ mod tests { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); // First create a counter - collection.increase_counter(&MetricName::new("test_metric"), &label_set, time); + collection.increase_counter(&metric_name!("test_metric"), &label_set, time); // Then try to create a gauge with the same name - this should panic - collection.set_gauge(&MetricName::new("test_metric"), &label_set, 1.0, time); + collection.set_gauge(&metric_name!("test_metric"), &label_set, 1.0, time); } #[test] @@ -469,15 +464,15 @@ mod tests { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); // First set the gauge - collection.set_gauge(&MetricName::new("test_metric"), &label_set, 1.0, time); + collection.set_gauge(&metric_name!("test_metric"), &label_set, 1.0, time); // Then try to create a counter with the same name - this should panic - collection.increase_counter(&MetricName::new("test_metric"), &label_set, time); + collection.increase_counter(&metric_name!("test_metric"), &label_set, time); } #[test] fn it_should_allow_serializing_to_json() { - // todo: this test does work with metric with multiple samples becuase + // todo: this test does work with metric with multiple samples because // samples are not serialized in the same order as they are created. let (metric_collection, expected_json, _expected_prometheus) = MetricCollectionFixture::default().deconstruct(); @@ -528,7 +523,7 @@ mod tests { let metric_collection = MetricCollection::new( MetricKindCollection::new(vec![Metric::new( - MetricName::new("http_tracker_core_announce_requests_received_total"), + metric_name!("http_tracker_core_announce_requests_received_total"), SampleCollection::new(vec![ Sample::new(Counter::new(1), time, label_set_1.clone()), Sample::new(Counter::new(2), time, label_set_2.clone()), @@ -557,8 +552,8 @@ mod tests { let mut counters = MetricKindCollection::new(vec![]); let mut gauges = MetricKindCollection::new(vec![]); - counters.ensure_metric_exists(&MetricName::new("test_counter")); - gauges.ensure_metric_exists(&MetricName::new("test_gauge")); + counters.ensure_metric_exists(&metric_name!("test_counter")); + gauges.ensure_metric_exists(&metric_name!("test_gauge")); let metric_collection = MetricCollection::new(counters, gauges); @@ -582,17 +577,17 @@ mod tests { let mut metric_collection = MetricCollection::new( MetricKindCollection::new(vec![Metric::new( - MetricName::new("test_counter"), + metric_name!("test_counter"), SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), )]), MetricKindCollection::new(vec![]), ); - metric_collection.increase_counter(&MetricName::new("test_counter"), &label_set, time); - metric_collection.increase_counter(&MetricName::new("test_counter"), &label_set, time); + metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); + metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); assert_eq!( - metric_collection.get_counter_value(&MetricName::new("test_counter"), &label_set), + metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), Counter::new(2) ); } @@ -605,11 +600,11 @@ mod tests { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); - metric_collection.increase_counter(&MetricName::new("test_counter"), &label_set, time); - metric_collection.increase_counter(&MetricName::new("test_counter"), &label_set, time); + metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); + metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); assert_eq!( - metric_collection.get_counter_value(&MetricName::new("test_counter"), &label_set), + metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), Counter::new(2) ); } @@ -621,10 +616,10 @@ mod tests { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); - metric_collection.ensure_counter_exists(&MetricName::new("test_counter")); + metric_collection.ensure_counter_exists(&metric_name!("test_counter")); assert_eq!( - metric_collection.get_counter_value(&MetricName::new("test_counter"), &label_set), + metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), Counter::default() ); } @@ -636,10 +631,10 @@ mod tests { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); - metric_collection.describe_counter(&MetricName::new("test_counter"), None, None); + metric_collection.describe_counter(&metric_name!("test_counter"), None, None); assert_eq!( - metric_collection.get_counter_value(&MetricName::new("test_counter"), &label_set), + metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), Counter::default() ); } @@ -652,11 +647,11 @@ mod tests { let _unused = MetricKindCollection::new(vec![ Metric::new( - MetricName::new("test_counter"), + metric_name!("test_counter"), SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), ), Metric::new( - MetricName::new("test_counter"), + metric_name!("test_counter"), SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), ), ]); @@ -679,15 +674,15 @@ mod tests { let mut metric_collection = MetricCollection::new( MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![Metric::new( - MetricName::new("test_gauge"), + metric_name!("test_gauge"), SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), )]), ); - metric_collection.set_gauge(&MetricName::new("test_gauge"), &label_set, 1.0, time); + metric_collection.set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time); assert_eq!( - metric_collection.get_gauge_value(&MetricName::new("test_gauge"), &label_set), + metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), Gauge::new(1.0) ); } @@ -700,10 +695,10 @@ mod tests { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); - metric_collection.set_gauge(&MetricName::new("test_gauge"), &label_set, 1.0, time); + metric_collection.set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time); assert_eq!( - metric_collection.get_gauge_value(&MetricName::new("test_gauge"), &label_set), + metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), Gauge::new(1.0) ); } @@ -715,10 +710,10 @@ mod tests { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); - metric_collection.ensure_gauge_exists(&MetricName::new("test_gauge")); + metric_collection.ensure_gauge_exists(&metric_name!("test_gauge")); assert_eq!( - metric_collection.get_gauge_value(&MetricName::new("test_gauge"), &label_set), + metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), Gauge::default() ); } @@ -730,10 +725,10 @@ mod tests { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); - metric_collection.describe_gauge(&MetricName::new("test_gauge"), None, None); + metric_collection.describe_gauge(&metric_name!("test_gauge"), None, None); assert_eq!( - metric_collection.get_gauge_value(&MetricName::new("test_gauge"), &label_set), + metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), Gauge::default() ); } @@ -746,11 +741,11 @@ mod tests { let _unused = MetricKindCollection::new(vec![ Metric::new( - MetricName::new("test_gauge"), + metric_name!("test_gauge"), SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), ), Metric::new( - MetricName::new("test_gauge"), + metric_name!("test_gauge"), SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), ), ]); diff --git a/packages/udp-tracker-core/src/statistics/event/handler.rs b/packages/udp-tracker-core/src/statistics/event/handler.rs index 59c382755..dcb512783 100644 --- a/packages/udp-tracker-core/src/statistics/event/handler.rs +++ b/packages/udp-tracker-core/src/statistics/event/handler.rs @@ -1,5 +1,5 @@ use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; -use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_name; use torrust_tracker_primitives::DurationSinceUnixEpoch; use crate::event::Event; @@ -29,7 +29,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("request_kind"), LabelValue::new("connect")); stats_repository - .increase_counter(&MetricName::new(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) + .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) .await; } Event::UdpAnnounce { context } => { @@ -50,7 +50,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("request_kind"), LabelValue::new("announce")); stats_repository - .increase_counter(&MetricName::new(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) + .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) .await; } Event::UdpScrape { context } => { @@ -71,7 +71,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("request_kind"), LabelValue::new("scrape")); stats_repository - .increase_counter(&MetricName::new(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) + .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) .await; } } diff --git a/packages/udp-tracker-core/src/statistics/mod.rs b/packages/udp-tracker-core/src/statistics/mod.rs index bc4d8d836..40a30f51b 100644 --- a/packages/udp-tracker-core/src/statistics/mod.rs +++ b/packages/udp-tracker-core/src/statistics/mod.rs @@ -7,7 +7,7 @@ pub mod setup; use metrics::Metrics; use torrust_tracker_metrics::metric::description::MetricDescription; -use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_name; use torrust_tracker_metrics::unit::Unit; const UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL: &str = "udp_tracker_core_requests_received_total"; @@ -17,7 +17,7 @@ pub fn describe_metrics() -> Metrics { let mut metrics = Metrics::default(); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), + &metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of UDP requests received")), ); diff --git a/packages/udp-tracker-server/src/statistics/event/handler.rs b/packages/udp-tracker-server/src/statistics/event/handler.rs index 4c10576c0..721d415ea 100644 --- a/packages/udp-tracker-server/src/statistics/event/handler.rs +++ b/packages/udp-tracker-server/src/statistics/event/handler.rs @@ -1,5 +1,5 @@ use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; -use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_name; use torrust_tracker_primitives::DurationSinceUnixEpoch; use crate::event::{Event, UdpRequestKind, UdpResponseKind}; @@ -25,7 +25,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics stats_repository .increase_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL), &LabelSet::from(context), now, ) @@ -38,7 +38,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics stats_repository .increase_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL), &LabelSet::from(context), now, ) @@ -58,7 +58,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics stats_repository .increase_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL), &LabelSet::from(context), now, ) @@ -100,7 +100,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("kind"), LabelValue::new(&kind.to_string())); stats_repository - .increase_counter(&MetricName::new(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), &label_set, now) + .increase_counter(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), &label_set, now) .await; } Event::UdpResponseSent { @@ -132,7 +132,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura stats_repository .set_gauge( - &MetricName::new(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), + &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), &label_set, new_avg, now, @@ -153,7 +153,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura stats_repository .set_gauge( - &MetricName::new(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), + &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), &label_set, new_avg, now, @@ -174,7 +174,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura stats_repository .set_gauge( - &MetricName::new(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), + &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), &label_set, new_avg, now, @@ -197,7 +197,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(LabelName::new("result"), result_label_value); stats_repository - .increase_counter(&MetricName::new(UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL), &label_set, now) + .increase_counter(&metric_name!(UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL), &label_set, now) .await; } Event::UdpError { context } => { @@ -213,11 +213,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics stats_repository - .increase_counter( - &MetricName::new(UDP_TRACKER_SERVER_ERRORS_TOTAL), - &LabelSet::from(context), - now, - ) + .increase_counter(&metric_name!(UDP_TRACKER_SERVER_ERRORS_TOTAL), &LabelSet::from(context), now) .await; } } diff --git a/packages/udp-tracker-server/src/statistics/mod.rs b/packages/udp-tracker-server/src/statistics/mod.rs index 523cd4bac..4eea13224 100644 --- a/packages/udp-tracker-server/src/statistics/mod.rs +++ b/packages/udp-tracker-server/src/statistics/mod.rs @@ -7,7 +7,7 @@ pub mod setup; use metrics::Metrics; use torrust_tracker_metrics::metric::description::MetricDescription; -use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_name; use torrust_tracker_metrics::unit::Unit; const UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL: &str = "udp_tracker_server_requests_aborted_total"; @@ -23,43 +23,43 @@ pub fn describe_metrics() -> Metrics { let mut metrics = Metrics::default(); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of UDP requests aborted")), ); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of UDP requests banned")), ); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of UDP requests received")), ); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of UDP requests accepted")), ); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of UDP responses sent")), ); metrics.metric_collection.describe_counter( - &MetricName::new(UDP_TRACKER_SERVER_ERRORS_TOTAL), + &metric_name!(UDP_TRACKER_SERVER_ERRORS_TOTAL), Some(Unit::Count), Some(MetricDescription::new("Total number of errors processing UDP requests")), ); metrics.metric_collection.describe_gauge( - &MetricName::new(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), + &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), Some(Unit::Nanoseconds), Some(MetricDescription::new( "Average time to process a UDP connect request in nanoseconds", From 7a24f855c1ac129bba890a72a9ccf9355bba2b85 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 11:38:53 +0100 Subject: [PATCH 03/12] refactor: [#1445] new macro to create metric label names --- packages/metrics/src/label/name.rs | 27 ++++++++++++++++++--------- packages/metrics/src/metric/name.rs | 7 +++++++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/packages/metrics/src/label/name.rs b/packages/metrics/src/label/name.rs index 22e75572f..194aeb2b3 100644 --- a/packages/metrics/src/label/name.rs +++ b/packages/metrics/src/label/name.rs @@ -14,11 +14,7 @@ impl LabelName { /// Panics if the provided name is empty. #[must_use] pub fn new(name: &str) -> Self { - assert!( - !name.is_empty(), - "Label name cannot be empty. It must have at least one character." - ); - + assert!(!name.is_empty(), "Label name cannot be empty."); Self(name.to_owned()) } } @@ -69,6 +65,19 @@ impl PrometheusSerializable for LabelName { } } } + +#[macro_export] +macro_rules! label_name { + ("") => { + compile_error!("Label name cannot be empty"); + }; + ($name:literal) => { + $crate::label::name::LabelName::new($name) + }; + ($name:ident) => { + $crate::label::name::LabelName::new($name) + }; +} #[cfg(test)] mod tests { mod serialization_of_label_name_to_prometheus { @@ -83,7 +92,7 @@ mod tests { #[case("3 leading lowercase", "v123", "v123")] #[case("4 leading uppercase", "V123", "V123")] fn valid_names_in_prometheus(#[case] case: &str, #[case] input: &str, #[case] output: &str) { - assert_eq!(LabelName::new(input).to_prometheus(), output, "{case} failed: {input:?}"); + assert_eq!(label_name!(input).to_prometheus(), output, "{case} failed: {input:?}"); } #[rstest] @@ -96,7 +105,7 @@ mod tests { #[case("7 all invalid characters", "!@#$%^&*()", "__________")] #[case("8 non_ascii_characters", "ñaca©", "_aca_")] fn names_that_need_changes_in_prometheus(#[case] case: &str, #[case] input: &str, #[case] output: &str) { - assert_eq!(LabelName::new(input).to_prometheus(), output, "{case} failed: {input:?}"); + assert_eq!(label_name!(input).to_prometheus(), output, "{case} failed: {input:?}"); } #[rstest] @@ -105,11 +114,11 @@ mod tests { #[case("3 processed to double underscore", "^^name", "___name")] #[case("4 processed to double underscore after first char", "0__name", "___name")] fn names_starting_with_double_underscore(#[case] case: &str, #[case] input: &str, #[case] output: &str) { - assert_eq!(LabelName::new(input).to_prometheus(), output, "{case} failed: {input:?}"); + assert_eq!(label_name!(input).to_prometheus(), output, "{case} failed: {input:?}"); } #[test] - #[should_panic(expected = "Label name cannot be empty. It must have at least one character.")] + #[should_panic(expected = "Label name cannot be empty.")] fn empty_name() { let _name = LabelName::new(""); } diff --git a/packages/metrics/src/metric/name.rs b/packages/metrics/src/metric/name.rs index 41f5e7058..09c8c9e6d 100644 --- a/packages/metrics/src/metric/name.rs +++ b/packages/metrics/src/metric/name.rs @@ -64,6 +64,7 @@ mod tests { mod serialization_of_metric_name_to_prometheus { + use crate::metric::name::MetricName; use crate::prometheus::PrometheusSerializable; #[test] @@ -86,5 +87,11 @@ mod tests { assert_eq!(metric_name!("!@#$%^&*()").to_prometheus(), "__________"); assert_eq!(metric_name!("ñaca©").to_prometheus(), "_aca_"); } + + #[test] + #[should_panic(expected = "Metric name cannot be empty.")] + fn empty_name() { + let _name = MetricName::new(""); + } } } From 4d68267fff03505c9de1aca279a45a07b0ba32cc Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 11:51:21 +0100 Subject: [PATCH 04/12] refactor: [#1445] replace metric label name constructor by macro --- packages/http-tracker-core/src/event/mod.rs | 9 ++-- .../src/statistics/event/handler.rs | 8 ++-- packages/metrics/src/label/mod.rs | 4 +- packages/metrics/src/label/pair.rs | 6 +-- packages/metrics/src/label/set.rs | 33 ++++++------- packages/metrics/src/metric/mod.rs | 18 ++++---- packages/metrics/src/metric_collection.rs | 46 +++++++++---------- packages/udp-tracker-core/src/event/mod.rs | 9 ++-- .../src/statistics/event/handler.rs | 10 ++-- packages/udp-tracker-server/src/event/mod.rs | 9 ++-- .../src/statistics/event/handler.rs | 16 +++---- 11 files changed, 86 insertions(+), 82 deletions(-) diff --git a/packages/http-tracker-core/src/event/mod.rs b/packages/http-tracker-core/src/event/mod.rs index d235c179f..ae997156a 100644 --- a/packages/http-tracker-core/src/event/mod.rs +++ b/packages/http-tracker-core/src/event/mod.rs @@ -1,6 +1,7 @@ use std::net::{IpAddr, SocketAddr}; -use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; +use torrust_tracker_metrics::label::{LabelSet, LabelValue}; +use torrust_tracker_metrics::label_name; use torrust_tracker_primitives::service_binding::ServiceBinding; pub mod sender; @@ -65,15 +66,15 @@ impl From for LabelSet { fn from(connection_context: ConnectionContext) -> Self { LabelSet::from([ ( - LabelName::new("server_binding_protocol"), + label_name!("server_binding_protocol"), LabelValue::new(&connection_context.server.service_binding.protocol().to_string()), ), ( - LabelName::new("server_binding_ip"), + label_name!("server_binding_ip"), LabelValue::new(&connection_context.server.service_binding.bind_address().ip().to_string()), ), ( - LabelName::new("server_binding_port"), + label_name!("server_binding_port"), LabelValue::new(&connection_context.server.service_binding.bind_address().port().to_string()), ), ]) diff --git a/packages/http-tracker-core/src/statistics/event/handler.rs b/packages/http-tracker-core/src/statistics/event/handler.rs index 6e37b0209..cea224d04 100644 --- a/packages/http-tracker-core/src/statistics/event/handler.rs +++ b/packages/http-tracker-core/src/statistics/event/handler.rs @@ -1,7 +1,7 @@ use std::net::IpAddr; -use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; -use torrust_tracker_metrics::metric_name; +use torrust_tracker_metrics::label::{LabelSet, LabelValue}; +use torrust_tracker_metrics::{label_name, metric_name}; use torrust_tracker_primitives::DurationSinceUnixEpoch; use crate::event::Event; @@ -29,7 +29,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(connection); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new("announce")); + label_set.upsert(label_name!("request_kind"), LabelValue::new("announce")); stats_repository .increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) @@ -50,7 +50,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(connection); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new("scrape")); + label_set.upsert(label_name!("request_kind"), LabelValue::new("scrape")); stats_repository .increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) diff --git a/packages/metrics/src/label/mod.rs b/packages/metrics/src/label/mod.rs index b5fd3b745..880fdbbb1 100644 --- a/packages/metrics/src/label/mod.rs +++ b/packages/metrics/src/label/mod.rs @@ -1,7 +1,7 @@ -mod name; +pub mod name; mod pair; mod set; -mod value; +pub mod value; pub type LabelName = name::LabelName; pub type LabelValue = value::LabelValue; diff --git a/packages/metrics/src/label/pair.rs b/packages/metrics/src/label/pair.rs index c89c726bd..858902451 100644 --- a/packages/metrics/src/label/pair.rs +++ b/packages/metrics/src/label/pair.rs @@ -13,16 +13,16 @@ impl PrometheusSerializabl #[cfg(test)] mod tests { mod serialization_of_label_pair_to_prometheus { - use super::super::LabelName; use crate::label::LabelValue; + use crate::label_name; use crate::prometheus::PrometheusSerializable; #[test] fn test_label_pair_serialization_to_prometheus() { - let label_pair = (LabelName::new("label_name"), LabelValue::new("value")); + let label_pair = (label_name!("label_name"), LabelValue::new("value")); assert_eq!(label_pair.to_prometheus(), r#"label_name="value""#); - let label_pair = (&LabelName::new("label_name"), &LabelValue::new("value")); + let label_pair = (&label_name!("label_name"), &LabelValue::new("value")); assert_eq!(label_pair.to_prometheus(), r#"label_name="value""#); } } diff --git a/packages/metrics/src/label/set.rs b/packages/metrics/src/label/set.rs index f46b01095..2b6334fc7 100644 --- a/packages/metrics/src/label/set.rs +++ b/packages/metrics/src/label/set.rs @@ -180,6 +180,7 @@ mod tests { use super::{LabelName, LabelValue}; use crate::label::LabelSet; + use crate::label_name; use crate::prometheus::PrometheusSerializable; fn sample_vec_of_label_pairs() -> Vec<(LabelName, LabelValue)> { @@ -188,9 +189,9 @@ mod tests { fn sample_array_of_label_pairs() -> [(LabelName, LabelValue); 3] { [ - (LabelName::new("server_service_binding_protocol"), LabelValue::new("http")), - (LabelName::new("server_service_binding_ip"), LabelValue::new("0.0.0.0")), - (LabelName::new("server_service_binding_port"), LabelValue::new("7070")), + (label_name!("server_service_binding_protocol"), LabelValue::new("http")), + (label_name!("server_service_binding_ip"), LabelValue::new("0.0.0.0")), + (label_name!("server_service_binding_port"), LabelValue::new("7070")), ] } @@ -232,12 +233,12 @@ mod tests { #[test] fn it_should_allow_instantiation_from_a_label_pair() { - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); assert_eq!( label_set, LabelSet { - items: BTreeMap::from([(LabelName::new("label_name"), LabelValue::new("value"))]) + items: BTreeMap::from([(label_name!("label_name"), LabelValue::new("value"))]) } ); } @@ -246,10 +247,10 @@ mod tests { fn it_should_allow_inserting_a_new_label_pair() { let mut label_set = LabelSet::default(); - label_set.upsert(LabelName::new("label_name"), LabelValue::new("value")); + label_set.upsert(label_name!("label_name"), LabelValue::new("value")); assert_eq!( - label_set.items.get(&LabelName::new("label_name")).unwrap(), + label_set.items.get(&label_name!("label_name")).unwrap(), &LabelValue::new("value") ); } @@ -258,18 +259,18 @@ mod tests { fn it_should_allow_updating_a_label_value() { let mut label_set = LabelSet::default(); - label_set.upsert(LabelName::new("label_name"), LabelValue::new("old value")); - label_set.upsert(LabelName::new("label_name"), LabelValue::new("new value")); + label_set.upsert(label_name!("label_name"), LabelValue::new("old value")); + label_set.upsert(label_name!("label_name"), LabelValue::new("new value")); assert_eq!( - label_set.items.get(&LabelName::new("label_name")).unwrap(), + label_set.items.get(&label_name!("label_name")).unwrap(), &LabelValue::new("new value") ); } #[test] fn it_should_allow_serializing_to_json_as_an_array_of_label_objects() { - let label_set = LabelSet::from((LabelName::new("label_name"), LabelValue::new("label value"))); + let label_set = LabelSet::from((label_name!("label_name"), LabelValue::new("label value"))); let json = serde_json::to_string(&label_set).unwrap(); @@ -307,13 +308,13 @@ mod tests { assert_eq!( label_set, - LabelSet::from((LabelName::new("label_name"), LabelValue::new("label value"))) + LabelSet::from((label_name!("label_name"), LabelValue::new("label value"))) ); } #[test] fn it_should_allow_serializing_to_prometheus_format() { - let label_set = LabelSet::from((LabelName::new("label_name"), LabelValue::new("label value"))); + let label_set = LabelSet::from((label_name!("label_name"), LabelValue::new("label value"))); assert_eq!(label_set.to_prometheus(), r#"{label_name="label value"}"#); } @@ -321,8 +322,8 @@ mod tests { #[test] fn it_should_alphabetically_order_labels_in_prometheus_format() { let label_set = LabelSet::from([ - (LabelName::new("b_label_name"), LabelValue::new("b label value")), - (LabelName::new("a_label_name"), LabelValue::new("a label value")), + (label_name!("b_label_name"), LabelValue::new("b label value")), + (label_name!("a_label_name"), LabelValue::new("a label value")), ]); assert_eq!( @@ -333,7 +334,7 @@ mod tests { #[test] fn it_should_allow_displaying() { - let label_set = LabelSet::from((LabelName::new("label_name"), LabelValue::new("label value"))); + let label_set = LabelSet::from((label_name!("label_name"), LabelValue::new("label value"))); assert_eq!(label_set.to_string(), r#"{label_name="label value"}"#); } diff --git a/packages/metrics/src/metric/mod.rs b/packages/metrics/src/metric/mod.rs index 95e35b520..777981fd8 100644 --- a/packages/metrics/src/metric/mod.rs +++ b/packages/metrics/src/metric/mod.rs @@ -86,9 +86,9 @@ mod tests { mod for_generic_metrics { use super::super::*; use crate::gauge::Gauge; - use crate::label::{LabelName, LabelValue}; - use crate::metric_name; + use crate::label::LabelValue; use crate::sample::Sample; + use crate::{label_name, metric_name}; #[test] fn it_should_be_empty_when_it_does_not_have_any_sample() { @@ -106,7 +106,7 @@ mod tests { let name = metric_name!("test_metric"); - let label_set: LabelSet = [(LabelName::new("server_binding_protocol"), LabelValue::new("http"))].into(); + let label_set: LabelSet = [(label_name!("server_binding_protocol"), LabelValue::new("http"))].into(); let samples = SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set.clone())]); @@ -133,9 +133,9 @@ mod tests { mod for_counter_metrics { use super::super::*; use crate::counter::Counter; - use crate::label::{LabelName, LabelValue}; - use crate::metric_name; + use crate::label::LabelValue; use crate::sample::Sample; + use crate::{label_name, metric_name}; #[test] fn it_should_be_created_from_its_name_and_a_collection_of_samples() { @@ -152,7 +152,7 @@ mod tests { let name = metric_name!("test_metric"); - let label_set: LabelSet = [(LabelName::new("server_binding_protocol"), LabelValue::new("http"))].into(); + let label_set: LabelSet = [(label_name!("server_binding_protocol"), LabelValue::new("http"))].into(); let samples = SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set.clone())]); @@ -167,9 +167,9 @@ mod tests { use super::super::*; use crate::gauge::Gauge; - use crate::label::{LabelName, LabelValue}; - use crate::metric_name; + use crate::label::LabelValue; use crate::sample::Sample; + use crate::{label_name, metric_name}; #[test] fn it_should_be_created_from_its_name_and_a_collection_of_samples() { @@ -186,7 +186,7 @@ mod tests { let name = metric_name!("test_metric"); - let label_set: LabelSet = [(LabelName::new("server_binding_protocol"), LabelValue::new("http"))].into(); + let label_set: LabelSet = [(label_name!("server_binding_protocol"), LabelValue::new("http"))].into(); let samples = SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set.clone())]); diff --git a/packages/metrics/src/metric_collection.rs b/packages/metrics/src/metric_collection.rs index eb75e5c77..5b3f92a19 100644 --- a/packages/metrics/src/metric_collection.rs +++ b/packages/metrics/src/metric_collection.rs @@ -314,10 +314,10 @@ mod tests { use pretty_assertions::assert_eq; use super::*; - use crate::label::{LabelName, LabelValue}; - use crate::metric_name; + use crate::label::LabelValue; use crate::sample::Sample; use crate::tests::{format_prometheus_output, sort_lines}; + use crate::{label_name, metric_name}; /// Fixture for testing serialization and deserialization of `MetricCollection`. /// @@ -348,9 +348,9 @@ mod tests { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); let label_set_1: LabelSet = [ - (LabelName::new("server_binding_protocol"), LabelValue::new("http")), - (LabelName::new("server_binding_ip"), LabelValue::new("0.0.0.0")), - (LabelName::new("server_binding_port"), LabelValue::new("7070")), + (label_name!("server_binding_protocol"), LabelValue::new("http")), + (label_name!("server_binding_ip"), LabelValue::new("0.0.0.0")), + (label_name!("server_binding_port"), LabelValue::new("7070")), ] .into(); @@ -508,16 +508,16 @@ mod tests { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); let label_set_1: LabelSet = [ - (LabelName::new("server_binding_protocol"), LabelValue::new("http")), - (LabelName::new("server_binding_ip"), LabelValue::new("0.0.0.0")), - (LabelName::new("server_binding_port"), LabelValue::new("7070")), + (label_name!("server_binding_protocol"), LabelValue::new("http")), + (label_name!("server_binding_ip"), LabelValue::new("0.0.0.0")), + (label_name!("server_binding_port"), LabelValue::new("7070")), ] .into(); let label_set_2: LabelSet = [ - (LabelName::new("server_binding_protocol"), LabelValue::new("http")), - (LabelName::new("server_binding_ip"), LabelValue::new("0.0.0.0")), - (LabelName::new("server_binding_port"), LabelValue::new("7171")), + (label_name!("server_binding_protocol"), LabelValue::new("http")), + (label_name!("server_binding_ip"), LabelValue::new("0.0.0.0")), + (label_name!("server_binding_port"), LabelValue::new("7171")), ] .into(); @@ -567,13 +567,13 @@ mod tests { use pretty_assertions::assert_eq; use super::*; - use crate::label::{LabelName, LabelValue}; + use crate::label::LabelValue; use crate::sample::Sample; #[test] fn it_should_increase_a_preexistent_counter() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new( MetricKindCollection::new(vec![Metric::new( @@ -595,7 +595,7 @@ mod tests { #[test] fn it_should_automatically_create_a_counter_when_increasing_if_it_does_not_exist() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); @@ -611,7 +611,7 @@ mod tests { #[test] fn it_should_allow_making_sure_a_counter_exists_without_increasing_it() { - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); @@ -626,7 +626,7 @@ mod tests { #[test] fn it_should_allow_describing_a_counter_before_using_it() { - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); @@ -643,7 +643,7 @@ mod tests { #[should_panic(expected = "Duplicate MetricName found in MetricKindCollection")] fn it_should_not_allow_duplicate_metric_names_when_instantiating() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let _unused = MetricKindCollection::new(vec![ Metric::new( @@ -663,13 +663,13 @@ mod tests { use pretty_assertions::assert_eq; use super::*; - use crate::label::{LabelName, LabelValue}; + use crate::label::LabelValue; use crate::sample::Sample; #[test] fn it_should_set_a_preexistent_gauge() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new( MetricKindCollection::new(vec![]), @@ -690,7 +690,7 @@ mod tests { #[test] fn it_should_automatically_create_a_gauge_when_setting_if_it_does_not_exist() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); @@ -705,7 +705,7 @@ mod tests { #[test] fn it_should_allow_making_sure_a_gauge_exists_without_increasing_it() { - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); @@ -720,7 +720,7 @@ mod tests { #[test] fn it_should_allow_describing_a_gauge_before_using_it() { - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); @@ -737,7 +737,7 @@ mod tests { #[should_panic(expected = "Duplicate MetricName found in MetricKindCollection")] fn it_should_not_allow_duplicate_metric_names_when_instantiating() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); - let label_set: LabelSet = (LabelName::new("label_name"), LabelValue::new("value")).into(); + let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let _unused = MetricKindCollection::new(vec![ Metric::new( diff --git a/packages/udp-tracker-core/src/event/mod.rs b/packages/udp-tracker-core/src/event/mod.rs index 6cb43e5a1..ddcba7792 100644 --- a/packages/udp-tracker-core/src/event/mod.rs +++ b/packages/udp-tracker-core/src/event/mod.rs @@ -1,6 +1,7 @@ use std::net::SocketAddr; -use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; +use torrust_tracker_metrics::label::{LabelSet, LabelValue}; +use torrust_tracker_metrics::label_name; use torrust_tracker_primitives::service_binding::ServiceBinding; pub mod sender; @@ -43,15 +44,15 @@ impl From for LabelSet { fn from(connection_context: ConnectionContext) -> Self { LabelSet::from([ ( - LabelName::new("server_binding_protocol"), + label_name!("server_binding_protocol"), LabelValue::new(&connection_context.server_service_binding.protocol().to_string()), ), ( - LabelName::new("server_binding_ip"), + label_name!("server_binding_ip"), LabelValue::new(&connection_context.server_service_binding.bind_address().ip().to_string()), ), ( - LabelName::new("server_binding_port"), + label_name!("server_binding_port"), LabelValue::new(&connection_context.server_service_binding.bind_address().port().to_string()), ), ]) diff --git a/packages/udp-tracker-core/src/statistics/event/handler.rs b/packages/udp-tracker-core/src/statistics/event/handler.rs index dcb512783..13a4840d5 100644 --- a/packages/udp-tracker-core/src/statistics/event/handler.rs +++ b/packages/udp-tracker-core/src/statistics/event/handler.rs @@ -1,5 +1,5 @@ -use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; -use torrust_tracker_metrics::metric_name; +use torrust_tracker_metrics::label::{LabelSet, LabelValue}; +use torrust_tracker_metrics::{label_name, metric_name}; use torrust_tracker_primitives::DurationSinceUnixEpoch; use crate::event::Event; @@ -26,7 +26,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(context); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new("connect")); + label_set.upsert(label_name!("request_kind"), LabelValue::new("connect")); stats_repository .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) @@ -47,7 +47,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(context); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new("announce")); + label_set.upsert(label_name!("request_kind"), LabelValue::new("announce")); stats_repository .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) @@ -68,7 +68,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(context); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new("scrape")); + label_set.upsert(label_name!("request_kind"), LabelValue::new("scrape")); stats_repository .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) diff --git a/packages/udp-tracker-server/src/event/mod.rs b/packages/udp-tracker-server/src/event/mod.rs index 316e1a414..0236b26a9 100644 --- a/packages/udp-tracker-server/src/event/mod.rs +++ b/packages/udp-tracker-server/src/event/mod.rs @@ -2,7 +2,8 @@ use std::fmt; use std::net::SocketAddr; use std::time::Duration; -use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; +use torrust_tracker_metrics::label::{LabelSet, LabelValue}; +use torrust_tracker_metrics::label_name; use torrust_tracker_primitives::service_binding::ServiceBinding; pub mod sender; @@ -94,15 +95,15 @@ impl From for LabelSet { fn from(connection_context: ConnectionContext) -> Self { LabelSet::from([ ( - LabelName::new("server_binding_protocol"), + label_name!("server_binding_protocol"), LabelValue::new(&connection_context.server_service_binding.protocol().to_string()), ), ( - LabelName::new("server_binding_ip"), + label_name!("server_binding_ip"), LabelValue::new(&connection_context.server_service_binding.bind_address().ip().to_string()), ), ( - LabelName::new("server_binding_port"), + label_name!("server_binding_port"), LabelValue::new(&connection_context.server_service_binding.bind_address().port().to_string()), ), ]) diff --git a/packages/udp-tracker-server/src/statistics/event/handler.rs b/packages/udp-tracker-server/src/statistics/event/handler.rs index 721d415ea..430bbc34c 100644 --- a/packages/udp-tracker-server/src/statistics/event/handler.rs +++ b/packages/udp-tracker-server/src/statistics/event/handler.rs @@ -1,5 +1,5 @@ -use torrust_tracker_metrics::label::{LabelName, LabelSet, LabelValue}; -use torrust_tracker_metrics::metric_name; +use torrust_tracker_metrics::label::{LabelSet, LabelValue}; +use torrust_tracker_metrics::{label_name, metric_name}; use torrust_tracker_primitives::DurationSinceUnixEpoch; use crate::event::{Event, UdpRequestKind, UdpResponseKind}; @@ -97,7 +97,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context); - label_set.upsert(LabelName::new("kind"), LabelValue::new(&kind.to_string())); + label_set.upsert(label_name!("kind"), LabelValue::new(&kind.to_string())); stats_repository .increase_counter(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), &label_set, now) @@ -128,7 +128,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(context.clone()); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new(&req_kind.to_string())); + label_set.upsert(label_name!("request_kind"), LabelValue::new(&req_kind.to_string())); stats_repository .set_gauge( @@ -149,7 +149,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(context.clone()); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new(&req_kind.to_string())); + label_set.upsert(label_name!("request_kind"), LabelValue::new(&req_kind.to_string())); stats_repository .set_gauge( @@ -170,7 +170,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura // Extendable metrics let mut label_set = LabelSet::from(context.clone()); - label_set.upsert(LabelName::new("request_kind"), LabelValue::new(&req_kind.to_string())); + label_set.upsert(label_name!("request_kind"), LabelValue::new(&req_kind.to_string())); stats_repository .set_gauge( @@ -192,9 +192,9 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context); if result_label_value == LabelValue::new("ok") { - label_set.upsert(LabelName::new("request_kind"), kind_label_value); + label_set.upsert(label_name!("request_kind"), kind_label_value); } - label_set.upsert(LabelName::new("result"), result_label_value); + label_set.upsert(label_name!("result"), result_label_value); stats_repository .increase_counter(&metric_name!(UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL), &label_set, now) From d263be70a5ed5c5f64c4ed296f0a930c1c48dee7 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 12:20:23 +0100 Subject: [PATCH 05/12] refactor: [#1445] return optionals for metric values in metric collection --- packages/metrics/src/metric_collection.rs | 60 ++++++++++------------- 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/packages/metrics/src/metric_collection.rs b/packages/metrics/src/metric_collection.rs index 5b3f92a19..c4db0706f 100644 --- a/packages/metrics/src/metric_collection.rs +++ b/packages/metrics/src/metric_collection.rs @@ -62,7 +62,12 @@ impl MetricCollection { } #[must_use] - pub fn get_counter_value(&self, name: &MetricName, label_set: &LabelSet) -> Counter { + pub fn contains_counter(&self, name: &MetricName) -> bool { + self.counters.metrics.contains_key(name) + } + + #[must_use] + pub fn get_counter_value(&self, name: &MetricName, label_set: &LabelSet) -> Option { self.counters.get_value(name, label_set) } @@ -89,7 +94,12 @@ impl MetricCollection { } #[must_use] - pub fn get_gauge_value(&self, name: &MetricName, label_set: &LabelSet) -> Gauge { + pub fn contains_gauge(&self, name: &MetricName) -> bool { + self.gauges.metrics.contains_key(name) + } + + #[must_use] + pub fn get_gauge_value(&self, name: &MetricName, label_set: &LabelSet) -> Option { self.gauges.get_value(name, label_set) } @@ -275,11 +285,11 @@ impl MetricKindCollection { } #[must_use] - pub fn get_value(&self, name: &MetricName, label_set: &LabelSet) -> Counter { + pub fn get_value(&self, name: &MetricName, label_set: &LabelSet) -> Option { self.metrics .get(name) .and_then(|metric| metric.get_sample_data(label_set)) - .map_or(Counter::default(), |sample| sample.value().clone()) + .map(|sample| sample.value().clone()) } } @@ -300,11 +310,11 @@ impl MetricKindCollection { } #[must_use] - pub fn get_value(&self, name: &MetricName, label_set: &LabelSet) -> Gauge { + pub fn get_value(&self, name: &MetricName, label_set: &LabelSet) -> Option { self.metrics .get(name) .and_then(|metric| metric.get_sample_data(label_set)) - .map_or(Gauge::default(), |sample| sample.value().clone()) + .map(|sample| sample.value().clone()) } } @@ -588,7 +598,7 @@ mod tests { assert_eq!( metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), - Counter::new(2) + Some(Counter::new(2)) ); } @@ -605,38 +615,28 @@ mod tests { assert_eq!( metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), - Counter::new(2) + Some(Counter::new(2)) ); } #[test] fn it_should_allow_making_sure_a_counter_exists_without_increasing_it() { - let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); - let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); metric_collection.ensure_counter_exists(&metric_name!("test_counter")); - assert_eq!( - metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), - Counter::default() - ); + assert!(metric_collection.contains_counter(&metric_name!("test_counter"))); } #[test] fn it_should_allow_describing_a_counter_before_using_it() { - let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); - let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); metric_collection.describe_counter(&metric_name!("test_counter"), None, None); - assert_eq!( - metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), - Counter::default() - ); + assert!(metric_collection.contains_counter(&metric_name!("test_counter"))); } #[test] @@ -683,7 +683,7 @@ mod tests { assert_eq!( metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), - Gauge::new(1.0) + Some(Gauge::new(1.0)) ); } @@ -699,38 +699,28 @@ mod tests { assert_eq!( metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), - Gauge::new(1.0) + Some(Gauge::new(1.0)) ); } #[test] - fn it_should_allow_making_sure_a_gauge_exists_without_increasing_it() { - let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); - + fn it_should_allow_making_sure_a_gauge_exists_without_setting_it() { let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); metric_collection.ensure_gauge_exists(&metric_name!("test_gauge")); - assert_eq!( - metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), - Gauge::default() - ); + assert!(metric_collection.contains_gauge(&metric_name!("test_gauge"))); } #[test] fn it_should_allow_describing_a_gauge_before_using_it() { - let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); - let mut metric_collection = MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); metric_collection.describe_gauge(&metric_name!("test_gauge"), None, None); - assert_eq!( - metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), - Gauge::default() - ); + assert!(metric_collection.contains_gauge(&metric_name!("test_gauge"))); } #[test] From 785a978e0d2ac45d70dc6f5c2a4109ced175d4d9 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 13:03:15 +0100 Subject: [PATCH 06/12] refactor: [#1445] remove panic from MetricColelction::new method --- packages/metrics/src/metric_collection.rs | 64 +++++++++++++---------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/packages/metrics/src/metric_collection.rs b/packages/metrics/src/metric_collection.rs index c4db0706f..29da4e509 100644 --- a/packages/metrics/src/metric_collection.rs +++ b/packages/metrics/src/metric_collection.rs @@ -27,21 +27,20 @@ pub struct MetricCollection { } impl MetricCollection { - /// # Panics + /// # Errors /// - /// Panics if there are duplicate metric names across counters and gauges. - #[must_use] - pub fn new(counters: MetricKindCollection, gauges: MetricKindCollection) -> Self { + /// Returns an error if there are duplicate metric names across counters and + /// gauges. + pub fn new(counters: MetricKindCollection, gauges: MetricKindCollection) -> Result { // Check for name collisions across metric types let counter_names: HashSet<_> = counters.names().collect(); let gauge_names: HashSet<_> = gauges.names().collect(); - assert!( - counter_names.is_disjoint(&gauge_names), - "Metric names must be unique across counters and gauges" - ); + if !counter_names.is_disjoint(&gauge_names) { + return Err(Error::DuplicateMetricNames); + } - Self { counters, gauges } + Ok(Self { counters, gauges }) } /// Merges another `MetricCollection` into this one. @@ -49,7 +48,7 @@ impl MetricCollection { /// # Errors /// /// Returns an error if a metric name already exists in the current collection. - pub fn merge(&mut self, other: &Self) -> Result<(), MergeError> { + pub fn merge(&mut self, other: &Self) -> Result<(), Error> { self.counters.merge(&other.counters)?; self.gauges.merge(&other.gauges)?; Ok(()) @@ -121,9 +120,12 @@ impl MetricCollection { } #[derive(thiserror::Error, Debug, Clone)] -pub enum MergeError { +pub enum Error { + #[error("Metric names must be unique across counters and gauges.")] + DuplicateMetricNames, + #[error("Cannot merge metric '{metric_name}': it already exists in the current collection")] - MetricNameAlreadyExists { metric_name: MetricName }, + MetricNameCollisionInMerge { metric_name: MetricName }, } /// Implements serialization for `MetricCollection`. @@ -177,10 +179,10 @@ impl<'de> Deserialize<'de> for MetricCollection { } } - Ok(MetricCollection::new( - MetricKindCollection::new(counters), - MetricKindCollection::new(gauges), - )) + let metric_collection = MetricCollection::new(MetricKindCollection::new(counters), MetricKindCollection::new(gauges)) + .map_err(serde::de::Error::custom)?; + + Ok(metric_collection) } } @@ -246,11 +248,11 @@ impl MetricKindCollection { /// # Errors /// /// Returns an error if a metric name already exists in the current collection. - pub fn merge(&mut self, other: &Self) -> Result<(), MergeError> { + pub fn merge(&mut self, other: &Self) -> Result<(), Error> { // Check for name collisions for metric_name in other.metrics.keys() { if self.metrics.contains_key(metric_name) { - return Err(MergeError::MetricNameAlreadyExists { + return Err(Error::MetricNameCollisionInMerge { metric_name: metric_name.clone(), }); } @@ -258,7 +260,7 @@ impl MetricKindCollection { for (metric_name, metric) in &other.metrics { if self.metrics.insert(metric_name.clone(), metric.clone()).is_some() { - return Err(MergeError::MetricNameAlreadyExists { + return Err(Error::MetricNameCollisionInMerge { metric_name: metric_name.clone(), }); } @@ -374,6 +376,7 @@ mod tests { SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set_1.clone())]), )]), ) + .unwrap() } fn json() -> String { @@ -540,7 +543,8 @@ mod tests { ]), )]), MetricKindCollection::new(vec![]), - ); + ) + .unwrap(); let prometheus_output = metric_collection.to_prometheus(); @@ -565,7 +569,7 @@ mod tests { counters.ensure_metric_exists(&metric_name!("test_counter")); gauges.ensure_metric_exists(&metric_name!("test_gauge")); - let metric_collection = MetricCollection::new(counters, gauges); + let metric_collection = MetricCollection::new(counters, gauges).unwrap(); let prometheus_output = metric_collection.to_prometheus(); @@ -591,7 +595,8 @@ mod tests { SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), )]), MetricKindCollection::new(vec![]), - ); + ) + .unwrap(); metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); @@ -608,7 +613,7 @@ mod tests { let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); + MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); @@ -622,7 +627,7 @@ mod tests { #[test] fn it_should_allow_making_sure_a_counter_exists_without_increasing_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); + MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); metric_collection.ensure_counter_exists(&metric_name!("test_counter")); @@ -632,7 +637,7 @@ mod tests { #[test] fn it_should_allow_describing_a_counter_before_using_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); + MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); metric_collection.describe_counter(&metric_name!("test_counter"), None, None); @@ -677,7 +682,8 @@ mod tests { metric_name!("test_gauge"), SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), )]), - ); + ) + .unwrap(); metric_collection.set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time); @@ -693,7 +699,7 @@ mod tests { let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); + MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); metric_collection.set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time); @@ -706,7 +712,7 @@ mod tests { #[test] fn it_should_allow_making_sure_a_gauge_exists_without_setting_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); + MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); metric_collection.ensure_gauge_exists(&metric_name!("test_gauge")); @@ -716,7 +722,7 @@ mod tests { #[test] fn it_should_allow_describing_a_gauge_before_using_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])); + MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); metric_collection.describe_gauge(&metric_name!("test_gauge"), None, None); From 42e1524c560f546d0b57c61b50bf83c928112990 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 16:21:49 +0100 Subject: [PATCH 07/12] refactor: [#1445] return errors instead of panicking in the MetricCollection struct --- .../src/statistics/event/handler.rs | 16 +- .../src/statistics/metrics.rs | 27 ++- .../src/statistics/repository.rs | 18 +- packages/metrics/src/metric_collection.rs | 229 +++++++++++------- .../src/statistics/event/handler.rs | 24 +- .../src/statistics/metrics.rs | 29 ++- .../src/statistics/repository.rs | 18 +- .../src/statistics/event/handler.rs | 72 ++++-- .../src/statistics/metrics.rs | 27 ++- .../src/statistics/repository.rs | 36 ++- 10 files changed, 363 insertions(+), 133 deletions(-) diff --git a/packages/http-tracker-core/src/statistics/event/handler.rs b/packages/http-tracker-core/src/statistics/event/handler.rs index cea224d04..182c86b01 100644 --- a/packages/http-tracker-core/src/statistics/event/handler.rs +++ b/packages/http-tracker-core/src/statistics/event/handler.rs @@ -31,9 +31,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(connection); label_set.upsert(label_name!("request_kind"), LabelValue::new("announce")); - stats_repository + match stats_repository .increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::TcpScrape { connection } => { // Global fixed metrics @@ -52,9 +56,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(connection); label_set.upsert(label_name!("request_kind"), LabelValue::new("scrape")); - stats_repository + match stats_repository .increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } } diff --git a/packages/http-tracker-core/src/statistics/metrics.rs b/packages/http-tracker-core/src/statistics/metrics.rs index 0b442c1cb..bf053b04e 100644 --- a/packages/http-tracker-core/src/statistics/metrics.rs +++ b/packages/http-tracker-core/src/statistics/metrics.rs @@ -1,7 +1,7 @@ use serde::Serialize; use torrust_tracker_metrics::label::LabelSet; use torrust_tracker_metrics::metric::MetricName; -use torrust_tracker_metrics::metric_collection::MetricCollection; +use torrust_tracker_metrics::metric_collection::{Error, MetricCollection}; use torrust_tracker_primitives::DurationSinceUnixEpoch; /// Metrics collected by the tracker. @@ -24,11 +24,28 @@ pub struct Metrics { } impl Metrics { - pub fn increase_counter(&mut self, metric_name: &MetricName, labels: &LabelSet, now: DurationSinceUnixEpoch) { - self.metric_collection.increase_counter(metric_name, labels, now); + /// # Errors + /// + /// Returns an error if the metric does not exist and it cannot be created. + pub fn increase_counter( + &mut self, + metric_name: &MetricName, + labels: &LabelSet, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + self.metric_collection.increase_counter(metric_name, labels, now) } - pub fn set_gauge(&mut self, metric_name: &MetricName, labels: &LabelSet, value: f64, now: DurationSinceUnixEpoch) { - self.metric_collection.set_gauge(metric_name, labels, value, now); + /// # Errors + /// + /// Returns an error if the metric does not exist and it cannot be created. + pub fn set_gauge( + &mut self, + metric_name: &MetricName, + labels: &LabelSet, + value: f64, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + self.metric_collection.set_gauge(metric_name, labels, value, now) } } diff --git a/packages/http-tracker-core/src/statistics/repository.rs b/packages/http-tracker-core/src/statistics/repository.rs index 88345722b..d5e718821 100644 --- a/packages/http-tracker-core/src/statistics/repository.rs +++ b/packages/http-tracker-core/src/statistics/repository.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use tokio::sync::{RwLock, RwLockReadGuard}; use torrust_tracker_metrics::label::LabelSet; use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_collection::Error; use torrust_tracker_primitives::DurationSinceUnixEpoch; use super::describe_metrics; @@ -56,9 +57,22 @@ impl Repository { drop(stats_lock); } - pub async fn increase_counter(&self, metric_name: &MetricName, labels: &LabelSet, now: DurationSinceUnixEpoch) { + /// # Errors + /// + /// This function will return an error if the metric collection fails to + /// increase the counter. + pub async fn increase_counter( + &self, + metric_name: &MetricName, + labels: &LabelSet, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { let mut stats_lock = self.stats.write().await; - stats_lock.increase_counter(metric_name, labels, now); + + let result = stats_lock.increase_counter(metric_name, labels, now); + drop(stats_lock); + + result } } diff --git a/packages/metrics/src/metric_collection.rs b/packages/metrics/src/metric_collection.rs index 29da4e509..c719e6054 100644 --- a/packages/metrics/src/metric_collection.rs +++ b/packages/metrics/src/metric_collection.rs @@ -13,13 +13,10 @@ use crate::metric::description::MetricDescription; use crate::sample_collection::SampleCollection; use crate::unit::Unit; -// todo: serialize in a deterministic order. For example: +// code-review: serialize in a deterministic order? For example: // - First the counter metrics ordered by name. // - Then the gauge metrics ordered by name. -/// Use this type only when behind a lock that guarantees thread-safety. -/// Otherwise, there could be race conditions that lead to duplicate metric -/// names in different metric types. #[derive(Debug, Clone, Default, PartialEq)] pub struct MetricCollection { counters: MetricKindCollection, @@ -37,7 +34,10 @@ impl MetricCollection { let gauge_names: HashSet<_> = gauges.names().collect(); if !counter_names.is_disjoint(&gauge_names) { - return Err(Error::DuplicateMetricNames); + return Err(Error::MetricNameCollisionInConstructor { + counter_names: counter_names.iter().map(std::string::ToString::to_string).collect(), + gauge_names: gauge_names.iter().map(std::string::ToString::to_string).collect(), + }); } Ok(Self { counters, gauges }) @@ -70,16 +70,25 @@ impl MetricCollection { self.counters.get_value(name, label_set) } - /// # Panics + /// # Errors /// - /// Panics if a gauge with the same name already exists. - pub fn increase_counter(&mut self, name: &MetricName, label_set: &LabelSet, time: DurationSinceUnixEpoch) { - assert!( - !self.gauges.metrics.contains_key(name), - "Cannot create counter with name '{name}': a gauge with this name already exists", - ); + /// Return an error if a metrics of a different type with the same name + /// already exists. + pub fn increase_counter( + &mut self, + name: &MetricName, + label_set: &LabelSet, + time: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + if self.gauges.metrics.contains_key(name) { + return Err(Error::MetricNameCollisionAdding { + metric_name: name.clone(), + }); + } self.counters.increment(name, label_set, time); + + Ok(()) } pub fn ensure_counter_exists(&mut self, name: &MetricName) { @@ -102,16 +111,26 @@ impl MetricCollection { self.gauges.get_value(name, label_set) } - /// # Panics + /// # Errors /// - /// Panics if a counter with the same name already exists. - pub fn set_gauge(&mut self, name: &MetricName, label_set: &LabelSet, value: f64, time: DurationSinceUnixEpoch) { - assert!( - !self.counters.metrics.contains_key(name), - "Cannot create gauge with name '{name}': a counter with this name already exists" - ); + /// Return an error if a metrics of a different type with the same name + /// already exists. + pub fn set_gauge( + &mut self, + name: &MetricName, + label_set: &LabelSet, + value: f64, + time: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + if self.counters.metrics.contains_key(name) { + return Err(Error::MetricNameCollisionAdding { + metric_name: name.clone(), + }); + } self.gauges.set(name, label_set, value, time); + + Ok(()) } pub fn ensure_gauge_exists(&mut self, name: &MetricName) { @@ -121,11 +140,20 @@ impl MetricCollection { #[derive(thiserror::Error, Debug, Clone)] pub enum Error { - #[error("Metric names must be unique across counters and gauges.")] - DuplicateMetricNames, + #[error("Metric names must be unique across all metrics types.")] + MetricNameCollisionInConstructor { + counter_names: Vec, + gauge_names: Vec, + }, + + #[error("Found duplicate metric name in list. Metric names must be unique across all metrics types.")] + DuplicateMetricNameInList { metric_name: MetricName }, #[error("Cannot merge metric '{metric_name}': it already exists in the current collection")] MetricNameCollisionInMerge { metric_name: MetricName }, + + #[error("Cannot create metric with name '{metric_name}': another metric with this name already exists")] + MetricNameCollisionAdding { metric_name: MetricName }, } /// Implements serialization for `MetricCollection`. @@ -179,8 +207,10 @@ impl<'de> Deserialize<'de> for MetricCollection { } } - let metric_collection = MetricCollection::new(MetricKindCollection::new(counters), MetricKindCollection::new(gauges)) - .map_err(serde::de::Error::custom)?; + let counters = MetricKindCollection::new(counters).map_err(serde::de::Error::custom)?; + let gauges = MetricKindCollection::new(gauges).map_err(serde::de::Error::custom)?; + + let metric_collection = MetricCollection::new(counters, gauges).map_err(serde::de::Error::custom)?; Ok(metric_collection) } @@ -213,20 +243,21 @@ pub struct MetricKindCollection { impl MetricKindCollection { /// Creates a new `MetricKindCollection` from a vector of metrics /// - /// # Panics + /// # Errors /// - /// Panics if duplicate metric names are found - #[must_use] - pub fn new(metrics: Vec>) -> Self { + /// Returns an error if duplicate metric names are passed. + pub fn new(metrics: Vec>) -> Result { let mut map = HashMap::with_capacity(metrics.len()); for metric in metrics { - assert!( - map.insert(metric.name().clone(), metric).is_none(), - "Duplicate MetricName found in MetricKindCollection" - ); + let metric_name = metric.name().clone(); + + if let Some(_old_metric) = map.insert(metric.name().clone(), metric) { + return Err(Error::DuplicateMetricNameInList { metric_name }); + } } - Self { metrics: map } + + Ok(Self { metrics: map }) } /// Returns an iterator over all metric names in this collection. @@ -234,10 +265,18 @@ impl MetricKindCollection { self.metrics.keys() } + /// # Panics + /// + /// It should not panic as long as empty sample collections are allowed. pub fn ensure_metric_exists(&mut self, name: &MetricName) { if !self.metrics.contains_key(name) { - self.metrics - .insert(name.clone(), Metric::new(name.clone(), SampleCollection::new(vec![]))); + self.metrics.insert( + name.clone(), + Metric::new( + name.clone(), + SampleCollection::new(vec![]).expect("Empty sample collection creation should not fail"), + ), + ); } } } @@ -369,12 +408,14 @@ mod tests { MetricCollection::new( MetricKindCollection::new(vec![Metric::new( metric_name!("http_tracker_core_announce_requests_received_total"), - SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set_1.clone())]), - )]), + SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set_1.clone())]).unwrap(), + )]) + .unwrap(), MetricKindCollection::new(vec![Metric::new( metric_name!("udp_tracker_server_performance_avg_announce_processing_time_ns"), - SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set_1.clone())]), - )]), + SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set_1.clone())]).unwrap(), + )]) + .unwrap(), ) .unwrap() } @@ -446,41 +487,47 @@ mod tests { } #[test] - #[should_panic(expected = "Metric names must be unique across counters and gauges")] fn it_should_not_allow_duplicate_names_across_types() { - let counter = MetricKindCollection::new(vec![Metric::new(metric_name!("test_metric"), SampleCollection::new(vec![]))]); - - let gauge = MetricKindCollection::new(vec![Metric::new(metric_name!("test_metric"), SampleCollection::new(vec![]))]); + let counters = + MetricKindCollection::new(vec![Metric::new(metric_name!("test_metric"), SampleCollection::default())]).unwrap(); + let gauges = + MetricKindCollection::new(vec![Metric::new(metric_name!("test_metric"), SampleCollection::default())]).unwrap(); - let _unused = MetricCollection::new(counter, gauge); + assert!(MetricCollection::new(counters, gauges).is_err()); } #[test] - #[should_panic(expected = "Cannot create gauge with name 'test_metric': a counter with this name already exists")] fn it_should_not_allow_creating_a_gauge_with_the_same_name_as_a_counter() { let mut collection = MetricCollection::default(); let label_set = LabelSet::default(); let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); // First create a counter - collection.increase_counter(&metric_name!("test_metric"), &label_set, time); + collection + .increase_counter(&metric_name!("test_metric"), &label_set, time) + .unwrap(); + + // Then try to create a gauge with the same name + let result = collection.set_gauge(&metric_name!("test_metric"), &label_set, 1.0, time); - // Then try to create a gauge with the same name - this should panic - collection.set_gauge(&metric_name!("test_metric"), &label_set, 1.0, time); + assert!(result.is_err()); } #[test] - #[should_panic(expected = "Cannot create counter with name 'test_metric': a gauge with this name already exists")] fn it_should_not_allow_creating_a_counter_with_the_same_name_as_a_gauge() { let mut collection = MetricCollection::default(); let label_set = LabelSet::default(); let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); // First set the gauge - collection.set_gauge(&metric_name!("test_metric"), &label_set, 1.0, time); + collection + .set_gauge(&metric_name!("test_metric"), &label_set, 1.0, time) + .unwrap(); + + // Then try to create a counter with the same name + let result = collection.increase_counter(&metric_name!("test_metric"), &label_set, time); - // Then try to create a counter with the same name - this should panic - collection.increase_counter(&metric_name!("test_metric"), &label_set, time); + assert!(result.is_err()); } #[test] @@ -540,9 +587,11 @@ mod tests { SampleCollection::new(vec![ Sample::new(Counter::new(1), time, label_set_1.clone()), Sample::new(Counter::new(2), time, label_set_2.clone()), - ]), - )]), - MetricKindCollection::new(vec![]), + ]) + .unwrap(), + )]) + .unwrap(), + MetricKindCollection::default(), ) .unwrap(); @@ -563,8 +612,8 @@ mod tests { #[test] fn it_should_exclude_metrics_without_samples_from_prometheus_format() { - let mut counters = MetricKindCollection::new(vec![]); - let mut gauges = MetricKindCollection::new(vec![]); + let mut counters = MetricKindCollection::default(); + let mut gauges = MetricKindCollection::default(); counters.ensure_metric_exists(&metric_name!("test_counter")); gauges.ensure_metric_exists(&metric_name!("test_gauge")); @@ -592,14 +641,19 @@ mod tests { let mut metric_collection = MetricCollection::new( MetricKindCollection::new(vec![Metric::new( metric_name!("test_counter"), - SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), - )]), - MetricKindCollection::new(vec![]), + SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]).unwrap(), + )]) + .unwrap(), + MetricKindCollection::default(), ) .unwrap(); - metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); - metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); + metric_collection + .increase_counter(&metric_name!("test_counter"), &label_set, time) + .unwrap(); + metric_collection + .increase_counter(&metric_name!("test_counter"), &label_set, time) + .unwrap(); assert_eq!( metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), @@ -613,10 +667,14 @@ mod tests { let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); + MetricCollection::new(MetricKindCollection::default(), MetricKindCollection::default()).unwrap(); - metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); - metric_collection.increase_counter(&metric_name!("test_counter"), &label_set, time); + metric_collection + .increase_counter(&metric_name!("test_counter"), &label_set, time) + .unwrap(); + metric_collection + .increase_counter(&metric_name!("test_counter"), &label_set, time) + .unwrap(); assert_eq!( metric_collection.get_counter_value(&metric_name!("test_counter"), &label_set), @@ -627,7 +685,7 @@ mod tests { #[test] fn it_should_allow_making_sure_a_counter_exists_without_increasing_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); + MetricCollection::new(MetricKindCollection::default(), MetricKindCollection::default()).unwrap(); metric_collection.ensure_counter_exists(&metric_name!("test_counter")); @@ -637,7 +695,7 @@ mod tests { #[test] fn it_should_allow_describing_a_counter_before_using_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); + MetricCollection::new(MetricKindCollection::default(), MetricKindCollection::default()).unwrap(); metric_collection.describe_counter(&metric_name!("test_counter"), None, None); @@ -645,21 +703,22 @@ mod tests { } #[test] - #[should_panic(expected = "Duplicate MetricName found in MetricKindCollection")] fn it_should_not_allow_duplicate_metric_names_when_instantiating() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); - let _unused = MetricKindCollection::new(vec![ + let result = MetricKindCollection::new(vec![ Metric::new( metric_name!("test_counter"), - SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), + SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]).unwrap(), ), Metric::new( metric_name!("test_counter"), - SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]), + SampleCollection::new(vec![Sample::new(Counter::new(0), time, label_set.clone())]).unwrap(), ), ]); + + assert!(result.is_err()); } } @@ -677,15 +736,18 @@ mod tests { let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = MetricCollection::new( - MetricKindCollection::new(vec![]), + MetricKindCollection::default(), MetricKindCollection::new(vec![Metric::new( metric_name!("test_gauge"), - SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), - )]), + SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]).unwrap(), + )]) + .unwrap(), ) .unwrap(); - metric_collection.set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time); + metric_collection + .set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time) + .unwrap(); assert_eq!( metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), @@ -699,9 +761,11 @@ mod tests { let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); + MetricCollection::new(MetricKindCollection::default(), MetricKindCollection::default()).unwrap(); - metric_collection.set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time); + metric_collection + .set_gauge(&metric_name!("test_gauge"), &label_set, 1.0, time) + .unwrap(); assert_eq!( metric_collection.get_gauge_value(&metric_name!("test_gauge"), &label_set), @@ -712,7 +776,7 @@ mod tests { #[test] fn it_should_allow_making_sure_a_gauge_exists_without_setting_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); + MetricCollection::new(MetricKindCollection::default(), MetricKindCollection::default()).unwrap(); metric_collection.ensure_gauge_exists(&metric_name!("test_gauge")); @@ -722,7 +786,7 @@ mod tests { #[test] fn it_should_allow_describing_a_gauge_before_using_it() { let mut metric_collection = - MetricCollection::new(MetricKindCollection::new(vec![]), MetricKindCollection::new(vec![])).unwrap(); + MetricCollection::new(MetricKindCollection::default(), MetricKindCollection::default()).unwrap(); metric_collection.describe_gauge(&metric_name!("test_gauge"), None, None); @@ -730,21 +794,22 @@ mod tests { } #[test] - #[should_panic(expected = "Duplicate MetricName found in MetricKindCollection")] fn it_should_not_allow_duplicate_metric_names_when_instantiating() { let time = DurationSinceUnixEpoch::from_secs(1_743_552_000); let label_set: LabelSet = (label_name!("label_name"), LabelValue::new("value")).into(); - let _unused = MetricKindCollection::new(vec![ + let result = MetricKindCollection::new(vec![ Metric::new( metric_name!("test_gauge"), - SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), + SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]).unwrap(), ), Metric::new( metric_name!("test_gauge"), - SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]), + SampleCollection::new(vec![Sample::new(Gauge::new(0.0), time, label_set.clone())]).unwrap(), ), ]); + + assert!(result.is_err()); } } } diff --git a/packages/udp-tracker-core/src/statistics/event/handler.rs b/packages/udp-tracker-core/src/statistics/event/handler.rs index 13a4840d5..2680c442f 100644 --- a/packages/udp-tracker-core/src/statistics/event/handler.rs +++ b/packages/udp-tracker-core/src/statistics/event/handler.rs @@ -28,9 +28,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context); label_set.upsert(label_name!("request_kind"), LabelValue::new("connect")); - stats_repository + match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpAnnounce { context } => { // Global fixed metrics @@ -49,9 +53,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context); label_set.upsert(label_name!("request_kind"), LabelValue::new("announce")); - stats_repository + match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpScrape { context } => { // Global fixed metrics @@ -70,9 +78,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context); label_set.upsert(label_name!("request_kind"), LabelValue::new("scrape")); - stats_repository + match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } } diff --git a/packages/udp-tracker-core/src/statistics/metrics.rs b/packages/udp-tracker-core/src/statistics/metrics.rs index 23cec8036..94aa7d08f 100644 --- a/packages/udp-tracker-core/src/statistics/metrics.rs +++ b/packages/udp-tracker-core/src/statistics/metrics.rs @@ -1,7 +1,7 @@ use serde::Serialize; use torrust_tracker_metrics::label::LabelSet; use torrust_tracker_metrics::metric::MetricName; -use torrust_tracker_metrics::metric_collection::MetricCollection; +use torrust_tracker_metrics::metric_collection::{Error, MetricCollection}; use torrust_tracker_primitives::DurationSinceUnixEpoch; /// Metrics collected by the tracker. @@ -37,11 +37,30 @@ pub struct Metrics { } impl Metrics { - pub fn increase_counter(&mut self, metric_name: &MetricName, labels: &LabelSet, now: DurationSinceUnixEpoch) { - self.metric_collection.increase_counter(metric_name, labels, now); + /// # Errors + /// + /// This function returns an error if the metric does not exist and it + /// cannot be created. + pub fn increase_counter( + &mut self, + metric_name: &MetricName, + labels: &LabelSet, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + self.metric_collection.increase_counter(metric_name, labels, now) } - pub fn set_gauge(&mut self, metric_name: &MetricName, labels: &LabelSet, value: f64, now: DurationSinceUnixEpoch) { - self.metric_collection.set_gauge(metric_name, labels, value, now); + /// # Errors + /// + /// This function returns an error if the metric does not exist and it + /// cannot be created. + pub fn set_gauge( + &mut self, + metric_name: &MetricName, + labels: &LabelSet, + value: f64, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + self.metric_collection.set_gauge(metric_name, labels, value, now) } } diff --git a/packages/udp-tracker-core/src/statistics/repository.rs b/packages/udp-tracker-core/src/statistics/repository.rs index 49c91c751..c68fa14f7 100644 --- a/packages/udp-tracker-core/src/statistics/repository.rs +++ b/packages/udp-tracker-core/src/statistics/repository.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use tokio::sync::{RwLock, RwLockReadGuard}; use torrust_tracker_metrics::label::LabelSet; use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_collection::Error; use torrust_tracker_primitives::DurationSinceUnixEpoch; use super::describe_metrics; @@ -68,9 +69,22 @@ impl Repository { drop(stats_lock); } - pub async fn increase_counter(&self, metric_name: &MetricName, labels: &LabelSet, now: DurationSinceUnixEpoch) { + /// # Errors + /// + /// This function will return an error if the metric collection fails to + /// increase the counter. + pub async fn increase_counter( + &self, + metric_name: &MetricName, + labels: &LabelSet, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { let mut stats_lock = self.stats.write().await; - stats_lock.increase_counter(metric_name, labels, now); + + let result = stats_lock.increase_counter(metric_name, labels, now); + drop(stats_lock); + + result } } diff --git a/packages/udp-tracker-server/src/statistics/event/handler.rs b/packages/udp-tracker-server/src/statistics/event/handler.rs index 430bbc34c..092ce93f2 100644 --- a/packages/udp-tracker-server/src/statistics/event/handler.rs +++ b/packages/udp-tracker-server/src/statistics/event/handler.rs @@ -23,26 +23,34 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura stats_repository.increase_udp_requests_aborted().await; // Extendable metrics - stats_repository + match stats_repository .increase_counter( &metric_name!(UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL), &LabelSet::from(context), now, ) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpRequestBanned { context } => { // Global fixed metrics stats_repository.increase_udp_requests_banned().await; // Extendable metrics - stats_repository + match stats_repository .increase_counter( &metric_name!(UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL), &LabelSet::from(context), now, ) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpRequestReceived { context } => { // Global fixed metrics @@ -56,13 +64,17 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura } // Extendable metrics - stats_repository + match stats_repository .increase_counter( &metric_name!(UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL), &LabelSet::from(context), now, ) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpRequestAccepted { context, kind } => { // Global fixed metrics @@ -99,9 +111,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura label_set.upsert(label_name!("kind"), LabelValue::new(&kind.to_string())); - stats_repository + match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpResponseSent { context, @@ -130,14 +146,18 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context.clone()); label_set.upsert(label_name!("request_kind"), LabelValue::new(&req_kind.to_string())); - stats_repository + match stats_repository .set_gauge( &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), &label_set, new_avg, now, ) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to set gauge: {}", err), + } (LabelValue::new("ok"), LabelValue::new(&UdpRequestKind::Connect.to_string())) } @@ -151,14 +171,18 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context.clone()); label_set.upsert(label_name!("request_kind"), LabelValue::new(&req_kind.to_string())); - stats_repository + match stats_repository .set_gauge( &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), &label_set, new_avg, now, ) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to set gauge: {}", err), + } (LabelValue::new("ok"), LabelValue::new(&UdpRequestKind::Announce.to_string())) } @@ -172,14 +196,18 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context.clone()); label_set.upsert(label_name!("request_kind"), LabelValue::new(&req_kind.to_string())); - stats_repository + match stats_repository .set_gauge( &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), &label_set, new_avg, now, ) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to set gauge: {}", err), + } (LabelValue::new("ok"), LabelValue::new(&UdpRequestKind::Scrape.to_string())) } @@ -196,9 +224,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura } label_set.upsert(label_name!("result"), result_label_value); - stats_repository + match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL), &label_set, now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } Event::UdpError { context } => { // Global fixed metrics @@ -212,9 +244,13 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura } // Extendable metrics - stats_repository + match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_SERVER_ERRORS_TOTAL), &LabelSet::from(context), now) - .await; + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + }; } } diff --git a/packages/udp-tracker-server/src/statistics/metrics.rs b/packages/udp-tracker-server/src/statistics/metrics.rs index 4fe07e7da..7b18f6418 100644 --- a/packages/udp-tracker-server/src/statistics/metrics.rs +++ b/packages/udp-tracker-server/src/statistics/metrics.rs @@ -1,7 +1,7 @@ use serde::Serialize; use torrust_tracker_metrics::label::LabelSet; use torrust_tracker_metrics::metric::MetricName; -use torrust_tracker_metrics::metric_collection::MetricCollection; +use torrust_tracker_metrics::metric_collection::{Error, MetricCollection}; use torrust_tracker_primitives::DurationSinceUnixEpoch; /// Metrics collected by the UDP tracker server. @@ -69,11 +69,28 @@ pub struct Metrics { } impl Metrics { - pub fn increase_counter(&mut self, metric_name: &MetricName, labels: &LabelSet, now: DurationSinceUnixEpoch) { - self.metric_collection.increase_counter(metric_name, labels, now); + /// # Errors + /// + /// Returns an error if the metric does not exist and it cannot be created. + pub fn increase_counter( + &mut self, + metric_name: &MetricName, + labels: &LabelSet, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + self.metric_collection.increase_counter(metric_name, labels, now) } - pub fn set_gauge(&mut self, metric_name: &MetricName, labels: &LabelSet, value: f64, now: DurationSinceUnixEpoch) { - self.metric_collection.set_gauge(metric_name, labels, value, now); + /// # Errors + /// + /// Returns an error if the metric does not exist and it cannot be created. + pub fn set_gauge( + &mut self, + metric_name: &MetricName, + labels: &LabelSet, + value: f64, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { + self.metric_collection.set_gauge(metric_name, labels, value, now) } } diff --git a/packages/udp-tracker-server/src/statistics/repository.rs b/packages/udp-tracker-server/src/statistics/repository.rs index c33c1231c..1a1db89c7 100644 --- a/packages/udp-tracker-server/src/statistics/repository.rs +++ b/packages/udp-tracker-server/src/statistics/repository.rs @@ -4,6 +4,7 @@ use std::time::Duration; use tokio::sync::{RwLock, RwLockReadGuard}; use torrust_tracker_metrics::label::LabelSet; use torrust_tracker_metrics::metric::MetricName; +use torrust_tracker_metrics::metric_collection::Error; use torrust_tracker_primitives::DurationSinceUnixEpoch; use super::describe_metrics; @@ -181,15 +182,42 @@ impl Repository { drop(stats_lock); } - pub async fn increase_counter(&self, metric_name: &MetricName, labels: &LabelSet, now: DurationSinceUnixEpoch) { + /// # Errors + /// + /// This function will return an error if the metric collection fails to + /// increase the counter. + pub async fn increase_counter( + &self, + metric_name: &MetricName, + labels: &LabelSet, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { let mut stats_lock = self.stats.write().await; - stats_lock.increase_counter(metric_name, labels, now); + + let result = stats_lock.increase_counter(metric_name, labels, now); + drop(stats_lock); + + result } - pub async fn set_gauge(&self, metric_name: &MetricName, labels: &LabelSet, value: f64, now: DurationSinceUnixEpoch) { + /// # Errors + /// + /// This function will return an error if the metric collection fails to + /// increase the counter. + pub async fn set_gauge( + &self, + metric_name: &MetricName, + labels: &LabelSet, + value: f64, + now: DurationSinceUnixEpoch, + ) -> Result<(), Error> { let mut stats_lock = self.stats.write().await; - stats_lock.set_gauge(metric_name, labels, value, now); + + let result = stats_lock.set_gauge(metric_name, labels, value, now); + drop(stats_lock); + + result } } From 2ccb247ff5e890708fdfd3d89ba3bac8c659b6d7 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 15 Apr 2025 16:44:52 +0100 Subject: [PATCH 08/12] refactor: [#1445] return errors instead of panicking in the SampleCollection struct --- packages/metrics/src/metric/mod.rs | 6 +-- packages/metrics/src/sample_collection.rs | 65 ++++++++++++----------- 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/packages/metrics/src/metric/mod.rs b/packages/metrics/src/metric/mod.rs index 777981fd8..ecce90f18 100644 --- a/packages/metrics/src/metric/mod.rs +++ b/packages/metrics/src/metric/mod.rs @@ -108,7 +108,7 @@ mod tests { let label_set: LabelSet = [(label_name!("server_binding_protocol"), LabelValue::new("http"))].into(); - let samples = SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set.clone())]); + let samples = SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set.clone())]).unwrap(); Metric::::new(name.clone(), samples) } @@ -154,7 +154,7 @@ mod tests { let label_set: LabelSet = [(label_name!("server_binding_protocol"), LabelValue::new("http"))].into(); - let samples = SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set.clone())]); + let samples = SampleCollection::new(vec![Sample::new(Counter::new(1), time, label_set.clone())]).unwrap(); let metric = Metric::::new(name.clone(), samples); @@ -188,7 +188,7 @@ mod tests { let label_set: LabelSet = [(label_name!("server_binding_protocol"), LabelValue::new("http"))].into(); - let samples = SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set.clone())]); + let samples = SampleCollection::new(vec![Sample::new(Gauge::new(1.0), time, label_set.clone())]).unwrap(); let metric = Metric::::new(name.clone(), samples); diff --git a/packages/metrics/src/sample_collection.rs b/packages/metrics/src/sample_collection.rs index 436a4bc7d..49c839673 100644 --- a/packages/metrics/src/sample_collection.rs +++ b/packages/metrics/src/sample_collection.rs @@ -1,8 +1,8 @@ use std::collections::hash_map::Iter; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fmt::Write as _; -use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; use torrust_tracker_primitives::DurationSinceUnixEpoch; use super::counter::Counter; @@ -18,23 +18,28 @@ pub struct SampleCollection { } impl SampleCollection { - /// # Panics + /// Creates a new `MetricKindCollection` from a vector of metrics /// - /// Panics if there are duplicate `LabelSets` in the provided samples. - #[must_use] - pub fn new(samples: Vec>) -> Self { + /// # Errors + /// + /// Returns an error if there are duplicate `LabelSets` in the provided + /// samples. + pub fn new(samples: Vec>) -> Result { let mut map: HashMap> = HashMap::with_capacity(samples.len()); for sample in samples { let (label_set, sample_data): (LabelSet, Measurement) = sample.into(); - assert!( - map.insert(label_set, sample_data).is_none(), - "Duplicate LabelSet found in SampleCollection" - ); + let label_set_clone = label_set.clone(); + + if let Some(_old_measurement) = map.insert(label_set, sample_data) { + return Err(Error::DuplicateLabelSetInList { + label_set: label_set_clone, + }); + } } - Self { samples: map } + Ok(Self { samples: map }) } #[must_use] @@ -59,6 +64,12 @@ impl SampleCollection { } } +#[derive(thiserror::Error, Debug, Clone)] +pub enum Error { + #[error("Found duplicate label set in list. Label set must be unique in a SampleCollection.")] + DuplicateLabelSetInList { label_set: LabelSet }, +} + impl SampleCollection { pub fn increment(&mut self, label_set: &LabelSet, time: DurationSinceUnixEpoch) { let sample = self @@ -104,20 +115,11 @@ where where D: Deserializer<'de>, { - // First deserialize into a temporary Vec let samples = Vec::>::deserialize(deserializer)?; - // Check for duplicate label sets - let mut seen_labels = HashSet::new(); + let sample_collection = SampleCollection::new(samples).map_err(serde::de::Error::custom)?; - for sample in &samples { - if !seen_labels.insert(sample.labels()) { - return Err(de::Error::custom(format!("Duplicate label set found: {}", sample.labels()))); - } - } - - // Convert to HashMap-based storage - Ok(SampleCollection::new(samples)) + Ok(sample_collection) } } @@ -149,14 +151,15 @@ mod tests { } #[test] - #[should_panic(expected = "Duplicate LabelSet found in SampleCollection")] fn it_should_fail_trying_to_create_a_sample_collection_with_duplicate_label_sets() { let samples = vec![ Sample::new(Counter::default(), sample_update_time(), LabelSet::default()), Sample::new(Counter::default(), sample_update_time(), LabelSet::default()), ]; - let _unused = SampleCollection::new(samples); + let result = SampleCollection::new(samples); + + assert!(result.is_err()); } #[test] @@ -165,7 +168,7 @@ mod tests { let sample = Sample::new(Counter::default(), sample_update_time(), label_set.clone()); - let collection = SampleCollection::new(vec![sample.clone()]); + let collection = SampleCollection::new(vec![sample.clone()]).unwrap(); let retrieved = collection.get(&label_set); @@ -180,7 +183,7 @@ mod tests { let sample_1 = Sample::new(Counter::new(1), sample_update_time(), label_set_1.clone()); let sample_2 = Sample::new(Counter::new(2), sample_update_time(), label_set_2.clone()); - let collection = SampleCollection::new(vec![sample_1.clone(), sample_2.clone()]); + let collection = SampleCollection::new(vec![sample_1.clone(), sample_2.clone()]).unwrap(); let retrieved = collection.get(&label_set_1); assert_eq!(retrieved.unwrap(), sample_1.measurement()); @@ -192,7 +195,7 @@ mod tests { #[test] fn it_should_return_the_number_of_samples_in_the_collection() { let samples = vec![Sample::new(Counter::default(), sample_update_time(), LabelSet::default())]; - let collection = SampleCollection::new(samples); + let collection = SampleCollection::new(samples).unwrap(); assert_eq!(collection.len(), 1); } @@ -208,14 +211,14 @@ mod tests { assert!(empty.is_empty()); let samples = vec![Sample::new(Counter::default(), sample_update_time(), LabelSet::default())]; - let collection = SampleCollection::new(samples); + let collection = SampleCollection::new(samples).unwrap(); assert!(!collection.is_empty()); } #[test] fn it_should_be_serializable_and_deserializable_for_json_format() { let sample = Sample::new(Counter::default(), sample_update_time(), LabelSet::default()); - let collection = SampleCollection::new(vec![sample]); + let collection = SampleCollection::new(vec![sample]).unwrap(); let serialized = serde_json::to_string(&collection).unwrap(); let deserialized: SampleCollection = serde_json::from_str(&serialized).unwrap(); @@ -240,7 +243,7 @@ mod tests { #[test] fn it_should_be_exportable_to_prometheus_format_when_empty() { let sample = Sample::new(Counter::default(), sample_update_time(), LabelSet::default()); - let collection = SampleCollection::new(vec![sample]); + let collection = SampleCollection::new(vec![sample]).unwrap(); let prometheus_output = collection.to_prometheus(); @@ -255,7 +258,7 @@ mod tests { LabelSet::from(vec![("labe_name_1", "label value value 1")]), ); - let collection = SampleCollection::new(vec![sample]); + let collection = SampleCollection::new(vec![sample]).unwrap(); let prometheus_output = collection.to_prometheus(); From 13ea09183a8b8eb6e632b63aa6a831efe5509917 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 21 Apr 2025 09:57:11 +0100 Subject: [PATCH 09/12] feat: [#1445] add logs to the event listener's initialization --- packages/http-tracker-core/src/statistics/keeper.rs | 9 ++++++++- packages/udp-tracker-core/src/statistics/keeper.rs | 9 ++++++++- packages/udp-tracker-server/src/statistics/keeper.rs | 9 ++++++++- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/http-tracker-core/src/statistics/keeper.rs b/packages/http-tracker-core/src/statistics/keeper.rs index 01a7a1569..1b69f032d 100644 --- a/packages/http-tracker-core/src/statistics/keeper.rs +++ b/packages/http-tracker-core/src/statistics/keeper.rs @@ -3,6 +3,7 @@ use tokio::sync::broadcast::Receiver; use super::event::listener::dispatch_events; use super::repository::Repository; use crate::event::Event; +use crate::HTTP_TRACKER_LOG_TARGET; /// The service responsible for keeping tracker metrics (listening to statistics events and handle them). /// @@ -29,7 +30,13 @@ impl Keeper { pub fn run_event_listener(&mut self, receiver: Receiver) { let stats_repository = self.repository.clone(); - tokio::spawn(async move { dispatch_events(receiver, stats_repository).await }); + tracing::info!(target: HTTP_TRACKER_LOG_TARGET, "Starting HTTP tracker core event listener"); + + tokio::spawn(async move { + dispatch_events(receiver, stats_repository).await; + + tracing::info!(target: HTTP_TRACKER_LOG_TARGET, "HTTP tracker core event listener finished"); + }); } } diff --git a/packages/udp-tracker-core/src/statistics/keeper.rs b/packages/udp-tracker-core/src/statistics/keeper.rs index 16ea51aac..d72dcb260 100644 --- a/packages/udp-tracker-core/src/statistics/keeper.rs +++ b/packages/udp-tracker-core/src/statistics/keeper.rs @@ -3,6 +3,7 @@ use tokio::sync::broadcast::Receiver; use super::event::listener::dispatch_events; use super::repository::Repository; use crate::event::Event; +use crate::UDP_TRACKER_LOG_TARGET; /// The service responsible for keeping tracker metrics (listening to statistics events and handle them). /// @@ -29,7 +30,13 @@ impl Keeper { pub fn run_event_listener(&mut self, receiver: Receiver) { let stats_repository = self.repository.clone(); - tokio::spawn(async move { dispatch_events(receiver, stats_repository).await }); + tracing::info!(target: UDP_TRACKER_LOG_TARGET, "Starting UDP tracker core event listener"); + + tokio::spawn(async move { + dispatch_events(receiver, stats_repository).await; + + tracing::info!(target: UDP_TRACKER_LOG_TARGET, "UDP tracker core event listener finished"); + }); } } diff --git a/packages/udp-tracker-server/src/statistics/keeper.rs b/packages/udp-tracker-server/src/statistics/keeper.rs index 62216ce88..c200b4cdf 100644 --- a/packages/udp-tracker-server/src/statistics/keeper.rs +++ b/packages/udp-tracker-server/src/statistics/keeper.rs @@ -1,3 +1,4 @@ +use bittorrent_udp_tracker_core::UDP_TRACKER_LOG_TARGET; use tokio::sync::broadcast::Receiver; use super::event::listener::dispatch_events; @@ -29,7 +30,13 @@ impl Keeper { pub fn run_event_listener(&mut self, receiver: Receiver) { let stats_repository = self.repository.clone(); - tokio::spawn(async move { dispatch_events(receiver, stats_repository).await }); + tracing::info!(target: UDP_TRACKER_LOG_TARGET, "Starting UDP tracker server event listener"); + + tokio::spawn(async move { + dispatch_events(receiver, stats_repository).await; + + tracing::info!(target: UDP_TRACKER_LOG_TARGET, "UDP tracker core server listener finished"); + }); } } From 482a1be9f46234db628b260b148cf5102d83f53d Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 21 Apr 2025 10:24:50 +0100 Subject: [PATCH 10/12] feat: [#1445] add request kind label to total errors metric --- packages/metrics/src/label/value.rs | 6 ++++++ packages/udp-tracker-server/src/event/mod.rs | 1 + packages/udp-tracker-server/src/handlers/error.rs | 1 + .../src/statistics/event/handler.rs | 13 +++++++++++-- 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/metrics/src/label/value.rs b/packages/metrics/src/label/value.rs index 528a0e2ab..ffdbce333 100644 --- a/packages/metrics/src/label/value.rs +++ b/packages/metrics/src/label/value.rs @@ -25,6 +25,12 @@ impl PrometheusSerializable for LabelValue { } } +impl From for LabelValue { + fn from(value: String) -> Self { + Self(value) + } +} + #[cfg(test)] mod tests { use crate::label::value::LabelValue; diff --git a/packages/udp-tracker-server/src/event/mod.rs b/packages/udp-tracker-server/src/event/mod.rs index 0236b26a9..a1770acc0 100644 --- a/packages/udp-tracker-server/src/event/mod.rs +++ b/packages/udp-tracker-server/src/event/mod.rs @@ -31,6 +31,7 @@ pub enum Event { }, UdpError { context: ConnectionContext, + kind: Option, }, } diff --git a/packages/udp-tracker-server/src/handlers/error.rs b/packages/udp-tracker-server/src/handlers/error.rs index 6a1bce51c..9d9ee8b1d 100644 --- a/packages/udp-tracker-server/src/handlers/error.rs +++ b/packages/udp-tracker-server/src/handlers/error.rs @@ -64,6 +64,7 @@ pub async fn handle_error( udp_server_stats_event_sender .send_event(Event::UdpError { context: ConnectionContext::new(client_socket_addr, server_service_binding), + kind: req_kind, }) .await; } diff --git a/packages/udp-tracker-server/src/statistics/event/handler.rs b/packages/udp-tracker-server/src/statistics/event/handler.rs index 092ce93f2..22253852c 100644 --- a/packages/udp-tracker-server/src/statistics/event/handler.rs +++ b/packages/udp-tracker-server/src/statistics/event/handler.rs @@ -232,7 +232,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura Err(err) => tracing::error!("Failed to increase the counter: {}", err), }; } - Event::UdpError { context } => { + Event::UdpError { context, kind } => { // Global fixed metrics match context.client_socket_addr().ip() { std::net::IpAddr::V4(_) => { @@ -244,8 +244,15 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura } // Extendable metrics + + let mut label_set = LabelSet::from(context); + + if let Some(kind) = kind { + label_set.upsert(label_name!("request_kind"), kind.to_string().into()); + } + match stats_repository - .increase_counter(&metric_name!(UDP_TRACKER_SERVER_ERRORS_TOTAL), &LabelSet::from(context), now) + .increase_counter(&metric_name!(UDP_TRACKER_SERVER_ERRORS_TOTAL), &label_set, now) .await { Ok(()) => {} @@ -510,6 +517,7 @@ mod tests { ) .unwrap(), ), + kind: None, }, &stats_repository, CurrentClock::now(), @@ -641,6 +649,7 @@ mod tests { ) .unwrap(), ), + kind: None, }, &stats_repository, CurrentClock::now(), From ad782eb3dd66dbd2f125aed4bf99b61ec464ee8a Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 21 Apr 2025 10:25:26 +0100 Subject: [PATCH 11/12] refactor: [#1445] rename metric label --- packages/udp-tracker-server/src/statistics/event/handler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/udp-tracker-server/src/statistics/event/handler.rs b/packages/udp-tracker-server/src/statistics/event/handler.rs index 22253852c..1e1502339 100644 --- a/packages/udp-tracker-server/src/statistics/event/handler.rs +++ b/packages/udp-tracker-server/src/statistics/event/handler.rs @@ -109,7 +109,7 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura let mut label_set = LabelSet::from(context); - label_set.upsert(label_name!("kind"), LabelValue::new(&kind.to_string())); + label_set.upsert(label_name!("request_kind"), LabelValue::new(&kind.to_string())); match stats_repository .increase_counter(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL), &label_set, now) From f9ad729f2899ddd351506adb240757b73bbaa309 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 21 Apr 2025 10:49:13 +0100 Subject: [PATCH 12/12] feat: [#1445] add logs for metrics initialization --- Cargo.lock | 1 + packages/metrics/Cargo.toml | 1 + packages/metrics/src/lib.rs | 2 ++ packages/metrics/src/metric_collection.rs | 7 +++++-- packages/metrics/src/unit.rs | 1 + 5 files changed, 10 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e05894e3c..b72047f37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4791,6 +4791,7 @@ dependencies = [ "serde_json", "thiserror 2.0.12", "torrust-tracker-primitives", + "tracing", ] [[package]] diff --git a/packages/metrics/Cargo.toml b/packages/metrics/Cargo.toml index 6520cf244..0597785f4 100644 --- a/packages/metrics/Cargo.toml +++ b/packages/metrics/Cargo.toml @@ -21,6 +21,7 @@ serde = { version = "1", features = ["derive"] } serde_json = "1.0.140" thiserror = "2" torrust-tracker-primitives = { version = "3.0.0-develop", path = "../primitives" } +tracing = "0.1.41" [dev-dependencies] approx = "0.5.1" diff --git a/packages/metrics/src/lib.rs b/packages/metrics/src/lib.rs index fd677b891..95d70bf6c 100644 --- a/packages/metrics/src/lib.rs +++ b/packages/metrics/src/lib.rs @@ -8,6 +8,8 @@ pub mod sample; pub mod sample_collection; pub mod unit; +pub const METRICS_TARGET: &str = "METRICS"; + #[cfg(test)] mod tests { /// It removes leading and trailing whitespace from each line, and empty lines. diff --git a/packages/metrics/src/metric_collection.rs b/packages/metrics/src/metric_collection.rs index c719e6054..6a2a7735d 100644 --- a/packages/metrics/src/metric_collection.rs +++ b/packages/metrics/src/metric_collection.rs @@ -12,6 +12,7 @@ use super::prometheus::PrometheusSerializable; use crate::metric::description::MetricDescription; use crate::sample_collection::SampleCollection; use crate::unit::Unit; +use crate::METRICS_TARGET; // code-review: serialize in a deterministic order? For example: // - First the counter metrics ordered by name. @@ -56,7 +57,8 @@ impl MetricCollection { // Counter-specific methods - pub fn describe_counter(&mut self, name: &MetricName, _opt_unit: Option, _opt_description: Option) { + pub fn describe_counter(&mut self, name: &MetricName, opt_unit: Option, opt_description: Option) { + tracing::info!(target: METRICS_TARGET, type = "counter", name = name.to_string(), unit = ?opt_unit, description = ?opt_description); self.counters.ensure_metric_exists(name); } @@ -97,7 +99,8 @@ impl MetricCollection { // Gauge-specific methods - pub fn describe_gauge(&mut self, name: &MetricName, _opt_unit: Option, _opt_description: Option) { + pub fn describe_gauge(&mut self, name: &MetricName, opt_unit: Option, opt_description: Option) { + tracing::info!(target: METRICS_TARGET, type = "gauge", name = name.to_string(), unit = ?opt_unit, description = ?opt_description); self.gauges.ensure_metric_exists(name); } diff --git a/packages/metrics/src/unit.rs b/packages/metrics/src/unit.rs index b98e6836d..f7a528bed 100644 --- a/packages/metrics/src/unit.rs +++ b/packages/metrics/src/unit.rs @@ -4,6 +4,7 @@ //! The `Unit` enum is used to specify the unit of measurement for metrics. //! //! They were copied from the `metrics` crate, to allow future compatibility. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum Unit { Count, Percent,