Skip to content

Commit 8376022

Browse files
hawkwhawkw
authored
subscriber: fix reload ergonomics (#1035)
## Motivation Currently, the `reload` layer is generic over both the type of the layer being reloaded, *and* the type of the subscriber that the layer is layered onto. This means that the `reload::Handle` type that's used to reload the value of the layer *also* is parameterized over the subscriber's type. The subscriber type parameter makes the `reload` API significantly harder to use. Any time a `reload::Handle` is returned by a function, taken as an argument, or stored in a struct, the full type of the subscriber under the layer must be written out --- and often, it is quite long. What makes this worse is that sometimes the type of the subscriber may vary at runtime based on some configuration, while the type of the layer that's reloaded remains the same. For example, in Linkerd, we've had to do [this][1], which is really not ideal. ## Solution This branch removes the `Subscriber` type parameter from `reload::Layer` and `reload::Handle`. Now, the `Handle` type is only generic over the type of the inner layer that's being reloaded. It turns out that the `Subscriber` type parameter was only necessary to add a `L: Layer<S>` bound to `reload::Layer`'s constructor, which isn't really necessary --- if the layer does not implement `Layer<S>`, the type error will occur when `Subscriber::with` is actually used to layer it, which is fine. I also changed the `with_filter_reloading` option on the `FmtSubscriber` builder to also work with `LevelFilter`s, since there's no reason for it not to, and added an example. Since this breaks existing code, this change has to be made as part of 0.3. [1]: https://github.com/linkerd/linkerd2-proxy/blob/6c484f6dcdeebda18b68c800b4494263bf98fcdc/linkerd/app/core/src/trace.rs#L19-L36
1 parent aef5bd8 commit 8376022

File tree

3 files changed

+63
-62
lines changed

3 files changed

+63
-62
lines changed

‎examples/examples/tower-load.rs‎

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -185,23 +185,20 @@ impl<T> Service<T> for MakeSvc {
185185
}
186186
}
187187

