Skip to content

Conversation

petm-dec
Copy link

@petm-dec petm-dec commented Mar 12, 2023

Hello, another point of discussion here: we would like to log the thread-id instead of the process-id under linux, similar to what the google glog c++ library does. Not sure if adding this linux specific dependency is ok. I cannot test it under windows right now, and if calling libc::gettid() would work under windows (glog c++ uses some windows api to get the thread id).

An example is in this commit: 38158a6

@petm-dec
Copy link
Author

Added a commit to use tid on windows too (similar to the glog c++ library).
I cannot test other target_os, but those would remain unchanged and use the pid.
An example for this is in commit 385bad5

@davidbarsky
Copy link
Owner

davidbarsky commented Mar 15, 2023

Whoops, sorry for the delay in reviewing/responding to this. Had my attention on some rust-analyzer stuff recently. I'm a little hesitant to make it a non-toggleable setting, as this library is used decently heavily in a Linux-based environment at my employer. I'll double-check what the current behavior is and what most people expect.

@petm-dec
Copy link
Author

Whoops, sorry for the delay in reviewing/responding to this. Had my attention on some rust-analyzer stuff recently. I'm a little hesitant to make it a non-toggleable setting, as this library is used decently heavily in a Linux-based environment at my employer. I'll double-check what the current behavior is and what most people expect.

No problem. Not in a hurry. With toggeable setting you mean as a feature?

I think in many cases one process writes one log file (in our case at least). In that case, the pid is always the same and can be derived from the log file, but for multithreaded applications the thread id might be useful.

@jsgf
Copy link
Collaborator

jsgf commented Mar 17, 2023

I'm a bit surprised glog is using the tid, but afaict that is what it uses.

With toggeable setting you mean as a feature?

No, I think it should be runtime config if its going to be a toggle. Features are not suitable for this kind of thing.

@petm-dec
Copy link
Author

petm-dec commented Mar 17, 2023

I'm a bit surprised glog is using the tid, but afaict that is what it uses.

With toggeable setting you mean as a feature?

No, I think it should be runtime config if its going to be a toggle. Features are not suitable for this kind of thing.

You're right, a runtime config to set this seems good.
Not sure about the details (how to toggle the option, how to name it).
Maybe it is better that you do that to your liking?

There are cases (most?) where a single process is writing a glog file (which is why the pid is in the file name), in that case the pid in the log record adds no information, but the tid could be useful for multi threaded programs.

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.

3 participants