add an environment-configured console_subscriber::init #53

jbr · 2021-06-18T19:03:42Z

closes #49

hawkw

overall, this looks like the right general direction; i had some minor style and documentation suggestions.

hawkw · 2021-06-18T19:28:37Z

console-subscriber/src/builder.rs

+    /**
+    Configures this builder from a standard set of environment variables:
+
+    * `TOKIO_CONSOLE_RETENTION`: The number of seconds to accumulate
+      completed tracing data. Default: 60s.
+    * `TOKIO_CONSOLE_BIND`: a HOST:PORT description, such as
+      localhost:1234 or similar. Default: 127.0.0.1:6669
+    * `TOKIO_CONSOLE_PUBLISH_INTERVAL`: The number of milliseconds to wait
+      between sending updates to the console. Default: 1000ms (1s)
+    */


style nit: elsewhere we use ///-style doc comments exclusively...i could be convinced to switch to using block comments for doc comments, but i'd prefer to be consistent about it.

I'm a big fan of /** / /*! for doc comments because it's less visually distracting, but in this case it was just habit and I should have checked for project convention

console-subscriber/src/builder.rs

hawkw · 2021-06-18T19:40:24Z

console-subscriber/src/lib.rs

+Starts the console subscriber server on its own thread
+
+This function represents the easiest way to get started using
+tokio-console.
+


these docs should probably state explicitly that this function also sets the default tracing subscriber (and maybe include a link to the tracing docs?). otherwise, i can see people being surprised if they try to set their own subscriber and it fails because the default subscriber is already set.

addressed in f2b6de5

hawkw · 2021-06-18T19:41:58Z

console-subscriber/src/builder.rs

+    ///   localhost:1234 or similar. Default: 127.0.0.1:6669
+    /// * `TOKIO_CONSOLE_PUBLISH_INTERVAL`: The number of milliseconds to wait
+    ///   between sending updates to the console. Default: 1000ms (1s)
+


take it or leave it: it would be nice if the docs for this also included an example of using the builder to configure a console layer from the default environment variables , and then using it to build a subscriber, spawn the background task, etc? but, we can come back and add that later.

Before I saw your review on this PR, I attempted to address this use case by adding a single in-code extension point: init_with_layer, with the hope that we can accommodate all common initialization patterns and discourage anyone who would need docs on how to eject from doing so. With the addition of init_with_layer, have we covered all of those use cases? An advantage of this approach is that if the majority of users are going through init or init_with_layer, future changes to those functions wouldn't require app code change, whereas if we encourage a copy-paste initializer through documentation, we have no way of pushing out changes to that "ejected" code.

console-subscriber/src/builder.rs

hawkw · 2021-06-18T19:46:01Z

console-subscriber/src/lib.rs

+* `TOKIO_CONSOLE_RETENTION`: The number of seconds to accumulate
+  completed tracing data. Default: 60s.
+* `TOKIO_CONSOLE_BIND`: a HOST:PORT description, such as
+  localhost:1234 or similar. Default: 127.0.0.1:6669
+* `TOKIO_CONSOLE_PUBLISH_INTERVAL`: The number of milliseconds to wait
+  between sending updates to the console. Default: 1000ms (1s)
+* `RUST_LOG`: configure the tracing filter. Default: `tokio=trace`,
+  and any additional filtering directives will be appended to this
+  default. See [`EnvFilter`] for further information on the format
+  for this variable.


take it or leave it: similarly, this might want to be a table?

addressed in f2b6de5

console-subscriber/src/lib.rs

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2021-06-18T21:11:30Z

console-subscriber/src/init.rs

+
+/// Starts the console subscriber server with an additional subscriber
+/// layer, for example a log layer. This interface can be combined
+/// with any of the environment configuration documented at [`init`].


it would be nice to show an example here, but it's okay to add that in a follow-up branch if you prefer?

What's a good example layer that people might want to add on here? My inexperience with tracing means I don't have a good intuition for what people might want

hawkw · 2021-06-18T21:13:15Z

console-subscriber/src/init.rs

+                EnvFilter::from_default_env().add_directive("tokio=trace".parse().unwrap());
+
+            let console_subscriber = tracing_subscriber::registry()
+                .with(fmt::layer())


do we always want to include a fmt layer? this means that users can't customize how events are logged, even if they call init_with_layer. i might just only add the fmt layer in init.

I'm new to tracing, but layers seem to be order-dependent, and there's no way to prepend because everything is composed through wrapping, so I'm not sure how to extract the fmt layer. Any suggestions?

Do you imagine replacing the fmt layer to be a commonly-needed extension point? I'm trying to figure out how to balance keeping the usage simple with providing the right sort of escape hatches for common customization

I'm new to tracing, but layers seem to be order-dependent, and there's no way to prepend because everything is composed through wrapping, so I'm not sure how to extract the fmt layer. Any suggestions?

layers are not order dependent :)

Do you imagine replacing the fmt layer to be a commonly-needed extension point? I'm trying to figure out how to balance keeping the usage simple with providing the right sort of escape hatches for common customization

the fmt layer is very configurable. i think wanting to change how events are formatted by providing your own fmt layer is probably going to be one of the most common use-cases for overriding the default configuration. for example, if you want to change the log format from textual to JSON, the current API means that you would have to use the builder and do everything manually.

addressed in bb2c2db — there's now init and build and build doesn't add the fmt layer. this does involve a bit of heavy-handed documentation because that might be surprising

