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

Add Debug impls and fix clippy warnings #71

Merged
merged 4 commits into from Jul 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Expand Up @@ -118,7 +118,7 @@ jobs:
- uses: actions-rs/cargo@v1
with:
command: clippy
args: -- -D warnings
args: --workspace --all-targets -- -D warnings

check-rustdoc-links:
name: Check rustdoc intra-doc links
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -11,13 +11,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- Added a `with_prefix` method to `Registry` to allow initializing a registry with a prefix. See [PR 70].
- Added `Debug` implementations on most public types that were missing them. See [PR 71].

### Removed
- Remove `Add` trait implementation for a private type which lead to compile time conflicts with existing `Add` implementations e.g. on `String`. See [PR 69].

[PR 65]: https://github.com/prometheus/client_rust/pull/65
[PR 69]: https://github.com/prometheus/client_rust/pull/69
[PR 70]: https://github.com/prometheus/client_rust/pull/70
[PR 71]: https://github.com/prometheus/client_rust/pull/71

## [0.16.0]

Expand Down
5 changes: 2 additions & 3 deletions benches/encoding/text.rs
Expand Up @@ -7,7 +7,6 @@ use prometheus_client::metrics::family::Family;
use prometheus_client::metrics::histogram::{exponential_buckets, Histogram};
use prometheus_client::registry::Registry;
use std::io::Write;
use std::sync::atomic::AtomicU64;

