Skip to content

Commit d97f801

Browse files
authored
util: change some layers to require recorders that are Sync (#538)
1 parent 54aa553 commit d97f801

File tree

4 files changed

+74
-63
lines changed

4 files changed

+74
-63
lines changed

metrics-util/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
## [Unreleased] - ReleaseDate
1111

12+
### Changed
13+
14+
- `FanoutBuilder` and `RouterBuilder` now both require recorders to be `Sync` to facilitate usage with being installed
15+
as the global recorder.
16+
1217
## [0.18.0] - 2024-10-12
1318

1419
### Added

metrics-util/src/layers/fanout.rs

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ impl From<FanoutHistogram> for Histogram {
100100

101101
/// Fans out metrics to multiple recorders.
102102
pub struct Fanout {
103-
recorders: Vec<Box<dyn Recorder>>,
103+
recorders: Vec<Box<dyn Recorder + Sync>>,
104104
}
105105

106106
impl fmt::Debug for Fanout {
@@ -163,7 +163,7 @@ impl Recorder for Fanout {
163163
/// More information on the behavior of the layer can be found in [`Fanout`].
164164
#[derive(Default)]
165165
pub struct FanoutBuilder {
166-
recorders: Vec<Box<dyn Recorder>>,
166+
recorders: Vec<Box<dyn Recorder + Sync>>,
167167
}
168168

169169
impl fmt::Debug for FanoutBuilder {
@@ -178,7 +178,7 @@ impl FanoutBuilder {
178178
/// Adds a recorder to the fanout list.
179179
pub fn add_recorder<R>(mut self, recorder: R) -> FanoutBuilder
180180
where
181-
R: Recorder + 'static,
181+
R: Recorder + Sync + 'static,
182182
{
183183
self.recorders.push(Box::new(recorder));
184184
self
@@ -194,11 +194,20 @@ impl FanoutBuilder {
194194
mod tests {
195195
use super::FanoutBuilder;
196196
use crate::test_util::*;
197-
use metrics::{Counter, Gauge, Histogram, Unit};
197+
use metrics::{Counter, Gauge, Histogram, Recorder, Unit};
198198

199199
static METADATA: metrics::Metadata =
200200
metrics::Metadata::new(module_path!(), metrics::Level::INFO, Some(module_path!()));
201201

202+
#[test]
203+
fn sync() {
204+
#[allow(dead_code)]
205+
fn assert_sync_recorder<T: Recorder + Sync>(_t: &T) {}
206+
207+
let recorder = FanoutBuilder::default().build();
208+
assert_sync_recorder(&recorder);
209+
}
210+
202211
#[test]
203212
fn test_basic_functionality() {
204213
let operations = vec![

metrics-util/src/layers/router.rs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ use crate::{MetricKind, MetricKindMask};
99
///
1010
/// More information on the behavior of the layer can be found in [`RouterBuilder`].
1111
pub struct Router {
12-
default: Box<dyn Recorder>,
12+
default: Box<dyn Recorder + Sync>,
1313
global_mask: MetricKindMask,
14-
targets: Vec<Box<dyn Recorder>>,
14+
targets: Vec<Box<dyn Recorder + Sync>>,
1515
counter_routes: Trie<String, usize>,
1616
gauge_routes: Trie<String, usize>,
1717
histogram_routes: Trie<String, usize>,
@@ -92,9 +92,9 @@ impl Recorder for Router {
9292
///
9393
/// A default route (recorder) is always present and used in the case that no specific route exists.
9494
pub struct RouterBuilder {
95-
default: Box<dyn Recorder>,
95+
default: Box<dyn Recorder + Sync>,
9696
global_mask: MetricKindMask,
97-
targets: Vec<Box<dyn Recorder>>,
97+
targets: Vec<Box<dyn Recorder + Sync>>,
9898
counter_routes: Trie<String, usize>,
9999
gauge_routes: Trie<String, usize>,
100100
histogram_routes: Trie<String, usize>,
@@ -118,7 +118,7 @@ impl RouterBuilder {
118118
/// The given recorder is used as the default route when no other specific route exists.
119119
pub fn from_recorder<R>(recorder: R) -> Self
120120
where
121-
R: Recorder + 'static,
121+
R: Recorder + Sync + 'static,
122122
{
123123
RouterBuilder {
124124
default: Box::new(recorder),
@@ -144,7 +144,7 @@ impl RouterBuilder {
144144
) -> &mut RouterBuilder
145145
where
146146
P: AsRef<str>,
147-
R: Recorder + 'static,
147+
R: Recorder + Sync + 'static,
148148
{
149149
let target_idx = self.targets.len();
150150
self.targets.push(Box::new(recorder));
@@ -214,6 +214,15 @@ mod tests {
214214
}
215215
}
216216

217+
#[test]
218+
fn sync() {
219+
#[allow(dead_code)]
220+
fn assert_sync_recorder<T: Recorder + Sync>(_t: &T) {}
221+
222+
let recorder = RouterBuilder::from_recorder(MockTestRecorder::new()).build();
223+
assert_sync_recorder(&recorder);
224+
}
225+
217226
#[test]
218227
fn test_construction() {
219228
let _ = RouterBuilder::from_recorder(MockTestRecorder::new()).build();

metrics/src/recorder/mod.rs

Lines changed: 41 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -20,31 +20,28 @@ thread_local! {
2020

2121
/// A trait for registering and recording metrics.
2222
///
23-
/// This is the core trait that allows interoperability between exporter implementations and the
24-
/// macros provided by `metrics`.
23+
/// This is the core trait that allows interoperability between exporter implementations and the macros provided by
24+
/// `metrics`.
2525
pub trait Recorder {
2626
/// Describes a counter.
2727
///
28-
/// Callers may provide the unit or a description of the counter being registered. Whether or
29-
/// not a metric can be re-registered to provide a unit/description, if one was already passed
30-
/// or not, as well as how units/descriptions are used by the underlying recorder, is an
31-
/// implementation detail.
28+
/// Callers may provide the unit or a description of the counter being registered. Whether or not a metric can be
29+
/// re-registered to provide a unit/description, if one was already passed or not, as well as how units/descriptions
30+
/// are used by the underlying recorder, is an implementation detail.
3231
fn describe_counter(&self, key: KeyName, unit: Option<Unit>, description: SharedString);
3332

3433
/// Describes a gauge.
3534
///
36-
/// Callers may provide the unit or a description of the gauge being registered. Whether or
37-
/// not a metric can be re-registered to provide a unit/description, if one was already passed
38-
/// or not, as well as how units/descriptions are used by the underlying recorder, is an
39-
/// implementation detail.
35+
/// Callers may provide the unit or a description of the gauge being registered. Whether or not a metric can be
36+
/// re-registered to provide a unit/description, if one was already passed or not, as well as how units/descriptions
37+
/// are used by the underlying recorder, is an implementation detail.
4038
fn describe_gauge(&self, key: KeyName, unit: Option<Unit>, description: SharedString);
4139

4240
/// Describes a histogram.
4341
///
44-
/// Callers may provide the unit or a description of the histogram being registered. Whether or
45-
/// not a metric can be re-registered to provide a unit/description, if one was already passed
46-
/// or not, as well as how units/descriptions are used by the underlying recorder, is an
47-
/// implementation detail.
42+
/// Callers may provide the unit or a description of the histogram being registered. Whether or not a metric can be
43+
/// re-registered to provide a unit/description, if one was already passed or not, as well as how units/descriptions
44+
/// are used by the underlying recorder, is an implementation detail.
4845
fn describe_histogram(&self, key: KeyName, unit: Option<Unit>, description: SharedString);
4946

5047
/// Registers a counter.
@@ -125,19 +122,16 @@ impl_recorder!(T, std::sync::Arc<T>);
125122

126123
/// Guard for setting a local recorder.
127124
///
128-
/// When using a local recorder, we take a reference to the recorder and only hold it for as long as
129-
/// the duration of the closure. However, we must store this reference in a static variable
130-
/// (thread-local storage) so that it can be accessed by the macros. This guard ensures that the
131-
/// pointer we store to the reference is cleared when the guard is dropped, so that it can't be used
132-
/// after the closure has finished, even if the closure panics and unwinds the stack.
125+
/// When using a local recorder, we take a reference to the recorder and only hold it for as long as the duration of the
126+
/// closure. However, we must store this reference in a static variable (thread-local storage) so that it can be
127+
/// accessed by the macros. This guard ensures that the pointer we store to the reference is cleared when the guard is
128+
/// dropped, so that it can't be used after the closure has finished, even if the closure panics and unwinds the stack.
133129
///
134130
/// ## Note
135131
///
136-
/// The guard has a lifetime parameter `'a` that is bounded using a
137-
/// `PhantomData` type. This upholds the guard's contravariance, it must live
138-
/// _at most as long_ as the recorder it takes a reference to. The bounded
139-
/// lifetime prevents accidental use-after-free errors when using a guard
140-
/// directly through [`crate::set_default_local_recorder`].
132+
/// The guard has a lifetime parameter `'a` that is bounded using a `PhantomData` type. This upholds the guard's
133+
/// contravariance, it must live _at most as long_ as the recorder it takes a reference to. The bounded lifetime
134+
/// prevents accidental use-after-free errors when using a guard directly through [`crate::set_default_local_recorder`].
141135
pub struct LocalRecorderGuard<'a> {
142136
prev_recorder: Option<NonNull<dyn Recorder>>,
143137
phantom: PhantomData<&'a dyn Recorder>,
@@ -146,10 +140,9 @@ pub struct LocalRecorderGuard<'a> {
146140
impl<'a> LocalRecorderGuard<'a> {
147141
/// Creates a new `LocalRecorderGuard` and sets the thread-local recorder.
148142
fn new(recorder: &'a dyn Recorder) -> Self {
149-
// SAFETY: While we take a lifetime-less pointer to the given reference, the reference we
150-
// derive _from_ the pointer is given the same lifetime of the reference
151-
// used to construct the guard -- captured in the guard type itself --
152-
// and so derived references never outlive the source reference.
143+
// SAFETY: While we take a lifetime-less pointer to the given reference, the reference we derive _from_ the
144+
// pointer is given the same lifetime of the reference used to construct the guard -- captured in the guard type
145+
// itself -- and so derived references never outlive the source reference.
153146
let recorder_ptr = unsafe { NonNull::new_unchecked(recorder as *const _ as *mut _) };
154147

155148
let prev_recorder =
@@ -168,11 +161,11 @@ impl<'a> Drop for LocalRecorderGuard<'a> {
168161

169162
/// Sets the global recorder.
170163
///
171-
/// This function may only be called once in the lifetime of a program. Any metrics recorded
172-
/// before this method is called will be completely ignored.
164+
/// This function may only be called once in the lifetime of a program. Any metrics recorded before this method is
165+
/// called will be completely ignored.
173166
///
174-
/// This function does not typically need to be called manually. Metrics implementations should
175-
/// provide an initialization method that installs the recorder internally.
167+
/// This function does not typically need to be called manually. Metrics implementations should provide an
168+
/// initialization method that installs the recorder internally.
176169
///
177170
/// # Errors
178171
///
@@ -184,25 +177,21 @@ where
184177
GLOBAL_RECORDER.set(recorder)
185178
}
186179

187-
/// Sets the recorder as the default for the current thread for the duration of
188-
/// the lifetime of the returned [`LocalRecorderGuard`].
180+
/// Sets the recorder as the default for the current thread for the duration of the lifetime of the returned
181+
/// [`LocalRecorderGuard`].
189182
///
190-
/// This function is suitable for capturing metrics in asynchronous code, in particular
191-
/// when using a single-threaded runtime. Any metrics registered prior to the returned
192-
/// guard will remain attached to the recorder that was present at the time of registration,
193-
/// and so this cannot be used to intercept existing metrics.
183+
/// This function is suitable for capturing metrics in asynchronous code, in particular when using a single-threaded
184+
/// runtime. Any metrics registered prior to the returned guard will remain attached to the recorder that was present at
185+
/// the time of registration, and so this cannot be used to intercept existing metrics.
194186
///
195-
/// Additionally, local recorders can be used in a nested fashion. When setting a new
196-
/// default local recorder, the previous default local recorder will be captured if one
197-
/// was set, and will be restored when the returned guard drops.
187+
/// Additionally, local recorders can be used in a nested fashion. When setting a new default local recorder, the
188+
/// previous default local recorder will be captured if one was set, and will be restored when the returned guard drops.
198189
/// the lifetime of the returned [`LocalRecorderGuard`].
199190
///
200-
/// Any metrics recorded before a guard is returned will be completely ignored.
201-
/// Metrics implementations should provide an initialization method that
202-
/// installs the recorder internally.
191+
/// Any metrics recorded before a guard is returned will be completely ignored. Metrics implementations should provide
192+
/// an initialization method that installs the recorder internally.
203193
///
204-
/// The function is suitable for capturing metrics in asynchronous code that
205-
/// uses a single threaded runtime.
194+
/// The function is suitable for capturing metrics in asynchronous code that uses a single threaded runtime.
206195
///
207196
/// If a global recorder is set, it will be restored once the guard is dropped.
208197
#[must_use]
@@ -222,19 +211,18 @@ pub fn with_local_recorder<T>(recorder: &dyn Recorder, f: impl FnOnce() -> T) ->
222211

223212
/// Runs the closure with a reference to the current recorder for this scope.
224213
///
225-
/// If a local recorder has been set, it will be used. Otherwise, the global recorder will be used.
226-
/// If neither a local recorder or global recorder have been set, a no-op recorder will be used.
214+
/// If a local recorder has been set, it will be used. Otherwise, the global recorder will be used. If neither a local
215+
/// recorder or global recorder have been set, a no-op recorder will be used.
227216
///
228217
/// It should typically not be necessary to call this function directly, as it is used primarily by generated code. You
229218
/// should prefer working with the macros provided by `metrics` instead: `counter!`, `gauge!`, `histogram!`, etc.
230219
pub fn with_recorder<T>(f: impl FnOnce(&dyn Recorder) -> T) -> T {
231220
LOCAL_RECORDER.with(|local_recorder| {
232221
if let Some(recorder) = local_recorder.get() {
233-
// SAFETY: If we have a local recorder, we know that it is valid because it can only be
234-
// set during the duration of a closure that is passed to `with_local_recorder`, which
235-
// is the only time this method can be called and have a local recorder set. This
236-
// ensures that the lifetime of the recorder is valid for the duration of this method
237-
// call.
222+
// SAFETY: If we have a local recorder, we know that it is valid because it can only be set during the
223+
// duration of a closure that is passed to `with_local_recorder`, which is the only time this method can be
224+
// called and have a local recorder set. This ensures that the lifetime of the recorder is valid for the
225+
// duration of this method call.
238226
unsafe { f(recorder.as_ref()) }
239227
} else if let Some(global_recorder) = GLOBAL_RECORDER.try_load() {
240228
f(global_recorder)

0 commit comments

Comments
 (0)