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

*: Move to parking_lot and thus make owning_ref obsolete #78

Merged
merged 2 commits into from
Aug 16, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.18.0] - unreleased

### Changed

- Use `parking_lot` instead of `std::sync::*`.

Before `proemtheus-client` would use the `owning_ref` crate to map the target
of a `std::sync::RwLockReadGuard`. `owning_ref` has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing `owning_ref` with a similar crate, we switch to locking via
`parking_lot` which supports the above mapping natively.

## [0.17.0]

### Changed
Expand Down
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "prometheus-client"
version = "0.17.0"
version = "0.18.0"
authors = ["Max Inden <mail@max-inden.de>"]
edition = "2021"
description = "Open Metrics client library allowing users to natively instrument applications."
Expand All @@ -16,7 +16,7 @@ members = ["derive-text-encode"]
[dependencies]
dtoa = "1.0"
itoa = "1.0"
owning_ref = "0.4"
parking_lot = "0.12"
prometheus-client-derive-text-encode = { version = "0.3.0", path = "derive-text-encode" }

[dev-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion src/encoding/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ where
{
fn encode(&self, encoder: Encoder) -> Result<(), std::io::Error> {
let (value, exemplar) = self.get();
encode_counter_with_maybe_exemplar(value, exemplar.as_ref().as_ref(), encoder)
encode_counter_with_maybe_exemplar(value, exemplar.as_ref(), encoder)
}

fn metric_type(&self) -> MetricType {
Expand Down
24 changes: 10 additions & 14 deletions src/metrics/exemplar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@

use super::counter::{self, Counter};
use super::histogram::Histogram;
use owning_ref::OwningRef;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
use std::collections::HashMap;
#[cfg(any(target_arch = "mips", target_arch = "powerpc"))]
use std::sync::atomic::AtomicU32;
#[cfg(not(any(target_arch = "mips", target_arch = "powerpc")))]
use std::sync::atomic::AtomicU64;
use std::sync::{Arc, RwLock, RwLockReadGuard};
use std::sync::Arc;

#[derive(Debug)]
pub struct Exemplar<S, V> {
Expand Down Expand Up @@ -74,7 +74,7 @@ impl<S, N: Clone, A: counter::Atomic<N>> CounterWithExemplar<S, N, A> {
/// Increase the [`CounterWithExemplar`] by `v`, updating the [`Exemplar`]
/// if a label set is provided, returning the previous value.
pub fn inc_by(&self, v: N, label_set: Option<S>) -> N {
let mut inner = self.inner.write().expect("Lock not to be poisoned.");
let mut inner = self.inner.write();

inner.exemplar = label_set.map(|label_set| Exemplar {
label_set,
Expand All @@ -86,10 +86,10 @@ impl<S, N: Clone, A: counter::Atomic<N>> CounterWithExemplar<S, N, A> {

/// Get the current value of the [`CounterWithExemplar`] as well as its
/// [`Exemplar`] if any.
pub fn get(&self) -> (N, RwLockGuardedCounterWithExemplar<S, N, A>) {
let inner = self.inner.read().expect("Lock not to be poisoned.");
pub fn get(&self) -> (N, MappedRwLockReadGuard<Option<Exemplar<S, N>>>) {
let inner = self.inner.read();
let value = inner.counter.get();
let exemplar = OwningRef::new(inner).map(|inner| &inner.exemplar);
let exemplar = RwLockReadGuard::map(inner, |inner| &inner.exemplar);
(value, exemplar)
}

Expand All @@ -101,15 +101,11 @@ impl<S, N: Clone, A: counter::Atomic<N>> CounterWithExemplar<S, N, A> {
/// The caller of this function has to uphold the property of an Open
/// Metrics counter namely that the value is monotonically increasing, i.e.
/// either stays the same or increases.
pub fn inner(&self) -> OwningRef<RwLockReadGuard<CounterWithExemplarInner<S, N, A>>, A> {
OwningRef::new(self.inner.read().expect("Lock not to be poisoned."))
.map(|inner| inner.counter.inner())
pub fn inner(&self) -> MappedRwLockReadGuard<A> {
RwLockReadGuard::map(self.inner.read(), |inner| inner.counter.inner())
}
}

type RwLockGuardedCounterWithExemplar<'a, S, N, A> =
OwningRef<RwLockReadGuard<'a, CounterWithExemplarInner<S, N, A>>, Option<Exemplar<S, N>>>;

/////////////////////////////////////////////////////////////////////////////////
// Histogram

Expand Down Expand Up @@ -153,7 +149,7 @@ impl<S> HistogramWithExemplars<S> {
}

pub fn observe(&self, v: f64, label_set: Option<S>) {
let mut inner = self.inner.write().expect("Lock not to be poisoned.");
let mut inner = self.inner.write();
let bucket = inner.histogram.observe_and_bucket(v);
if let (Some(bucket), Some(label_set)) = (bucket, label_set) {
inner.exemplars.insert(
Expand All @@ -167,6 +163,6 @@ impl<S> HistogramWithExemplars<S> {
}

pub(crate) fn inner(&self) -> RwLockReadGuard<HistogramWithExemplarsInner<S>> {
self.inner.read().expect("Lock not to be poisoned.")
self.inner.read()
}
}
16 changes: 7 additions & 9 deletions src/metrics/family.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
//! See [`Family`] for details.

use super::{MetricType, TypedMetric};
use owning_ref::OwningRef;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
use std::collections::HashMap;
use std::sync::{Arc, RwLock, RwLockReadGuard};
use std::sync::Arc;

/// Representation of the OpenMetrics *MetricFamily* data type.
///
Expand Down Expand Up @@ -214,29 +214,27 @@ impl<S: Clone + std::hash::Hash + Eq, M, C: MetricConstructor<M>> Family<S, M, C
/// // calls.
/// family.get_or_create(&vec![("method".to_owned(), "GET".to_owned())]).inc();
/// ```
pub fn get_or_create(&self, label_set: &S) -> OwningRef<RwLockReadGuard<HashMap<S, M>>, M> {
let read_guard = self.metrics.read().expect("Lock not to be poisoned.");
pub fn get_or_create(&self, label_set: &S) -> MappedRwLockReadGuard<M> {
if let Ok(metric) =
OwningRef::new(read_guard).try_map(|metrics| metrics.get(label_set).ok_or(()))
RwLockReadGuard::try_map(self.metrics.read(), |metrics| metrics.get(label_set))
{
return metric;
}

let mut write_guard = self.metrics.write().unwrap();
let mut write_guard = self.metrics.write();
write_guard.insert(label_set.clone(), self.constructor.new_metric());

drop(write_guard);

let read_guard = self.metrics.read().unwrap();
OwningRef::new(read_guard).map(|metrics| {
RwLockReadGuard::map(self.metrics.read(), |metrics| {
metrics
.get(label_set)
.expect("Metric to exist after creating it.")
})
}

pub(crate) fn read(&self) -> RwLockReadGuard<HashMap<S, M>> {
self.metrics.read().unwrap()
self.metrics.read()
}
}

Expand Down
18 changes: 8 additions & 10 deletions src/metrics/histogram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
//! See [`Histogram`] for details.

use super::{MetricType, TypedMetric};
use owning_ref::OwningRef;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
use std::iter::{self, once};
use std::sync::{Arc, Mutex, MutexGuard};
use std::sync::Arc;

/// Open Metrics [`Histogram`] to measure distributions of discrete events.
///
Expand Down Expand Up @@ -33,7 +33,7 @@ use std::sync::{Arc, Mutex, MutexGuard};
// https://github.com/tikv/rust-prometheus/pull/314.
#[derive(Debug)]
pub struct Histogram {
inner: Arc<Mutex<Inner>>,
inner: Arc<RwLock<Inner>>,
}

impl Clone for Histogram {
Expand All @@ -56,7 +56,7 @@ pub(crate) struct Inner {
impl Histogram {
pub fn new(buckets: impl Iterator<Item = f64>) -> Self {
Self {
inner: Arc::new(Mutex::new(Inner {
inner: Arc::new(RwLock::new(Inner {
sum: Default::default(),
count: Default::default(),
buckets: buckets
Expand All @@ -78,7 +78,7 @@ impl Histogram {
/// Needed in
/// [`HistogramWithExemplars`](crate::metrics::exemplar::HistogramWithExemplars).
pub(crate) fn observe_and_bucket(&self, v: f64) -> Option<usize> {
let mut inner = self.inner.lock().unwrap();
let mut inner = self.inner.write();
inner.sum += v;
inner.count += 1;

Expand All @@ -97,17 +97,15 @@ impl Histogram {
}
}

pub(crate) fn get(&self) -> (f64, u64, MutexGuardedBuckets) {
let inner = self.inner.lock().unwrap();
pub(crate) fn get(&self) -> (f64, u64, MappedRwLockReadGuard<Vec<(f64, u64)>>) {
let inner = self.inner.read();
let sum = inner.sum;
let count = inner.count;
let buckets = OwningRef::new(inner).map(|inner| &inner.buckets);
let buckets = RwLockReadGuard::map(inner, |inner| &inner.buckets);
(sum, count, buckets)
}
}

pub(crate) type MutexGuardedBuckets<'a> = OwningRef<MutexGuard<'a, Inner>, Vec<(f64, u64)>>;

impl TypedMetric for Histogram {
const TYPE: MetricType = MetricType::Histogram;
}
Expand Down