Trace static level filtering #987

phungleson · 2019-03-16T09:41:32Z

Motivation

tokio-trace should have static verbosity level filtering, like the log crate. The static max verbosity level should be controlled at compile time with a set of features. It should be possible to set a separate max level for release and debug mode builds.

#959

Solution

We can do this fairly similarly to how the log crate does it: tokio-trace should export a constant whose value is set based on the static max level feature flags. Then, we can add an if statement to the span! and event! macros, like

hawkw

Thanks for the contribution! This seems like a good start; I left some pointers in comments.

For next steps, we'll need to add if statements to the event! and span! macros comparing the event/span's verbosity level with the static max. I gave an example of what this would look like in the description of issue #959.

hawkw · 2019-03-16T23:19:55Z

tokio-trace/src/lib.rs

+/// An enum representing the available verbosity level filters of the logger.
+#[repr(usize)]
+#[derive(Copy, Clone, Debug, Hash)]
+pub enum LevelFilter {


For forwards-compatibility reasons, we try to avoid having enum types in public APIs. Can this be a struct wrapping an inner private enum type instead? Refer to how the Level type in tokio-trace-core is implemented:

tokio/tokio-trace/tokio-trace-core/src/metadata.rs

Lines 224 to 272 in 92d5120

// ===== impl Level =====

impl Level {

/// The "error" level.

///

/// Designates very serious errors.

pub const ERROR: Level = Level(LevelInner::Error);

/// The "warn" level.

///

/// Designates hazardous situations.

pub const WARN: Level = Level(LevelInner::Warn);

/// The "info" level.

///

/// Designates useful information.

pub const INFO: Level = Level(LevelInner::Info);

/// The "debug" level.

///

/// Designates lower priority information.

pub const DEBUG: Level = Level(LevelInner::Debug);

/// The "trace" level.

///

/// Designates very low priority, often extremely verbose, information.

pub const TRACE: Level = Level(LevelInner::Trace);

}

#[repr(usize)]

#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]

enum LevelInner {

/// The "error" level.

///

/// Designates very serious errors.

Error = 1,

/// The "warn" level.

///

/// Designates hazardous situations.

Warn,

/// The "info" level.

///

/// Designates useful information.

Info,

/// The "debug" level.

///

/// Designates lower priority information.

Debug,

/// The "trace" level.

///

/// Designates very low priority, often extremely verbose, information.

Trace,

}

@hawkw Thanks for this, if we already have this Level, perhaps we can just use Level for this purpose?

Only Off level is missing, do we actually need it? what do you think?

It seems to be private struct though.

@hawkw Thanks for this, if we already have this Level, perhaps we can just use Level for this purpose?

Only Off level is missing, do we actually need it? what do you think?

I think we will want an OFF level for the static filter, so that tokio-trace can be completely disabled in release mode, etc. So, we'll probably need a separate LevelFilter type.

The implementation can be the same as tokio-trace-cores Level type, but with the addition of an OFF level. We will want to implement the PartialOrd trait for comparing LevelFilters with Levels, (impl PartialOrd<Level> for LevelFilter, with the same ordering as for Levels, but with LevelFilter::OFF considered less than all Levels

It seems to be private struct though.

The Level struct is public:

tokio/tokio-trace/tokio-trace-core/src/metadata.rs

Line 129 in 92d5120

pub struct Level(LevelInner);

while the LevelInner enum is private:

tokio/tokio-trace/tokio-trace-core/src/metadata.rs

Line 251 in 92d5120

enum LevelInner {

We'll want our LevelFilter type to follow the same pattern.

tokio-trace/src/lib.rs

phungleson · 2019-03-17T00:34:54Z

@hawkw Hey mate, thanks again I have updated the code to the best I can understand. Please give some thoughts about the implementation.

If it is ok (or almost there) I can start looking into tests for this feature. 😄

And remove cfg-if dependency

tokio-trace/src/macros.rs

phungleson · 2019-03-20T10:45:34Z

Hey @hawkw I have updated the implementation, can you check again? cheers,

A bit strange is that I have to impl PartialOrd<LevelFilter> for Level instead of impl PartialOrd<Level> for LevelFilter.

hawkw · 2019-03-20T17:10:41Z

It looks like the CI build failure is due to an error installing Rust, so that's not your fault. I'm going to restart the build.

hawkw

I think we're on the right track, but there are a couple more changes I'd like made.

In particular:

can we rename the fully disabled LevelFilter from "zero" to "off" (which is what log calls it)? This would mean renaming the associated const as well as the feature flags.
I just noticed an issue with the way the cfg attributes are being used that I think means we'll probably want to bring back the cfg_if macro, despite what I said earlier. Sorry about that.

That said, I really like the way you've implemented this feature, and I think we're most of the way there. Thanks for putting up with all the changes I keep requesting!

tokio-trace/src/level_filters.rs

hawkw

This is looking good. I commented on a couple more nits. I think if we address them and my comment about changing the feature flags (#987 (comment)), this will be good to merge (although tests would still be nice...)

tokio-trace/src/level_filters.rs

hawkw · 2019-03-20T20:35:33Z

tokio-trace/src/level_filters.rs

+use std::cmp::Ordering;
+use tokio_trace_core::Level;
+
+/// Describes the level of verbosity of a span or event.


Is this comment copied from Level? I think it should probably describe the difference between a Level and a LevelFilter...

outdated

phungleson · 2019-03-21T10:47:07Z

Hey @hawkw thanks so much for your guidance so far. Even though I would like to add the test, and a vaguely understand the test in log, it seems hard for me to digest what need to be done in order to test this feature correctly. It would be great if you drops some hints in my main.rs copy from log.

Thanks again!

hawkw

I've added some comments describing how we can adapt the log crate's tests to work for tokio-trace. Let me know if you get stuck!

tokio-trace/test_static_max_level_features/main.rs

phungleson · 2019-03-22T11:23:01Z

Hey @hawkw I have updated to the best I can understand, but it is probably missing something.

Let me know if you want to add or remove anything.

hawkw

@phungleson The test looks good to me --- now all we have to do is integrate it into the rest of the test suite.

In addition to the inline comments I left, I think we'd want to add it to the list here:

tokio/azure-pipelines.yml

Lines 39 to 49 in 678f15b

    
           crates: 
        
             - tokio-buf 
        
             - tokio-codec 
        
             - tokio-current-thread 
        
             - tokio-executor 
        
             - tokio-io 
        
             - tokio-sync 
        
             - tokio-threadpool 
        
             - tokio-timer 
        
             - tokio-trace 
        
             - tokio-trace/tokio-trace-core

so that the test actually gets run on CI.

Once we do that, I think this is good to merge! Thanks for working on this!

hawkw · 2019-03-22T20:23:03Z

tokio-trace/test_static_max_level_features/main.rs

+    fn exit(&self, _span: &Id) {}
+}
+
+fn main() {


Can we change this from a main method to a test? so

#[test] fn test_static_max_level_features() { // ... }

I think in order for cargo test to pick this up, we'd also want to move the file into atokio-trace/test_static_max_level_features/tests/ directory.

hawkw · 2019-03-22T23:24:33Z

tokio-trace/test_static_max_level_features/Cargo.toml

+version = "0.1.0"
+publish = false
+
+[[bin]]


Since the test was moved into tests, the whole [[bin]] section needs to be removed.

Doing so will fix the CI build.

hawkw

Beyond one issue in azure-pipelines.yml that should be trivial to fix, this looks good to me. Thanks @phungleson!

I noticed that the FreeBSD CI build is failing, but I don't think that's your fault --- we've been having trouble with it recently. I can sort that out before merging this PR.

azure-pipelines.yml

phungleson · 2019-03-24T00:00:53Z

Thanks so much for all the guidance @hawkw I have made the changes and things seem ok, except failing FreeBSD!

hawkw · 2019-03-25T18:59:40Z

Okay, I've figured out what's up with the CI build on FreeBSD: the FreeBSD build runs tests using cargo test --all, while the Azure Pipelines build runs cargo test for each individual crate.

What this means is that on FreeBSD, one version of tokio-trace is compiled as a dependency for the test_static_max_level test crate, and this is also used as the version of tokio-trace that's used to run the tokio-trace test suite. Since the dependency on tokio-trace in the test_static_max_level crate sets the max_level_debug feature, this feature is also set for tokio-trace's own tests. This disables anything at the trace level in those tests, causing the tests to fail.

An obvious solution is to just change the Cirrus CI config to do the same thing as the Azure Pipelines CI builds, but I'd prefer to have cargo test --all still work. Will look into coming up with a workaround.

hawkw · 2019-03-25T20:20:24Z

@phungleson if I can get you to merge https://github.com/phungleson/tokio/pull/1 into your fork, I think that will resolve the FreeBSD CI issues and we'll be able to move forward with this!

Fix FreeBSD CI

phungleson · 2019-03-25T21:59:43Z

Looks pretty good, thanks @hawkw I have learned a lot through the process!

hawkw · 2019-03-25T22:00:38Z

@phungleson Great, thanks for your contribution! I'm going to go ahead and merge this when the CI build passes.

hawkw · 2019-03-25T22:19:35Z

aaaand....merged! thanks again @phungleson!

phungleson · 2019-03-25T22:40:42Z

Cool awesome! thanks again for all your help @hawkw

Do you know any particular issues that can be suitable to look into? (and hope I can be more helpful this time 😄)

hawkw · 2019-03-25T23:17:35Z

@phungleson you'll probably want to keep your eye out for issues tagged as "easy". There is also a "good first issue" tag in the tokio-trace-nursery repository, which has less stable crates to use with tokio-trace (although that tag is currently empty).

## Motivation `tokio-trace` should have static verbosity level filtering, like the `log` crate. The static max verbosity level should be controlled at compile time with a set of features. It should be possible to set a separate max level for release and debug mode builds. ## Solution We can do this fairly similarly to how the `log` crate does it: `tokio-trace` should export a constant whose value is set based on the static max level feature flags. Then, we add an if statement to the `span!` and `event!` macros which tests if that event or span's level is enabled. Closes #959

First try with some copies from log crate

d1a1b36

phungleson mentioned this pull request Mar 16, 2019

trace: Static level filtering #959

Closed

hawkw reviewed Mar 16, 2019

View reviewed changes

Use STATIC_MAX_LEVEL for event and span macro

da55666

And remove cfg-if dependency

hawkw reviewed Mar 18, 2019

View reviewed changes

tokio-trace/src/macros.rs Outdated Show resolved Hide resolved

Add level filters

88a52e6

hawkw previously requested changes Mar 20, 2019

View reviewed changes

tokio-trace/src/level_filters.rs Outdated Show resolved Hide resolved

tokio-trace/src/level_filters.rs Outdated Show resolved Hide resolved

tokio-trace/src/level_filters.rs Show resolved Hide resolved

tokio-trace/src/level_filters.rs Outdated Show resolved Hide resolved

Simplify LevelFilter

4ca5275

hawkw reviewed Mar 20, 2019

View reviewed changes

phungleson added 2 commits March 21, 2019 09:57

Back flip to use cfg-if

19c8dec

Better description for OFF level filter

b50d7d8

phungleson added 4 commits March 21, 2019 21:15

Docs for LevelFilter

833bf7a

Have STATIC_MAX_LEVEL as a separate var.

fdb9aa2

Add list of features to tokio-trace

f36e76f

Add test_static_max_level_features

8d22c94

hawkw reviewed Mar 21, 2019

View reviewed changes

Fix test_static_max_level_features

f707b63

hawkw reviewed Mar 22, 2019

View reviewed changes

Small adjustment

0655762

hawkw reviewed Mar 22, 2019

View reviewed changes

Remove un-necessary [[bin]]

b720a42

hawkw approved these changes Mar 23, 2019

View reviewed changes

azure-pipelines.yml Outdated Show resolved Hide resolved

hawkw added finish-line C-enhancement Category: A PR with an enhancement or bugfix. labels Mar 23, 2019

hawkw added the tokio-trace label Mar 23, 2019

Copy and paste fix

0455b63

hawkw mentioned this pull request Mar 25, 2019

trace: Allow trace instrumentation to emit log records #992

Merged

de-break FreeBSD CI

dcf501a

Merge pull request #1 from tokio-rs/eliza/fix-ci

2fd6352

Fix FreeBSD CI

hawkw merged commit 1524ee4 into tokio-rs:master Mar 25, 2019

phungleson deleted the trace-static-level-filtering branch March 25, 2019 22:34

hawkw mentioned this pull request Mar 25, 2019

Add span and event macros that are only enabled in debug mode tokio-rs/tracing#12

Closed

hawkw added this to the tokio-trace v0.1 milestone Mar 27, 2019

hawkw mentioned this pull request Mar 27, 2019

trace: What level should spans default to? #952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trace static level filtering #987

Trace static level filtering #987

phungleson commented Mar 16, 2019

hawkw left a comment

hawkw Mar 16, 2019

phungleson Mar 16, 2019 •

edited

Loading

hawkw Mar 18, 2019

phungleson commented Mar 17, 2019 •

edited

Loading

phungleson commented Mar 20, 2019 •

edited

Loading

hawkw commented Mar 20, 2019

hawkw left a comment

hawkw left a comment

hawkw Mar 20, 2019

phungleson commented Mar 21, 2019

hawkw left a comment

phungleson commented Mar 22, 2019

hawkw left a comment

hawkw Mar 22, 2019

hawkw Mar 22, 2019

hawkw left a comment

phungleson commented Mar 24, 2019 •

edited

Loading

hawkw commented Mar 25, 2019

hawkw commented Mar 25, 2019

phungleson commented Mar 25, 2019

hawkw commented Mar 25, 2019

hawkw commented Mar 25, 2019

phungleson commented Mar 25, 2019

hawkw commented Mar 25, 2019

	// ===== impl Level =====

	impl Level {
	/// The "error" level.
	///
	/// Designates very serious errors.
	pub const ERROR: Level = Level(LevelInner::Error);
	/// The "warn" level.
	///
	/// Designates hazardous situations.
	pub const WARN: Level = Level(LevelInner::Warn);
	/// The "info" level.
	///
	/// Designates useful information.
	pub const INFO: Level = Level(LevelInner::Info);
	/// The "debug" level.
	///
	/// Designates lower priority information.
	pub const DEBUG: Level = Level(LevelInner::Debug);
	/// The "trace" level.
	///
	/// Designates very low priority, often extremely verbose, information.
	pub const TRACE: Level = Level(LevelInner::Trace);
	}

	#[repr(usize)]
	#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
	enum LevelInner {
	/// The "error" level.
	///
	/// Designates very serious errors.
	Error = 1,
	/// The "warn" level.
	///
	/// Designates hazardous situations.
	Warn,
	/// The "info" level.
	///
	/// Designates useful information.
	Info,
	/// The "debug" level.
	///
	/// Designates lower priority information.
	Debug,
	/// The "trace" level.
	///
	/// Designates very low priority, often extremely verbose, information.
	Trace,
	}

	crates:
	- tokio-buf
	- tokio-codec
	- tokio-current-thread
	- tokio-executor
	- tokio-io
	- tokio-sync
	- tokio-threadpool
	- tokio-timer
	- tokio-trace
	- tokio-trace/tokio-trace-core

Trace static level filtering #987

Trace static level filtering #987

Conversation

phungleson commented Mar 16, 2019

Motivation

Solution

hawkw left a comment

Choose a reason for hiding this comment

hawkw Mar 16, 2019

Choose a reason for hiding this comment

phungleson Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

hawkw Mar 18, 2019

Choose a reason for hiding this comment

phungleson commented Mar 17, 2019 • edited Loading

phungleson commented Mar 20, 2019 • edited Loading

hawkw commented Mar 20, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Mar 20, 2019

Choose a reason for hiding this comment

phungleson commented Mar 21, 2019

hawkw left a comment

Choose a reason for hiding this comment

phungleson commented Mar 22, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw Mar 22, 2019

Choose a reason for hiding this comment

hawkw Mar 22, 2019

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

phungleson commented Mar 24, 2019 • edited Loading

hawkw commented Mar 25, 2019

hawkw commented Mar 25, 2019

phungleson commented Mar 25, 2019

hawkw commented Mar 25, 2019

hawkw commented Mar 25, 2019

phungleson commented Mar 25, 2019

hawkw commented Mar 25, 2019

phungleson Mar 16, 2019 •

edited

Loading

phungleson commented Mar 17, 2019 •

edited

Loading

phungleson commented Mar 20, 2019 •

edited

Loading

phungleson commented Mar 24, 2019 •

edited

Loading