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

Use tracing for logging #141

Closed
wants to merge 1 commit into from
Closed

Use tracing for logging #141

wants to merge 1 commit into from

Conversation

Dev380
Copy link
Contributor

@Dev380 Dev380 commented Jul 8, 2023

Attempts progress on #125

Currently, this PR switches logging to the tracing crate, and uses system loggers and tracing's stdout logger on mac and linux. I'm unable to test if it actually works on mac because I don't have access to one. Additionally, everything is instrumented with the #[instrument] macro so we have spans for free. Haven't figured out how to log to a system log storage yet on windows, and the linux implementation depends on systemd existing. Adding more logging to tun still needs to be completed.

Copy link
Collaborator

@conradev conradev left a comment

Choose a reason for hiding this comment

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

This looks fantastic! I requested a few changes

Do you think you could squash your commits into a single commit?

burrow/Cargo.toml Outdated Show resolved Hide resolved
tun/Cargo.toml Outdated Show resolved Hide resolved
burrow/src/main.rs Outdated Show resolved Hide resolved
burrow/src/main.rs Outdated Show resolved Hide resolved

#[cfg(target_os = "linux")]
fn init_logger_layer() -> tracing_journald::Layer {
tracing_journald::layer().expect("Couldn't open journald socket - are you using systemd?").with_syslog_identifier("burrow".to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not crash if the user doesn't have journald installed on Linux. Further, we should fail silently if the error is an io::ErrorKind::NotFound (and I'd ask that we test that on a system without journald, if possible), but log the error if it is anything else. We don't want to spam the log for users that don't use journald.

also, can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - although I don't have a system/vm without journald available right now so I added a trace level log for later testing (it won't show up unless the RUST_LOG env variable is changed)

burrow/src/main.rs Outdated Show resolved Hide resolved
burrow/src/main.rs Outdated Show resolved Hide resolved
burrow/src/main.rs Outdated Show resolved Hide resolved
burrow/src/main.rs Outdated Show resolved Hide resolved
@conradev
Copy link
Collaborator

also, I can test on macOS!

@Dev380
Copy link
Contributor Author

Dev380 commented Jul 13, 2023

Do you think you could squash your commits into a single commit?

I'm not too familiar with git so I'm not sure how to do that - github says the repo owner has to enable it because the option is grayed out for me
20230712_20h54m09s_grim

This commit replaces the log crate and the env_logger backend with a
newer tokio tracing framework. This framework allows logging spans in
adddition to messages.
@conradev
Copy link
Collaborator

A git rebase is something you do locally, and is just a way of editing your commits. You'll have to do it locally, because Github doesn't help with it

I can show you how to do it when I get a chance, but also I squashed your commits for you in this case, so we should be good to merge

@conradev
Copy link
Collaborator

@Dev380 it looks like the builds are failing, though! You can ignore the "Apple Build" ones (which won't work because this is a PR from a fork and not a branch), but we need to fix the Rust Build errors

@conradev
Copy link
Collaborator

conradev commented Jul 15, 2023

It looks like tracing-oslog is broken on some versions of Rust/macOS, which I might need to help with:

3 |         __dso_handle, _os_activity_create, _os_activity_current, mach_header,
  |                       ^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^ no `_os_activity_current` in `ffi`

It also looks like we need a manual Debug implementation on Windows for TunInterface

Linux looks like a typo

@conradev
Copy link
Collaborator

Update: good to go, failing CI and merge conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants