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

Add support for gettid() to PatternEncoder #232

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Add support for gettid() to PatternEncoder #232

merged 3 commits into from
Aug 17, 2021

Conversation

cloneable
Copy link
Contributor

Add {i} and {tid} to allow logging in particular the Linux
thread ID as returned by gettid(). This is very useful for
system-wide debugging und monitoring with consistent
thread/task IDs.

Fixes: #231

Add {i} and {tid} to allow logging in particular the Linux
thread ID as returned by gettid(). This is very useful for
system-wide debugging und monitoring with consistent
thread/task IDs.

Fixes: #231
@cloneable
Copy link
Contributor Author

Two build errors. I'll take a look later.

error[E0658]: `cfg(rustdoc)` is experimental and subject to change
  --> C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\gettid-0.1.2\src\lib.rs:16:55
   |
16 | #[cfg(any(target_os = "linux", target_os = "android", rustdoc))]
   |                                                       ^^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/43781
error[E0433]: failed to resolve: use of undeclared crate or module `gettid`
  --> src/encode/mod.rs:33:34
   |
33 |     pub(crate) static TID: u64 = gettid::gettid()
   |                                  ^^^^^^ use of undeclared crate or module `gettid`

src/encode/mod.rs Outdated Show resolved Hide resolved
@estk
Copy link
Owner

estk commented Aug 12, 2021

The gettid crate seems to use the unstable compilation flag cfg(rustdoc). The issue is that we build for stable rust and unstable compilation flags are unavailable on the stable channel of rust. I don't love that the crate has no tests.

I found another crate which has a seemingly more complete impl. I also like that it leverages nix which is know to be high quality software. The downside of the palaver crate is it seems maintenance has lapsed, and latest version is an alpha release from 2020.

It's a little unclear what to do from here, maybe bug the palaver authors or steal their implementation and add a comment linking back to an issue requesting they stabilize their api, so when that happens we can just pull them in...

@cloneable
Copy link
Contributor Author

I had some similar thoughts about gettid crate. I was thinking of asking the thread-id people if the would add gettid() to their crate. (I didn't know about palaver.) log4rs already depends on it and thread-id seems like a good place to add support. What do you think about this?

@estk
Copy link
Owner

estk commented Aug 12, 2021

@cloneable that sounds like a great plan. Lmk if there is anything I can do to help.

@cloneable
Copy link
Contributor Author

So I looked at palaver and it seems like a well written library, but I agree that owners may have stopped caring. (The owner of the repo is still active elsewhere.) I like that they support more OSes (but not Redox), that they avoid phtread_self() and that their gettid() returns u64 whereas thread-id's get() returns usize for some weird reason.

In summary none of gettid, thread-id and palaver is a great choice, but I still prefer palaver's provided implementations of gettid(). I'll ask them about state of maintainership and stability.

@cloneable
Copy link
Contributor Author

cloneable commented Aug 14, 2021

nix is failing to build in 1.40 due to missing support for const_fn. I understand 1.40 is required for something, but are you planning to move to a younger release soon? If not then palaver is out.

@estk
Copy link
Owner

estk commented Aug 16, 2021

So, im open to changing the 1.40 requirement, but my feeling is that we should have at least a year of rust compiler backward compatibility. We could open a PR for palaver to gate the const fns on rust versions with dtolnay's: rustversion. @alecmocatta do you plan on cutting a non-alpha 0.3 soon? Are you looking for a maintainer for the palaver crate?

@alecmocatta
Copy link

@estk Thanks for the tag. I don't have the bandwidth at the moment but I'd happily add you as palaver maintainer to do so.

@estk
Copy link
Owner

estk commented Aug 17, 2021

@alecmocatta that would be great, I'll work on a pr for palaver that will add compatibility for 1.40. Excellent crate btw.

@alecmocatta
Copy link

@estk Thanks, look forward to reviewing!

@estk
Copy link
Owner

estk commented Aug 17, 2021

@alecmocatta & @cloneable it actually looks like the problem is in nix and it is only in one line of a macro there. I'm going to see if its just that and if they are open to PR's on the subject. I'll keep this thread up to date on how I go.

@estk
Copy link
Owner

estk commented Aug 17, 2021

Ok @alecmocatta & @cloneable I think the world has moved on. MSRV is 1.46 in not only nix, but also the bitflags crate. I've bumped master to test on 1.46. @cloneable you'll need to rebase and should be good. Apologies for holding this up.

@estk estk marked this pull request as ready for review August 17, 2021 20:54
@cloneable
Copy link
Contributor Author

Looks good now. Thank you both!

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.

Offer thread ID returned by gettid() in pattern encoder
3 participants