-
Notifications
You must be signed in to change notification settings - Fork 14
Add option to include thread names in debug output #257
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
Conversation
scravy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 8200d7a
Apart from the condition-thing all in all good restructuring!
When I touched this code I was maybe a little too worried about performance. Nevertheless, I do think that this whole string business there should be handled by a proper string stream in the future. Just some random toughts, does not have to do anything with this pull.
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
…hread name) Signed-off-by: Mateusz Morusiewicz <[email protected]>
cc8c156 to
fe5b7e7
Compare
src/util.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep adding prefixes makes the actual log message harder to read. I see we add another 15 characters prefix.
WDYS if we don't make any fixed prefix? If some category has 10 characters, we write 10, if another 4 we write 4.
And I'd suggest removing []. The reason is, it makes easier to parse logs, simply split by space delimiter. In case when we don't have a category, then I'd print 2 spaces (it doesn't violate parsing rule)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's does not have anything to do with this pull request, open an issue for this.
I for one find log messages easier to read if they are aligned column wise (way easier).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's related as after this PR, significant part of the debug.log (and screen) will be filled with metadata (prefixes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also find it easier to read vertically aligned logs. And without the delimiter it could become less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will always be aligned, for instance when someone shares logs via email or another channel and they use a different font. Then these extra spaces will be at least annoying. Or the person who shares logs will manually remove them as you did in the description to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran out of arguments, so feel free to keep it as it is if it's not convincing.
Unless others want to add smth: @cornelius @Gnappuraz @AM5800 @frolosofsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description of this pull request is a nice example how vertical alignment breaks down. I personally do not care too much about log formatting as long as the information is there and it's machine-readable. Then you can always post-process it to make it more beautiful or align it to your needs.
src/util.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid fixed prefix sizes for the reason mentioned below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't. Logs are horrible to read if not aligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, this table-view looks nice only from a distance, but once I start reading the actual content, extra whitespaces make it harder + it causes more logs to be split by multiple lines on a small screen. Also, it makes harder to parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tooling problem (I use vim/less for unwrapping logs often), also I believe some terminals allow you to disable wrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that vertical alignment is a real problem except for some specific fields like timestamps, log type and "module". If the module name (or prefix as is named here) has a variable length and the log message starts at a different column is not very problematic since we have the ability to use/create proper tools to deal with these logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kostyantyn Why don't you open up an issue for this. I can imagine a bunch of creative ways of fixing this. Ofr example have an option to emit structured output, json for example; or by specifying the formatting in a sprintf style kind of format on the command line, etc. etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with the comment as it was related but I am OK to move it to a separate issue/PR and discuss it separately. And I don't see this discussion as a blocker to merge this PR as extra meta information can be disabled.
kostyantyn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConceptACK
Signed-off-by: Mateusz Morusiewicz <[email protected]> Switched thread name to char[] and added renaming to proposer thread Signed-off-by: Mateusz Morusiewicz <[email protected]>
fe5b7e7 to
df1c464
Compare
|
utACK |
|
I like this. Since it is not the default I think this should be merged. You can opt in to this new functionality if you like it, and you don't have to if you do not. You can send a pull request if you want to improve on this newly added, nice feature. I do see merit in the considerations put forward by reviewers, but I do think that this is highly subjective to use case and personal preference. In my opinion these things should be configured. Then again proper tooling can already do the job. Pipe the output to strong utACK df1c464 |
kostyantyn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK df1c464
Added a thread_local variable to store thread name.
As for torcontrol, it is too long (max is 15 chars) as unite- is prepended to it.
Example output (IRL everything is vertically adjusted):
2018-11-20 17:06:04 unite-net [ ] net thread start
2018-11-20 17:06:04 unite-dnsseed [ ] dnsseed thread start
2018-11-20 17:06:04 unite-dnsseed [ ] Loading addresses from DNS seeds
2018-11-20 17:06:04 unite-addcon [ ] addcon thread start
2018-11-20 17:06:04 unite-opencon [ ] opencon thread start
2018-11-20 17:06:04 united [ ] init message: Done loading
2018-11-20 17:06:04 unite-msghand [ ] msghand thread start
2018-11-20 17:06:04 united [ proposing] proposer-0: initialized.
2018-11-20 17:06:04 united [ proposing] responsible for: wallet.dat
Signed-off-by: Mateusz Morusiewicz [email protected]