hawkw · 2021-06-18T21:16:14Z

console-subscriber/src/init.rs

+pub fn init_with_layer<AdditionalLayer>(additional_layer: AdditionalLayer)
+where
+    AdditionalLayer: Layer<ConsoleSubscriberLayer> + Send + Sync + 'static,


an alternative API design is to have a function that spawns the background thread, and just returns a Registry with the EnvFilter and TasksLayer layers already added. that way, the user can call any methods on that Registry to add additional layers, and set it as the default subscriber themselves. the init function could just call that and then immediately call init() on the returned registry.

i feel like this design could be more intuitive than taking a layer as an argument, but it might be slightly wordier to use...i could be convinced either way.

That does feel more flexible. What if instead of init_with_layer it was build() -> impl Layer + Into<Dispatch>, with #[must_use].

console_subscriber::build().with(…).init() console_subscriber::init(); // shorthand for console_subscriber::build().init();

addressed in bb2c2db — build and init

console-subscriber/src/init.rs

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

because clippy was right

hawkw · 2021-06-21T20:42:34Z

console-subscriber/src/init.rs

+use tokio::runtime;
+use tracing_subscriber::{fmt, layer::Layered, prelude::*, EnvFilter, Registry};
+
+type ConsoleSubscriberLayer = Layered<TasksLayer, Layered<EnvFilter, Registry>>;


nit (and this isn't that important, since this isn't part of the public API), i would just call this

Suggested change

type ConsoleSubscriberLayer = Layered<TasksLayer, Layered<EnvFilter, Registry>>;

type ConsoleSubscriber = Layered<TasksLayer, Layered<EnvFilter, Registry>>;

this type isn't actually a Layer, since it includes a subscriber --- it doesn't implement the Layer trait, it implements the Subscriber trait.

hawkw · 2021-06-21T20:44:14Z

console-subscriber/src/init.rs

+/// ```
+


nit: extra whitespace here

Suggested change

/// ```

/// ```

console-subscriber/src/init.rs

hawkw · 2021-06-21T20:46:39Z

console-subscriber/src/init.rs

+/// ```
+
+#[must_use = "build() without init() will not attach a tracing subscriber"]
+pub fn build() -> ConsoleSubscriberLayer {


take it or leave it: i'm not totally sold on the name build() for this --- it doesn't feel super clear to me how this differs from init().. i might have called this subscriber() or something, because it returns a Subscriber. but, i'm not attached to that, this is fine.

console-subscriber/src/init.rs

add an environment-configured console_subscriber::init

f8edc47

jbr force-pushed the console-subscriber-init branch from 103021e to f8edc47 Compare June 18, 2021 19:30

hawkw requested changes Jun 18, 2021

View reviewed changes

jbr and others added 7 commits June 18, 2021 13:38

add init_with_layer to support additional use cases

dc57331

correct default retention documentation

a8854eb

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

formatting

6f14f44

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Use self-describing units

e70acaa

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

update docs to reflect self-describing units

c93f6a8

re-add blank line, assuming it was intentional

add8026

add env-filter to the set of tracing-subscriber features

8562bef

hawkw reviewed Jun 18, 2021

View reviewed changes

jbr added 3 commits June 18, 2021 14:17

tables!

f2b6de5

capitalization

4ebf2ec

update tables based on review

a1ef76b

hawkw reviewed Jun 19, 2021

View reviewed changes

console-subscriber/src/init.rs Outdated Show resolved Hide resolved

console-subscriber/src/init.rs Outdated Show resolved Hide resolved

jbr and others added 6 commits June 19, 2021 17:25

standardize docs

d03eee8

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

switch from init_with_layer to build

bb2c2db

punctuation

572e454

from_default_env→with_default_env

0c1a2da

because clippy was right

unrelated clippy fixes

f1f51aa

another quick pass over the docs

fc2b382

hawkw approved these changes Jun 22, 2021

View reviewed changes

docs suggestions from code review

d96366e

hawkw merged commit 5142a60 into tokio-rs:main Jun 22, 2021

hawkw mentioned this pull request Jul 13, 2021

subscriber: remove fmt layer from init #64

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add an environment-configured console_subscriber::init #53

add an environment-configured console_subscriber::init #53

jbr commented Jun 18, 2021

hawkw left a comment

hawkw Jun 18, 2021

jbr Jun 18, 2021

hawkw Jun 18, 2021

jbr Jun 18, 2021

hawkw Jun 18, 2021

jbr Jun 18, 2021

hawkw Jun 18, 2021

jbr Jun 18, 2021

hawkw Jun 18, 2021

jbr Jun 18, 2021

hawkw Jun 18, 2021

jbr Jun 18, 2021

hawkw Jun 19, 2021

jbr Jun 21, 2021

hawkw Jun 18, 2021

jbr Jun 18, 2021

jbr Jun 21, 2021

hawkw Jun 21, 2021

hawkw Jun 21, 2021

hawkw Jun 21, 2021

	type ConsoleSubscriberLayer = Layered<TasksLayer, Layered<EnvFilter, Registry>>;
	type ConsoleSubscriber = Layered<TasksLayer, Layered<EnvFilter, Registry>>;

add an environment-configured console_subscriber::init #53

add an environment-configured console_subscriber::init #53

Conversation

jbr commented Jun 18, 2021

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment