Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace HashMap with BTreeMap to sort metrics deterministically #128

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/actix-web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ use prometheus_client::metrics::counter::Counter;
use prometheus_client::metrics::family::Family;
use prometheus_client::registry::Registry;

#[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelValue)]
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelValue)]
pub enum Method {
Get,
Post,
}

#[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelSet)]
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet)]
pub struct MethodLabels {
pub method: Method,
}
Expand Down
4 changes: 2 additions & 2 deletions examples/tide.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ async fn main() -> std::result::Result<(), std::io::Error> {
Ok(())
}

#[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelSet)]
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet)]
struct Labels {
method: Method,
path: String,
}

#[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelValue)]
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelValue)]
enum Method {
Get,
Put,
Expand Down
49 changes: 49 additions & 0 deletions src/encoding/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -751,4 +751,53 @@ def parse(input):
.unwrap();
})
}

#[test]
fn metrics_are_sorted_in_registration_order() {
let mut registry = Registry::default();
let counter: Counter = Counter::default();
let another_counter: Counter = Counter::default();
registry.register("my_counter", "My counter", counter);
registry.register("another_counter", "Another counter", another_counter);

let mut encoded = String::new();
encode(&mut encoded, &registry).unwrap();

let expected = "# HELP my_counter My counter.\n".to_owned()
+ "# TYPE my_counter counter\n"
+ "my_counter_total 0\n"
+ "# HELP another_counter Another counter.\n"
+ "# TYPE another_counter counter\n"
+ "another_counter_total 0\n"
+ "# EOF\n";
assert_eq!(expected, encoded);
}

#[test]
fn metric_family_is_sorted_lexicographically() {
let mut registry = Registry::default();
let gauge = Family::<Vec<(String, String)>, Gauge>::default();
registry.register("my_gauge", "My gauge", gauge.clone());

gauge
.get_or_create(&vec![("label".to_string(), "0".to_string())])
.set(0);
gauge
.get_or_create(&vec![("label".to_string(), "2".to_string())])
.set(2);
gauge
.get_or_create(&vec![("label".to_string(), "1".to_string())])
.set(1);

let mut encoded = String::new();
encode(&mut encoded, &registry).unwrap();

let expected = "# HELP my_gauge My gauge.\n".to_owned()
+ "# TYPE my_gauge gauge\n"
+ "my_gauge{label=\"0\"} 0\n"
+ "my_gauge{label=\"1\"} 1\n"
+ "my_gauge{label=\"2\"} 2\n"
+ "# EOF\n";
assert_eq!(expected, encoded);
}
}
4 changes: 2 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@
//! //
//! // You could as well use `(String, String)` to represent a label set,
//! // instead of the custom type below.
//! #[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelSet)]
//! #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet)]
//! struct Labels {
//! // Use your own enum types to represent label values.
//! method: Method,
//! // Or just a plain string.
//! path: String,
//! };
//!
//! #[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelValue)]
//! #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelValue)]
//! enum Method {
//! GET,
//! PUT,
Expand Down
8 changes: 4 additions & 4 deletions src/metrics/exemplar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ pub struct Exemplar<S, V> {
/// # use prometheus_client::metrics::histogram::exponential_buckets;
/// # use prometheus_client::metrics::family::Family;
/// # use prometheus_client_derive_encode::EncodeLabelSet;
/// #[derive(Clone, Hash, PartialEq, Eq, EncodeLabelSet, Debug, Default)]
/// #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet, Debug, Default)]
/// pub struct ResultLabel {
/// pub result: String,
/// }
///
/// #[derive(Clone, Hash, PartialEq, Eq, EncodeLabelSet, Debug, Default)]
/// #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet, Debug, Default)]
/// pub struct TraceLabel {
/// pub trace_id: String,
/// }
Expand Down Expand Up @@ -183,12 +183,12 @@ where
/// # use prometheus_client::metrics::histogram::exponential_buckets;
/// # use prometheus_client::metrics::family::Family;
/// # use prometheus_client::encoding::EncodeLabelSet;
/// #[derive(Clone, Hash, PartialEq, Eq, EncodeLabelSet, Debug, Default)]
/// #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet, Debug, Default)]
/// pub struct ResultLabel {
/// pub result: String,
/// }
///
/// #[derive(Clone, Hash, PartialEq, Eq, EncodeLabelSet, Debug, Default)]
/// #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet, Debug, Default)]
/// pub struct TraceLabel {
/// pub trace_id: String,
/// }
Expand Down
19 changes: 9 additions & 10 deletions src/metrics/family.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::encoding::{EncodeLabelSet, EncodeMetric, MetricEncoder};
use super::{MetricType, TypedMetric};
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard, RwLockWriteGuard};
use std::cell::RefCell;
use std::collections::HashMap;
use std::collections::BTreeMap;
use std::sync::Arc;