pub fn text(c: &mut Criterion) {
c.bench_function("encode", |b| {
Expand All @@ -23,7 +22,7 @@ pub fn text(c: &mut Criterion) {
Get,
#[allow(dead_code)]
Put,
};
}

#[derive(Clone, Hash, PartialEq, Eq)]
enum Status {
Expand All @@ -32,7 +31,7 @@ pub fn text(c: &mut Criterion) {
Four,
#[allow(dead_code)]
Five,
};
}

impl Encode for Status {
fn encode(&self, writer: &mut dyn Write) -> Result<(), std::io::Error> {
Expand Down
6 changes: 3 additions & 3 deletions benches/family.rs
@@ -1,4 +1,4 @@
use criterion::{black_box, criterion_group, criterion_main, Criterion};
use criterion::{criterion_group, criterion_main, Criterion};
use prometheus_client::metrics::counter::Counter;
use prometheus_client::metrics::family::Family;

Expand Down Expand Up @@ -28,7 +28,7 @@ pub fn family(c: &mut Criterion) {
Get,
#[allow(dead_code)]
Put,
};
}

#[derive(Clone, Hash, PartialEq, Eq)]
enum Status {
Expand All @@ -37,7 +37,7 @@ pub fn family(c: &mut Criterion) {
Four,
#[allow(dead_code)]
Five,
};
}
let family = Family::<Labels, Counter>::default();

b.iter(|| {
Expand Down
16 changes: 8 additions & 8 deletions derive-text-encode/tests/lib.rs
Expand Up @@ -10,22 +10,22 @@ fn basic_flow() {
struct Labels {
method: Method,
path: String,
};
}

#[derive(Clone, Hash, PartialEq, Eq, Encode)]
enum Method {
GET,
Get,
#[allow(dead_code)]
PUT,
};
Put,
}

let family = Family::<Labels, Counter>::default();
registry.register("my_counter", "This is my counter", family.clone());

// Record a single HTTP GET request.
family
.get_or_create(&Labels {
method: Method::GET,
method: Method::Get,
path: "/metrics".to_string(),
})
.inc();
Expand All @@ -36,7 +36,7 @@ fn basic_flow() {

let expected = "# HELP my_counter This is my counter.\n".to_owned()
+ "# TYPE my_counter counter\n"
+ "my_counter_total{method=\"GET\",path=\"/metrics\"} 1\n"
+ "my_counter_total{method=\"Get\",path=\"/metrics\"} 1\n"
+ "# EOF\n";
assert_eq!(expected, String::from_utf8(buffer).unwrap());
}
Expand All @@ -52,13 +52,13 @@ fn remap_keyword_identifiers() {
// Test makes sure `r#type` is replaced by `type` in the OpenMetrics
// output.
r#type: u64,
};
}

let labels = Labels { r#type: 42 };

let mut buffer = vec![];

labels.encode(&mut buffer);
labels.encode(&mut buffer).unwrap();

assert_eq!(
"type=\"42\"".to_string(),
Expand Down
10 changes: 7 additions & 3 deletions src/encoding/text.rs
Expand Up @@ -225,6 +225,7 @@ impl Encode for () {
// objects can not use type parameters.
//
// TODO: Alternative solutions to the above are very much appreciated.
#[allow(missing_debug_implementations)]
pub struct Encoder<'a, 'b> {
writer: &'a mut dyn Write,
name: &'a str,
Expand Down Expand Up @@ -303,6 +304,7 @@ impl<'a, 'b> Encoder<'a, 'b> {
}
}

#[allow(missing_debug_implementations)]
#[must_use]
pub struct BucketEncoder<'a> {
writer: &'a mut dyn Write,
Expand Down Expand Up @@ -342,6 +344,7 @@ impl<'a> BucketEncoder<'a> {
}
}

#[allow(missing_debug_implementations)]
#[must_use]
pub struct ValueEncoder<'a> {
writer: &'a mut dyn Write,
Expand All @@ -359,6 +362,7 @@ impl<'a> ValueEncoder<'a> {
}
}

#[allow(missing_debug_implementations)]
#[must_use]
pub struct ExemplarEncoder<'a> {
writer: &'a mut dyn Write,
Expand Down Expand Up @@ -616,7 +620,7 @@ mod tests {
fn encode_counter() {
let counter: Counter = Counter::default();
let mut registry = Registry::default();
registry.register("my_counter", "My counter", counter.clone());
registry.register("my_counter", "My counter", counter);
Victor-N-Suadicani marked this conversation as resolved.
Show resolved Hide resolved

let mut encoded = Vec::new();

Expand All @@ -629,7 +633,7 @@ mod tests {
fn encode_counter_with_unit() {
let mut registry = Registry::default();
let counter: Counter = Counter::default();
registry.register_with_unit("my_counter", "My counter", Unit::Seconds, counter.clone());
registry.register_with_unit("my_counter", "My counter", Unit::Seconds, counter);

let mut encoded = Vec::new();
encode(&mut encoded, &registry).unwrap();
Expand Down Expand Up @@ -677,7 +681,7 @@ mod tests {
fn encode_gauge() {
let mut registry = Registry::default();
let gauge: Gauge = Gauge::default();
registry.register("my_gauge", "My gauge", gauge.clone());
registry.register("my_gauge", "My gauge", gauge);

let mut encoded = Vec::new();

Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
@@ -1,6 +1,7 @@
#![forbid(unsafe_code)]
#![deny(unused)]
#![deny(dead_code)]
#![warn(missing_debug_implementations)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit undecided. I don't think we should be adding Debug implementations to types by default as it slows compile time and bloats the compiled binary.

Do you feel strongly about this? What do you think of removing this line but keeping the new derive statements below?

Suggested change
#![warn(missing_debug_implementations)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only lints for public types I believe. So it's not every type.

But I don't feel super strongly about it. Though I think it is a good idea.

Your call :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only lints for public types I believe. So it's not every type.

Good point. Thanks. Let's keep it as is then.


//! Client library implementation of the [Open Metrics
//! specification](https://github.com/OpenObservability/OpenMetrics). Allows
Expand Down
2 changes: 1 addition & 1 deletion src/metrics.rs
Expand Up @@ -12,7 +12,7 @@ pub trait TypedMetric {
const TYPE: MetricType = MetricType::Unknown;
}

#[derive(Clone, Copy)]
#[derive(Clone, Copy, Debug)]
pub enum MetricType {
Counter,
Gauge,
Expand Down
2 changes: 2 additions & 0 deletions src/metrics/counter.rs
Expand Up @@ -39,12 +39,14 @@ use std::sync::Arc;
/// let _value: f64 = counter.get();
/// ```
#[cfg(not(any(target_arch = "mips", target_arch = "powerpc")))]
#[derive(Debug)]
pub struct Counter<N = u64, A = AtomicU64> {
value: Arc<A>,
phantom: PhantomData<N>,
}

#[cfg(any(target_arch = "mips", target_arch = "powerpc"))]
#[derive(Debug)]
pub struct Counter<N = u32, A = AtomicU32> {
value: Arc<A>,
phantom: PhantomData<N>,
Expand Down
6 changes: 6 additions & 0 deletions src/metrics/exemplar.rs
Expand Up @@ -12,6 +12,7 @@ use std::sync::atomic::AtomicU32;
use std::sync::atomic::AtomicU64;
use std::sync::{Arc, RwLock, RwLockReadGuard};

#[derive(Debug)]
pub struct Exemplar<S, V> {
pub(crate) label_set: S,
pub(crate) value: V,
Expand All @@ -30,11 +31,13 @@ pub struct Exemplar<S, V> {
/// let _value: (u64, _) = counter_with_exemplar.get();
/// ```
#[cfg(not(any(target_arch = "mips", target_arch = "powerpc")))]
#[derive(Debug)]
pub struct CounterWithExemplar<S, N = u64, A = AtomicU64> {
pub(crate) inner: Arc<RwLock<CounterWithExemplarInner<S, N, A>>>,
}

#[cfg(any(target_arch = "mips", target_arch = "powerpc"))]
#[derive(Debug)]
pub struct CounterWithExemplar<S, N = u32, A = AtomicU32> {
pub(crate) inner: Arc<RwLock<CounterWithExemplarInner<S, N, A>>>,
}
Expand All @@ -47,6 +50,7 @@ impl<S, N, A> Clone for CounterWithExemplar<S, N, A> {
}
}

#[derive(Debug)]
pub struct CounterWithExemplarInner<S, N, A> {
pub(crate) exemplar: Option<Exemplar<S, N>>,
pub(crate) counter: Counter<N, A>,
Expand Down Expand Up @@ -118,6 +122,7 @@ type RwLockGuardedCounterWithExemplar<'a, S, N, A> =
/// let histogram = HistogramWithExemplars::new(exponential_buckets(1.0, 2.0, 10));
/// histogram.observe(4.2, Some(vec![("user_id".to_string(), "42".to_string())]));
/// ```
#[derive(Debug)]
pub struct HistogramWithExemplars<S> {
// TODO: Not ideal, as Histogram has a Mutex as well.
pub(crate) inner: Arc<RwLock<HistogramWithExemplarsInner<S>>>,
Expand All @@ -131,6 +136,7 @@ impl<S> Clone for HistogramWithExemplars<S> {
}
}

#[derive(Debug)]
pub struct HistogramWithExemplarsInner<S> {
pub(crate) exemplars: HashMap<usize, Exemplar<S, f64>>,
pub(crate) histogram: Histogram,
Expand Down
1 change: 1 addition & 0 deletions src/metrics/family.rs
Expand Up @@ -97,6 +97,7 @@ use std::sync::{Arc, RwLock, RwLockReadGuard};
/// # assert_eq!(expected, String::from_utf8(buffer).unwrap());
/// ```
// TODO: Consider exposing hash algorithm.
#[derive(Debug)]
pub struct Family<S, M, C = fn() -> M> {
metrics: Arc<RwLock<HashMap<S, M>>>,
/// Function that when called constructs a new metric.
Expand Down
2 changes: 2 additions & 0 deletions src/metrics/gauge.rs
Expand Up @@ -39,12 +39,14 @@ use std::sync::Arc;
/// let _value: f64 = gauge.get();
/// ```
#[cfg(not(any(target_arch = "mips", target_arch = "powerpc")))]
#[derive(Debug)]
pub struct Gauge<N = u64, A = AtomicU64> {
value: Arc<A>,
phantom: PhantomData<N>,
}

#[cfg(any(target_arch = "mips", target_arch = "powerpc"))]
#[derive(Debug)]
pub struct Gauge<N = u32, A = AtomicU32> {
value: Arc<A>,
phantom: PhantomData<N>,
Expand Down
2 changes: 2 additions & 0 deletions src/metrics/histogram.rs
Expand Up @@ -31,6 +31,7 @@ use std::sync::{Arc, Mutex, MutexGuard};
/// ```
// TODO: Consider using atomics. See
// https://github.com/tikv/rust-prometheus/pull/314.
#[derive(Debug)]
pub struct Histogram {
inner: Arc<Mutex<Inner>>,
}
Expand All @@ -43,6 +44,7 @@ impl Clone for Histogram {
}
}

#[derive(Debug)]
pub(crate) struct Inner {
// TODO: Consider allowing integer observe values.
sum: f64,
Expand Down
1 change: 1 addition & 0 deletions src/metrics/info.rs
Expand Up @@ -12,6 +12,7 @@ use crate::metrics::{MetricType, TypedMetric};
///
/// let _info = Info::new(vec![("os", "GNU/linux")]);
/// ```
#[derive(Debug)]
pub struct Info<S>(pub(crate) S);

impl<S> Info<S> {
Expand Down
8 changes: 6 additions & 2 deletions src/registry.rs
Expand Up @@ -57,6 +57,7 @@ use std::borrow::Cow;
/// # "# EOF\n";
/// # assert_eq!(expected, String::from_utf8(buffer).unwrap());
/// ```
#[derive(Debug)]
pub struct Registry<M = Box<dyn crate::encoding::text::SendSyncEncodeMetric>> {
prefix: Option<Prefix>,
labels: Vec<(Cow<'static, str>, Cow<'static, str>)>,
Expand Down Expand Up @@ -250,6 +251,7 @@ impl<M> Registry<M> {

/// Iterator iterating both the metrics registered directly with the registry as
/// well as all metrics registered with sub-registries.
#[derive(Debug)]
pub struct RegistryIterator<'a, M> {
metrics: std::slice::Iter<'a, (Descriptor, M)>,
sub_registries: std::slice::Iter<'a, Registry<M>>,
Expand Down Expand Up @@ -280,7 +282,7 @@ impl<'a, M> Iterator for RegistryIterator<'a, M> {
}
}

#[derive(Clone)]
#[derive(Clone, Debug)]
struct Prefix(String);

impl From<String> for Prefix {
Expand All @@ -295,6 +297,7 @@ impl From<Prefix> for String {
}
}

#[derive(Debug)]
pub struct Descriptor {
name: String,
help: String,
Expand Down Expand Up @@ -323,6 +326,7 @@ impl Descriptor {
/// Metric units recommended by Open Metrics.
///
/// See [`Unit::Other`] to specify alternative units.
#[derive(Debug)]
pub enum Unit {
Amperes,
Bytes,
Expand All @@ -345,7 +349,7 @@ mod tests {
fn register_and_iterate() {
let mut registry: Registry<Counter> = Registry::default();
let counter = Counter::default();
registry.register("my_counter", "My counter", counter.clone());
registry.register("my_counter", "My counter", counter);

assert_eq!(1, registry.iter().count())
}
Expand Down