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

tracing_subscriber should log to stderr #4992

Open
mxinden opened this issue Dec 10, 2023 · 20 comments
Open

tracing_subscriber should log to stderr #4992

mxinden opened this issue Dec 10, 2023 · 20 comments
Assignees
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:nicetohave

Comments

@mxinden
Copy link
Member

mxinden commented Dec 10, 2023

Summary

Diagnostics (e.g. logging) should go to stderr. Output of a program should go to stdout.

Expected behavior

tracing_subscriber to log to stderr.

Actual behavior

tracing_subscriber is logging to stdout.

Upstream bug report tokio-rs/tracing#2492

Relevant log output

No response

Possible Solution

One can easily override the default:

    let _ = tracing_subscriber::fmt()
        .with_env_filter(EnvFilter::from_default_env())
        .with_writer(std::io::stderr)
        .try_init();

Initialization of tracing_subscriber is already quite noisy. E.g. it requires 4 lines instead of the 1 line of env_logger. Should we introduce some abstraction?

Version

tracing_subscriber introduced with #4282.

Would you like to work on fixing this bug ?

No

@mxinden mxinden added priority:nicetohave help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Dec 10, 2023
@eserilev
Copy link
Contributor

I'd like to pick this up

@thomaseizinger
Copy link
Contributor

Initialization of tracing_subscriber is already quite noisy. E.g. it requires 4 lines instead of the 1 line of env_logger. Should we introduce some abstraction?

I think that would make sense. We only use this in examples and tests, neither of which get released to crates.io, so we can make a crate in misc/ that is publish = false.

@JacobOgle
Copy link

@eserilev still working on this? If not I wouldn't mind taking a look

@eserilev
Copy link
Contributor

eserilev commented Jan 9, 2024

@JacobOgle feel free to pick this up if you'd like

@thomaseizinger thomaseizinger assigned JacobOgle and unassigned eserilev Jan 10, 2024
@nazreen
Copy link

nazreen commented Feb 25, 2024

Can I pick this up?

@JacobOgle
Copy link

@nazreen go for it! I've been too tied up with school to take a dive into it

@nazreen
Copy link

nazreen commented Feb 25, 2024

@JacobOgle cool! this will be my first time contributing to this repo. the label getting-started is synonymous with good-first-issue right?

@thomaseizinger
Copy link
Contributor

@JacobOgle cool! this will be my first time contributing to this repo. the label getting-started is synonymous with good-first-issue right?

Yes!

@drHuangMHT
Copy link
Contributor

@nazreen Are you currently working on this? I would like to pick this up if you don't mind.

@nazreen
Copy link

nazreen commented Mar 11, 2024 via email

@drHuangMHT
Copy link
Contributor

I suppose we can use a macro to generate the code? And we temporarily override the default manually until
the upstream have fixed it. What do you think @thomaseizinger

@thomaseizinger
Copy link
Contributor

I don't think there is a need for macros if we can just have a function that is reused across different places.

@drHuangMHT
Copy link
Contributor

My plan is to put the function in /libp2p/src/lib.rs, any idea where to put the function?

@thomaseizinger
Copy link
Contributor

My plan is to put the function in /libp2p/src/lib.rs, any idea where to put the function?

We definitely don't want it in the libp2p crate because:

  • It would make it part of the public API
  • We would create a circular dependency

Like I said above, best to make a new crate that is marked as publish = false and only used as a dev-dependency.

@drHuangMHT
Copy link
Contributor

Like I said above, best to make a new crate that is marked as publish = false and only used as a dev-dependency.

Ok. What would you like to call it? logging? And put it under misc?

@thomaseizinger
Copy link
Contributor

Like I said above, best to make a new crate that is marked as publish = false and only used as a dev-dependency.

Ok. What would you like to call it? logging? And put it under misc?

Yeah, logging works :)

@drHuangMHT
Copy link
Contributor

I find another problem: dependency version. There has been multiple instances of tracing (and tracing_subscribers) in the manifests of all crates that depend on it, which will make depencency bumps harder to track. And cargo deny has been throwing a TON of warnings about duplicate entries of the same crate. Should we use logging to replace tracing(with re-exporting of course) across the repo?

@thomaseizinger
Copy link
Contributor

I find another problem: dependency version. There has been multiple instances of tracing (and tracing_subscribers) in the manifests of all crates that depend on it, which will make depencency bumps harder to track. And cargo deny has been throwing a TON of warnings about duplicate entries of the same crate. Should we use logging to replace tracing(with re-exporting of course) across the repo?

We can unify the version of tracing used across the entire workspace using workspace dependencies.

Re-exports don't work that well because we'd have to make logging a non-dev dependency then which means we'd have to publish it to crates.io.

@drHuangMHT
Copy link
Contributor

We can unify the version of tracing used across the entire workspace using workspace dependencies.

Yeah sure, workspace, I forgot that...

Re-exports don't work that well because we'd have to make logging a non-dev dependency then which means we'd have to publish it to crates.io.

Not sure why it has to be published though.

@thomaseizinger
Copy link
Contributor

Re-exports don't work that well because we'd have to make logging a non-dev dependency then which means we'd have to publish it to crates.io.

Not sure why it has to be published though.

If you want to use logging as a kind of "proxy" crate for tracing, you would also have to replace all uses of tracing that we use in the protocol crates for emitting logs. Those are published on crates.io and you can't publish something on crates.io with dependencies that aren't published there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:nicetohave
Projects
None yet
Development

No branches or pull requests

6 participants