Skip to content

Commit 34a6635

Browse files
committed
Merge #1577: Overhaul stats: Refactor metrics, use label metrics to calculate global metrics
dcfb5d5 refactor: [#1446] Calculate global metrics from labeled metrics (Jose Celano) 3f5216e fix: [#1446] clippy error (Jose Celano) 96bae36 feat: [#1446] add new metric label server_binding_address_ip_type (Jose Celano) 1376a7c refactor: [#1446] rename AddressType to IpType (Jose Celano) 476ece4 refactor: [#1446] WIP. Calculate global metrics from labeled metrics (Jose Celano) 0d13439 test: [#1446] add more tests to metrics package (Jose Celano) 4da4f83 refactor: [#1446] rename vars (Jose Celano) 64be847 feat: [#1446] add aggregate function sum to metric collection (Jose Celano) Pull request description: The idea is to continue exposing the `stats` endpoint but removing the internal code to calculate them. Instead, we calculate the metrics from the labeled metrics. That will **simplify** the event listeners and **avoid duplicate code** to calculate the same data. ### Endpoints for statistics `stats` (simple predefined global metrics) - http://localhost:1212/api/v1/stats?token=MyAccessToken&format=json - http://localhost:1212/api/v1/stats?token=MyAccessToken&format=prometheus `metrics` (extendable-labelled metrics) - http://localhost:1212/api/v1/metrics?token=MyAccessToken&format=json - http://localhost:1212/api/v1/metrics?token=MyAccessToken&format=prometheus ### Subtasks - [x] Add a new method to `MetricCollection` to sum the metric's samples. - [x] Review test coverage. - [x] Use the new method to calculate metrics in the `stats` endpoint. ### After merging this PR - Deploy to the tracker demo and verify that both Grafana dashboards appear identical. - Delete unused code to calculate the metrics in the `stats` endpoint directly in the event listeners. - Redeploy to the tracker demo and re-verify. **NOTICE:** We have two Grafana dashboards, one collecting data from the `stats` endpoint and another collecting data from `metrics`, and they look identical now. If there is a bug in the way we calculate aggregate values in the `MetricCollection`, we should see different graphs in the dashboards. Both dashboards (built from stats on the left, built from metrics on the right): ![image](https://github.com/user-attachments/assets/638b35af-ca46-4e21-b601-780588a3ba1e) **NOTICE:** Also, the new methods will be helpful for external users once we publish the metrics crate. Consumers of the Torrust Tracker API (`metrics` endpoint) will be able to deserialize and calculate aggregate values from the response. ACKs for top commit: josecelano: ACK dcfb5d5 Tree-SHA512: 3de4e41817b4b3ceeb5c501ed0bbe9d47c4ab7feb06fe3b5c472c4482b221d7ec360995bf381f0e519c5856d83b2aafe92fce22b53ea3e60517db26ac3a3a300
2 parents c3dffa5 + dcfb5d5 commit 34a6635

File tree

31 files changed

+1577
-57
lines changed

31 files changed

+1577
-57
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/http-tracker-core/src/event.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,12 @@ impl From<ConnectionContext> for LabelSet {
8787
LabelValue::new(&connection_context.server.service_binding.bind_address().ip().to_string()),
8888
),
8989
(
90-
label_name!("server_binding_address_type"),
91-
LabelValue::new(&connection_context.server.service_binding.bind_address_type().to_string()),
90+
label_name!("server_binding_address_ip_type"),
91+
LabelValue::new(&connection_context.server.service_binding.bind_address_ip_type().to_string()),
92+
),
93+
(
94+
label_name!("server_binding_address_ip_family"),
95+
LabelValue::new(&connection_context.server.service_binding.bind_address_ip_family().to_string()),
9296
),
9397
(
9498
label_name!("server_binding_port"),

packages/http-tracker-core/src/statistics/event/handler.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,12 @@ pub async fn handle_event(event: Event, stats_repository: &Arc<Repository>, now:
3232
.increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now)
3333
.await
3434
{
35-
Ok(()) => {}
35+
Ok(()) => {
36+
tracing::debug!(
37+
"Successfully increased the counter for HTTP announce requests received: {}",
38+
label_set
39+
);
40+
}
3641
Err(err) => tracing::error!("Failed to increase the counter: {}", err),
3742
};
3843
}
@@ -57,7 +62,12 @@ pub async fn handle_event(event: Event, stats_repository: &Arc<Repository>, now:
5762
.increase_counter(&metric_name!(HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL), &label_set, now)
5863
.await
5964
{
60-
Ok(()) => {}
65+
Ok(()) => {
66+
tracing::debug!(
67+
"Successfully increased the counter for HTTP scrape requests received: {}",
68+
label_set
69+
);
70+
}
6171
Err(err) => tracing::error!("Failed to increase the counter: {}", err),
6272
};
6373
}

packages/http-tracker-core/src/statistics/metrics.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ impl Metrics {
3333
labels: &LabelSet,
3434
now: DurationSinceUnixEpoch,
3535
) -> Result<(), Error> {
36-
self.metric_collection.increase_counter(metric_name, labels, now)
36+
self.metric_collection.increment_counter(metric_name, labels, now)
3737
}
3838

3939
/// # Errors

packages/http-tracker-core/src/statistics/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use torrust_tracker_metrics::metric::description::MetricDescription;
88
use torrust_tracker_metrics::metric_name;
99
use torrust_tracker_metrics::unit::Unit;
1010

11-
const HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL: &str = "http_tracker_core_requests_received_total";
11+
pub const HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL: &str = "http_tracker_core_requests_received_total";
1212

1313
#[must_use]
1414
pub fn describe_metrics() -> Metrics {

packages/metrics/.gitignore

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
./.coverage
1+
.coverage

packages/metrics/README.md

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,28 @@ A library with the metrics types used by the [Torrust Tracker](https://github.co
66

77
[Crate documentation](https://docs.rs/torrust-tracker-metrics).
88

9+
## Testing
10+
11+
Run coverage report:
12+
13+
```console
14+
cargo llvm-cov --package torrust-tracker-metrics
15+
```
16+
17+
Generate LCOV report with `llvm-cov` (for Visual Studio Code extension):
18+
19+
```console
20+
mkdir -p ./.coverage
21+
cargo llvm-cov --package torrust-tracker-metrics --lcov --output-path=./.coverage/lcov.info
22+
```
23+
24+
Generate HTML report with `llvm-cov`:
25+
26+
```console
27+
mkdir -p ./.coverage
28+
cargo llvm-cov --package torrust-tracker-metrics --html --output-dir ./.coverage
29+
```
30+
931
## Acknowledgements
1032

1133
We copied some parts like units or function names and signatures from the crate [metrics](https://crates.io/crates/metrics) because we wanted to make it compatible as much as possible with it. In the future, we may consider using the `metrics` crate directly instead of maintaining our own version.

packages/metrics/cSpell.json

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
{
2+
"words": [
3+
"cloneable",
4+
"formatjson",
5+
"Gibibytes",
6+
"Kibibytes",
7+
"Mebibytes",
8+
"ñaca",
9+
"rstest",
10+
"subsec",
11+
"Tebibytes",
12+
"thiserror"
13+
],
14+
"enableFiletypes": [
15+
"dockerfile",
16+
"shellscript",
17+
"toml"
18+
]
19+
}

packages/metrics/src/aggregate.rs

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
use derive_more::Display;
2+
3+
#[derive(Debug, Display, Clone, Copy, PartialEq, Default)]
4+
pub struct AggregateValue(f64);
5+
6+
impl AggregateValue {
7+
#[must_use]
8+
pub fn new(value: f64) -> Self {
9+
Self(value)
10+
}
11+
12+
#[must_use]
13+
pub fn value(&self) -> f64 {
14+
self.0
15+
}
16+
}
17+
18+
impl From<f64> for AggregateValue {
19+
fn from(value: f64) -> Self {
20+
Self(value)
21+
}
22+
}
23+
24+
impl From<AggregateValue> for f64 {
25+
fn from(value: AggregateValue) -> Self {
26+
value.0
27+
}
28+
}
29+
30+
#[cfg(test)]
31+
mod tests {
32+
use approx::assert_relative_eq;
33+
34+
use super::*;
35+
36+
#[test]
37+
fn it_should_be_created_with_new() {
38+
let value = AggregateValue::new(42.5);
39+
assert_relative_eq!(value.value(), 42.5);
40+
}
41+
42+
#[test]
43+
fn it_should_return_the_inner_value() {
44+
let value = AggregateValue::new(123.456);
45+
assert_relative_eq!(value.value(), 123.456);
46+
}
47+
48+
#[test]
49+
fn it_should_handle_zero_value() {
50+
let value = AggregateValue::new(0.0);
51+
assert_relative_eq!(value.value(), 0.0);
52+
}
53+
54+
#[test]
55+
fn it_should_handle_negative_values() {
56+
let value = AggregateValue::new(-42.5);
57+
assert_relative_eq!(value.value(), -42.5);
58+
}
59+
60+
#[test]
61+
fn it_should_handle_infinity() {
62+
let value = AggregateValue::new(f64::INFINITY);
63+
assert_relative_eq!(value.value(), f64::INFINITY);
64+
}
65+
66+
#[test]
67+
fn it_should_handle_nan() {
68+
let value = AggregateValue::new(f64::NAN);
69+
assert!(value.value().is_nan());
70+
}
71+
72+
#[test]
73+
fn it_should_be_created_from_f64() {
74+
let value: AggregateValue = 42.5.into();
75+
assert_relative_eq!(value.value(), 42.5);
76+
}
77+
78+
#[test]
79+
fn it_should_convert_to_f64() {
80+
let value = AggregateValue::new(42.5);
81+
let f64_value: f64 = value.into();
82+
assert_relative_eq!(f64_value, 42.5);
83+
}
84+
85+
#[test]
86+
fn it_should_be_displayable() {
87+
let value = AggregateValue::new(42.5);
88+
assert_eq!(value.to_string(), "42.5");
89+
}
90+
91+
#[test]
92+
fn it_should_be_debuggable() {
93+
let value = AggregateValue::new(42.5);
94+
let debug_string = format!("{value:?}");
95+
assert_eq!(debug_string, "AggregateValue(42.5)");
96+
}
97+
98+
#[test]
99+
fn it_should_be_cloneable() {
100+
let value = AggregateValue::new(42.5);
101+
let cloned_value = value;
102+
assert_eq!(value, cloned_value);
103+
}
104+
105+
#[test]
106+
fn it_should_be_copyable() {
107+
let value = AggregateValue::new(42.5);
108+
let copied_value = value;
109+
assert_eq!(value, copied_value);
110+
}
111+
112+
#[test]
113+
fn it_should_support_equality_comparison() {
114+
let value1 = AggregateValue::new(42.5);
115+
let value2 = AggregateValue::new(42.5);
116+
let value3 = AggregateValue::new(43.0);
117+
118+
assert_eq!(value1, value2);
119+
assert_ne!(value1, value3);
120+
}
121+
122+
#[test]
123+
fn it_should_handle_special_float_values_in_equality() {
124+
let nan1 = AggregateValue::new(f64::NAN);
125+
let nan2 = AggregateValue::new(f64::NAN);
126+
let infinity = AggregateValue::new(f64::INFINITY);
127+
let neg_infinity = AggregateValue::new(f64::NEG_INFINITY);
128+
129+
// NaN is not equal to itself in IEEE 754
130+
assert_ne!(nan1, nan2);
131+
assert_eq!(infinity, AggregateValue::new(f64::INFINITY));
132+
assert_eq!(neg_infinity, AggregateValue::new(f64::NEG_INFINITY));
133+
assert_ne!(infinity, neg_infinity);
134+
}
135+
136+
#[test]
137+
fn it_should_handle_conversion_roundtrip() {
138+
let original_value = 42.5;
139+
let aggregate_value = AggregateValue::from(original_value);
140+
let converted_back: f64 = aggregate_value.into();
141+
assert_relative_eq!(original_value, converted_back);
142+
}
143+
}

0 commit comments

Comments
 (0)