Skip to content

Commit

Permalink
subscriber: fix reload ergonomics (#1035)
Browse files Browse the repository at this point in the history 
## 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
  • Loading branch information
@hawkw
hawkw authored Oct 13, 2020
1 parent aef5bd8 commit 8376022
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 62 deletions.
23 changes: 7 additions & 16 deletions examples/examples/tower-load.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,23 +185,20 @@ impl<T> Service<T> for MakeSvc {
}
}

struct AdminSvc<S> {
handle: Handle<EnvFilter, S>,
struct AdminSvc {
handle: Handle<EnvFilter>,
}

impl<S> Clone for AdminSvc<S> {
impl Clone for AdminSvc {
fn clone(&self) -> Self {
Self {
handle: self.handle.clone(),
}
}
}

impl<'a, S> Service<&'a AddrStream> for AdminSvc<S>
where
S: tracing::Subscriber,
{
type Response = AdminSvc<S>;
impl<'a> Service<&'a AddrStream> for AdminSvc {
type Response = AdminSvc;
type Error = hyper::Error;
type Future = Ready<Result<Self::Response, Self::Error>>;

Expand All @@ -214,10 +211,7 @@ where
}
}

impl<S> Service<Request<Body>> for AdminSvc<S>
where
S: tracing::Subscriber + 'static,
{
impl Service<Request<Body>> for AdminSvc {
type Response = Response<Body>;
type Error = Err;
type Future = Pin<Box<dyn Future<Output = Result<Response<Body>, Err>> + std::marker::Send>>;
Expand Down Expand Up @@ -252,10 +246,7 @@ where
}
}

impl<S> AdminSvc<S>
where
S: tracing::Subscriber + 'static,
{
impl AdminSvc {
fn set_from(&self, bytes: Bytes) -> Result<(), String> {
use std::str;
let body = str::from_utf8(&bytes.as_ref()).map_err(|e| format!("{}", e))?;
Expand Down
72 changes: 48 additions & 24 deletions tracing-subscriber/src/fmt/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ use crate::{
filter::LevelFilter,
layer,
registry::{LookupSpan, Registry},
reload,
};

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

#[cfg(feature = "env-filter")]
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
impl<N, E, W> SubscriberBuilder<N, E, crate::EnvFilter, W>
where
Formatter<N, E, W>: tracing_core::Subscriber + 'static,
{
/// Configures the subscriber being built to allow filter reloading at
/// runtime.
pub fn with_filter_reloading(
self,
) -> SubscriberBuilder<N, E, crate::reload::Layer<crate::EnvFilter, Formatter<N, E, W>>, W>
{
let (filter, _) = crate::reload::Layer::new(self.filter);
SubscriberBuilder {
filter,
inner: self.inner,
}
}
}

#[cfg(feature = "env-filter")]
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
impl<N, E, W> SubscriberBuilder<N, E, crate::reload::Layer<crate::EnvFilter, Formatter<N, E, W>>, W>
impl<N, E, F, W> SubscriberBuilder<N, E, reload::Layer<F>, W>
where
Formatter<N, E, W>: tracing_core::Subscriber + 'static,
{
/// Returns a `Handle` that may be used to reload the constructed subscriber's
/// filter.
pub fn reload_handle(&self) -> crate::reload::Handle<crate::EnvFilter, Formatter<N, E, W>> {
pub fn reload_handle(&self) -> reload::Handle<F> {
self.filter.handle()
}
}
Expand Down Expand Up @@ -814,6 +793,51 @@ impl<N, E, F, W> SubscriberBuilder<N, E, F, W> {
}
}

/// Configures the subscriber being built to allow filter reloading at
/// runtime.
///
/// The returned builder will have a [`reload_handle`] method, which returns
/// a [`reload::Handle`] that may be used to set a new filter value.
///
/// For example:
///
/// ```
/// use tracing::Level;
/// use tracing_subscriber::prelude::*;
///
/// let builder = tracing_subscriber::fmt()
/// // Set a max level filter on the subscriber
/// .with_max_level(Level::INFO)
/// .with_filter_reloading();
///
/// // Get a handle for modifying the subscriber's max level filter.
/// let handle = builder.reload_handle();
///
/// // Finish building the subscriber, and set it as the default.
/// builder.finish().init();
///
/// // Currently, the max level is INFO, so this event will be disabled.
/// tracing::debug!("this is not recorded!");
///
/// // Use the handle to set a new max level filter.
/// // (this returns an error if the subscriber has been dropped, which shouldn't
/// // happen in this example.)
/// handle.reload(Level::DEBUG).expect("the subscriber should still exist");
///
/// // Now, the max level is INFO, so this event will be recorded.
/// tracing::debug!("this is recorded!");
/// ```
///
/// [`reload_handle`]: SubscriberBuilder::reload_handle
/// [`reload::Handle`]: crate::reload::Handle
pub fn with_filter_reloading(self) -> SubscriberBuilder<N, E, reload::Layer<F>, W> {
let (filter, _) = reload::Layer::new(self.filter);
SubscriberBuilder {
filter,
inner: self.inner,
}
}

/// Sets the function that the subscriber being built should use to format
/// events that occur.
pub fn event_format<E2>(self, fmt_event: E2) -> SubscriberBuilder<N, E2, F, W>
Expand Down
30 changes: 8 additions & 22 deletions tracing-subscriber/src/reload.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use crate::sync::RwLock;

use std::{
error, fmt,
marker::PhantomData,
sync::{Arc, Weak},
};
use tracing_core::{
Expand All @@ -27,20 +26,18 @@ use tracing_core::{

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

/// Allows reloading the state of an associated `Layer`.
#[derive(Debug)]
pub struct Handle<L, S> {
pub struct Handle<L> {
inner: Weak<RwLock<L>>,
_s: PhantomData<fn(S)>,
}

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

// ===== impl Layer =====

impl<L, S> crate::Layer<S> for Layer<L, S>
impl<L, S> crate::Layer<S> for Layer<L>
where
L: crate::Layer<S> + 'static,
S: Subscriber,
Expand Down Expand Up @@ -113,38 +110,28 @@ where
}
}

impl<L, S> Layer<L, S>
where
L: crate::Layer<S> + 'static,
S: Subscriber,
{
impl<L> Layer<L> {
/// Wraps the given `Layer`, returning a `Layer` and a `Handle` that allows
/// the inner type to be modified at runtime.
pub fn new(inner: L) -> (Self, Handle<L, S>) {
pub fn new(inner: L) -> (Self, Handle<L>) {
let this = Self {
inner: Arc::new(RwLock::new(inner)),
_s: PhantomData,
};
let handle = this.handle();
(this, handle)
}

/// Returns a `Handle` that can be used to reload the wrapped `Layer`.
pub fn handle(&self) -> Handle<L, S> {
pub fn handle(&self) -> Handle<L> {
Handle {
inner: Arc::downgrade(&self.inner),
_s: PhantomData,
}
}
}

// ===== impl Handle =====

impl<L, S> Handle<L, S>
where
L: crate::Layer<S> + 'static,
S: Subscriber,
{
impl<L> Handle<L> {
/// Replace the current layer with the provided `new_layer`.
pub fn reload(&self, new_layer: impl Into<L>) -> Result<(), Error> {
self.modify(|layer| {
Expand Down Expand Up @@ -189,11 +176,10 @@ where
}
}

impl<L, S> Clone for Handle<L, S> {
impl<L> Clone for Handle<L> {
fn clone(&self) -> Self {
Handle {
inner: self.inner.clone(),
_s: PhantomData,
}
}
}
Expand Down

0 comments on commit 8376022

Please sign in to comment.