188-
struct AdminSvc<S> {
189-
handle: Handle<EnvFilter, S>,
188+
struct AdminSvc {
189+
handle: Handle<EnvFilter>,
190190
}
191191

192-
impl<S> Clone for AdminSvc<S> {
192+
impl Clone for AdminSvc {
193193
fn clone(&self) -> Self {
194194
Self {
195195
handle: self.handle.clone(),
196196
}
197197
}
198198
}
199199

200-
impl<'a, S> Service<&'a AddrStream> for AdminSvc<S>
201-
where
202-
S: tracing::Subscriber,
203-
{
204-
type Response = AdminSvc<S>;
200+
impl<'a> Service<&'a AddrStream> for AdminSvc {
201+
type Response = AdminSvc;
205202
type Error = hyper::Error;
206203
type Future = Ready<Result<Self::Response, Self::Error>>;
207204

@@ -214,10 +211,7 @@ where
214211
}
215212
}
216213

217-
impl<S> Service<Request<Body>> for AdminSvc<S>
218-
where
219-
S: tracing::Subscriber + 'static,
220-
{
214+
impl Service<Request<Body>> for AdminSvc {
221215
type Response = Response<Body>;
222216
type Error = Err;
223217
type Future = Pin<Box<dyn Future<Output = Result<Response<Body>, Err>> + std::marker::Send>>;
@@ -252,10 +246,7 @@ where
252246
}
253247
}
254248

255-
impl<S> AdminSvc<S>
256-
where
257-
S: tracing::Subscriber + 'static,
258-
{
249+
impl AdminSvc {
259250
fn set_from(&self, bytes: Bytes) -> Result<(), String> {
260251
use std::str;
261252
let body = str::from_utf8(&bytes.as_ref()).map_err(|e| format!("{}", e))?;

‎tracing-subscriber/src/fmt/mod.rs‎

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ use crate::{
137137
filter::LevelFilter,
138138
layer,
139139
registry::{LookupSpan, Registry},
140+
reload,
140141
};
141142

142143
#[doc(inline)]
@@ -646,35 +647,13 @@ impl<T, F, W> SubscriberBuilder<format::JsonFields, format::Format<format::Json,
646647
}
647648
}
648649

649-
#[cfg(feature = "env-filter")]
650-
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
651-
impl<N, E, W> SubscriberBuilder<N, E, crate::EnvFilter, W>
652-
where
653-
Formatter<N, E, W>: tracing_core::Subscriber + 'static,
654-
{
655-
/// Configures the subscriber being built to allow filter reloading at
656-
/// runtime.
657-
pub fn with_filter_reloading(
658-
self,
659-
) -> SubscriberBuilder<N, E, crate::reload::Layer<crate::EnvFilter, Formatter<N, E, W>>, W>
660-
{
661-
let (filter, _) = crate::reload::Layer::new(self.filter);
662-
SubscriberBuilder {
663-
filter,
664-
inner: self.inner,
665-
}
666-
}
667-
}
668-
669-
#[cfg(feature = "env-filter")]
670-
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
671-
impl<N, E, W> SubscriberBuilder<N, E, crate::reload::Layer<crate::EnvFilter, Formatter<N, E, W>>, W>
650+
impl<N, E, F, W> SubscriberBuilder<N, E, reload::Layer<F>, W>
672651
where
673652
Formatter<N, E, W>: tracing_core::Subscriber + 'static,
674653
{
675654
/// Returns a `Handle` that may be used to reload the constructed subscriber's
676655
/// filter.
677-
pub fn reload_handle(&self) -> crate::reload::Handle<crate::EnvFilter, Formatter<N, E, W>> {
656+
pub fn reload_handle(&self) -> reload::Handle<F> {
678657
self.filter.handle()
679658
}
680659
}
@@ -814,6 +793,51 @@ impl<N, E, F, W> SubscriberBuilder<N, E, F, W> {
814793
}
815794
}
816795

796+
/// Configures the subscriber being built to allow filter reloading at
797+
/// runtime.
798+
///
799+
/// The returned builder will have a [`reload_handle`] method, which returns
800+
/// a [`reload::Handle`] that may be used to set a new filter value.
801+
///
802+
/// For example:
803+
///
804+
/// ```
805+
/// use tracing::Level;
806+
/// use tracing_subscriber::prelude::*;
807+
///
808+
/// let builder = tracing_subscriber::fmt()
809+
/// // Set a max level filter on the subscriber
810+
/// .with_max_level(Level::INFO)
811+
/// .with_filter_reloading();
812+
///
813+
/// // Get a handle for modifying the subscriber's max level filter.
814+
/// let handle = builder.reload_handle();
815+
///
816+
/// // Finish building the subscriber, and set it as the default.
817+
/// builder.finish().init();
818+
///
819+
/// // Currently, the max level is INFO, so this event will be disabled.
820+
/// tracing::debug!("this is not recorded!");
821+
///
822+
/// // Use the handle to set a new max level filter.
823+
/// // (this returns an error if the subscriber has been dropped, which shouldn't
824+
/// // happen in this example.)
825+
/// handle.reload(Level::DEBUG).expect("the subscriber should still exist");
826+
///
827+
/// // Now, the max level is INFO, so this event will be recorded.
828+
/// tracing::debug!("this is recorded!");
829+
/// ```
830+
///
831+
/// [`reload_handle`]: SubscriberBuilder::reload_handle
832+
/// [`reload::Handle`]: crate::reload::Handle
833+
pub fn with_filter_reloading(self) -> SubscriberBuilder<N, E, reload::Layer<F>, W> {
834+
let (filter, _) = reload::Layer::new(self.filter);
835+
SubscriberBuilder {
836+
filter,
837+
inner: self.inner,
838+
}
839+
}
840+
817841
/// Sets the function that the subscriber being built should use to format
818842
/// events that occur.
819843
pub fn event_format<E2>(self, fmt_event: E2) -> SubscriberBuilder<N, E2, F, W>

‎tracing-subscriber/src/reload.rs‎

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ use crate::sync::RwLock;
1616

1717
use std::{
1818
error, fmt,
19-
marker::PhantomData,
2019
sync::{Arc, Weak},
2120
};
2221
use tracing_core::{
@@ -27,20 +26,18 @@ use tracing_core::{
2726

2827
/// Wraps a `Layer`, allowing it to be reloaded dynamically at runtime.
2928
#[derive(Debug)]
30-
pub struct Layer<L, S> {
29+
pub struct Layer<L> {
3130
// TODO(eliza): this once used a `crossbeam_util::ShardedRwLock`. We may
3231
// eventually wish to replace it with a sharded lock implementation on top
3332
// of our internal `RwLock` wrapper type. If possible, we should profile
3433
// this first to determine if it's necessary.
3534
inner: Arc<RwLock<L>>,
36-
_s: PhantomData<fn(S)>,
3735
}
3836

3937
/// Allows reloading the state of an associated `Layer`.
4038
#[derive(Debug)]
41-
pub struct Handle<L, S> {
39+
pub struct Handle<L> {
4240
inner: Weak<RwLock<L>>,
43-
_s: PhantomData<fn(S)>,
4441
}
4542

4643
/// Indicates that an error occurred when reloading a layer.
@@ -57,7 +54,7 @@ enum ErrorKind {
5754

5855
// ===== impl Layer =====
5956

60-
impl<L, S> crate::Layer<S> for Layer<L, S>
57+
impl<L, S> crate::Layer<S> for Layer<L>
6158
where
6259
L: crate::Layer<S> + 'static,
6360
S: Subscriber,
@@ -113,38 +110,28 @@ where
113110
}
114111
}
115112

116-
impl<L, S> Layer<L, S>
117-
where
118-
L: crate::Layer<S> + 'static,
119-
S: Subscriber,
120-
{
113+
impl<L> Layer<L> {
121114
/// Wraps the given `Layer`, returning a `Layer` and a `Handle` that allows
122115
/// the inner type to be modified at runtime.
123-
pub fn new(inner: L) -> (Self, Handle<L, S>) {
116+
pub fn new(inner: L) -> (Self, Handle<L>) {
124117
let this = Self {
125118
inner: Arc::new(RwLock::new(inner)),
126-
_s: PhantomData,
127119
};
128120
let handle = this.handle();
129121
(this, handle)
130122
}
131123

132124
/// Returns a `Handle` that can be used to reload the wrapped `Layer`.
133-
pub fn handle(&self) -> Handle<L, S> {
125+
pub fn handle(&self) -> Handle<L> {
134126
Handle {
135127
inner: Arc::downgrade(&self.inner),
136-
_s: PhantomData,
137128
}
138129
}
139130
}
140131

141132
// ===== impl Handle =====
142133

143-
impl<L, S> Handle<L, S>
144-
where
145-
L: crate::Layer<S> + 'static,
146-
S: Subscriber,
147-
{
134+
impl<L> Handle<L> {
148135
/// Replace the current layer with the provided `new_layer`.
149136
pub fn reload(&self, new_layer: impl Into<L>) -> Result<(), Error> {
150137
self.modify(|layer| {
@@ -189,11 +176,10 @@ where
189176
}
190177
}
191178

192-
impl<L, S> Clone for Handle<L, S> {
179+
impl<L> Clone for Handle<L> {
193180
fn clone(&self) -> Self {
194181
Handle {
195182
inner: self.inner.clone(),
196-
_s: PhantomData,
197183
}
198184
}
199185
}

0 commit comments

Comments
 (0)