Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the logging component to a tracing component. #154

Merged
merged 2 commits into from Nov 28, 2019

Conversation

hdevalence
Copy link
Contributor

@hdevalence hdevalence commented Nov 19, 2019

Will resolve #149 when complete.

Some questions:

  1. How do I determine whether ANSI should be used?
  2. Should the tracing settings respect RUST_LOG?

I didn't update the crate list in the README.

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented on a few things. I'm not sure what @tarcieri thinks is best for abscissa in particular, so none of my feedback should be considered blocking :)

core/Cargo.toml Show resolved Hide resolved

// Construct a tracing subscriber with the supplied filter and enable reloading.
let builder = FmtSubscriber::builder()
// XXX this should use ColorChoice::should_ansi, but where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the log code that was removed in this PR, it looks like ansi colors were always used (but I may be misreading it?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the color support regressed when I switched from term to termcolor (#47). I posted some up-to-date thoughts on it in this comment:

#154 (comment)

core/src/trace/component.rs Show resolved Hide resolved
/// Attempt to log
pub fn try_log(&self, record: &Record<'_>) -> Result<(), Error> {
let mut stream = self.level_stream(record);
let now = chrono::Utc::now();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the removed logging code always used UTC timestamps. Should we configure tracing's timestamps to also use UTC?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with using the system timezone (which for all of my servers is UTC) if that's the default.

core/src/logging/logger.rs Show resolved Hide resolved
@tarcieri
Copy link
Collaborator

How do I determine whether ANSI should be used?

There's an Application::term_colors method you can source that info from.

Also as of #148 the atty crate is a hard dependency. It might make sense (but is certainly not required, just food for thought) to disable colors if no TTY is detected.

Should the tracing settings respect RUST_LOG?

It wasn't respected before, but I think it'd be a good idea. I've had people try to use it who were confused when it didn't work.

@hdevalence
Copy link
Contributor Author

It might make sense (but is certainly not required, just food for thought) to disable colors if no TTY is detected.

If the tracing component uses Application::term_colors, does it make sense to also do TTY checking there, or would that logic (if it existed) be better in the term_colors function itself?

@tarcieri
Copy link
Collaborator

tarcieri commented Nov 20, 2019

It'd be better if the term_colors method did it, so it also affects the color settings for the terminal component and color-backtraces

@hdevalence hdevalence marked this pull request as ready for review November 27, 2019 20:36
@hdevalence
Copy link
Contributor Author

I'm not sure which of the discussions above are "resolved", but I think that the remaining things are: 1) correct use of Application::term_colors and 2) overriding config-based filters with the value of RUST_LOG?

@hdevalence
Copy link
Contributor Author

I know how to construct a filter based on RUST_LOG using from_default_env via https://docs.rs/tracing-subscriber/0.2.0-alpha.1/tracing_subscriber/filter/struct.EnvFilter.html#method.from_default_env , but I don't know how to have that override a filter directive -- it looks like using https://docs.rs/tracing-subscriber/0.2.0-alpha.1/tracing_subscriber/filter/struct.EnvFilter.html#method.add_directive will override the env variable rather than the other way around.

@tarcieri
Copy link
Collaborator

@hdevalence yup, sounds good!

@tarcieri
Copy link
Collaborator

Re: RUST_LOG, it doesn't work currently, so if nothing else it won't be a regression if you want to save it for a followup PR (or perhaps I can take a look). It's just a nice-to-have.

@hdevalence
Copy link
Contributor Author

Okay, this is blocking ZcashFoundation/zebra#108 so I would prefer to fix up the RUST_LOG stuff later if that's OK.

core/src/application.rs Outdated Show resolved Hide resolved
Copy link
Member

@tony-iqlusion tony-iqlusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hdevalence if you're done it looks good to me

@hdevalence
Copy link
Contributor Author

Yep!

@tony-iqlusion tony-iqlusion merged commit 50c4b85 into iqlusioninc:develop Nov 28, 2019
@tony-iqlusion tony-iqlusion mentioned this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from log to tracing
4 participants