/// Representation of the OpenMetrics *MetricFamily* data type.
Expand Down Expand Up @@ -69,12 +69,12 @@ use std::sync::Arc;
/// # use std::io::Write;
/// #
/// # let mut registry = Registry::default();
/// #[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelSet)]
/// #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelSet)]
/// struct Labels {
/// method: Method,
/// };
///
/// #[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelValue)]
/// #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, EncodeLabelValue)]
/// enum Method {
/// GET,
/// PUT,
Expand All @@ -100,9 +100,8 @@ use std::sync::Arc;
/// # "# EOF\n";
/// # assert_eq!(expected, buffer);
/// ```
// TODO: Consider exposing hash algorithm.
pub struct Family<S, M, C = fn() -> M> {
metrics: Arc<RwLock<HashMap<S, M>>>,
metrics: Arc<RwLock<BTreeMap<S, M>>>,
/// Function that when called constructs a new metric.
///
/// For most metric types this would simply be its [`Default`]
Expand Down Expand Up @@ -169,7 +168,7 @@ impl<M, F: Fn() -> M> MetricConstructor<M> for F {
}
}

impl<S: Clone + std::hash::Hash + Eq, M: Default> Default for Family<S, M> {
impl<S: Clone + Eq + Ord, M: Default> Default for Family<S, M> {
fn default() -> Self {
Self {
metrics: Arc::new(RwLock::new(Default::default())),
Expand All @@ -178,7 +177,7 @@ impl<S: Clone + std::hash::Hash + Eq, M: Default> Default for Family<S, M> {
}
}

impl<S: Clone + std::hash::Hash + Eq, M, C> Family<S, M, C> {
impl<S: Clone + Eq + Ord, M, C> Family<S, M, C> {
/// Create a metric family using a custom constructor to construct new
/// metrics.
///
Expand Down Expand Up @@ -208,7 +207,7 @@ impl<S: Clone + std::hash::Hash + Eq, M, C> Family<S, M, C> {
}
}

impl<S: Clone + std::hash::Hash + Eq, M, C: MetricConstructor<M>> Family<S, M, C> {
impl<S: Clone + Eq + Ord, M, C: MetricConstructor<M>> Family<S, M, C> {
/// Access a metric with the given label set, creating it if one does not
/// yet exist.
///
Expand Down Expand Up @@ -289,7 +288,7 @@ impl<S: Clone + std::hash::Hash + Eq, M, C: MetricConstructor<M>> Family<S, M, C
self.metrics.write().clear()
}

pub(crate) fn read(&self) -> RwLockReadGuard<HashMap<S, M>> {
pub(crate) fn read(&self) -> RwLockReadGuard<BTreeMap<S, M>> {
self.metrics.read()
}
}
Expand All @@ -309,7 +308,7 @@ impl<S, M: TypedMetric, C> TypedMetric for Family<S, M, C> {

impl<S, M, C> EncodeMetric for Family<S, M, C>
where
S: Clone + std::hash::Hash + Eq + EncodeLabelSet,
S: Clone + Eq + Ord + EncodeLabelSet,
M: EncodeMetric + TypedMetric,
C: MetricConstructor<M>,
{
Expand Down