Skip to content

Conversation

@Ruteri
Copy link
Member

@Ruteri Ruteri commented Nov 20, 2018

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]

@Ruteri Ruteri requested review from a team and scravy November 20, 2018 17:00
Copy link
Member

@scravy scravy left a 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.

@Ruteri Ruteri force-pushed the logprintthreadnames branch from cc8c156 to fe5b7e7 Compare November 20, 2018 17:19
@scravy scravy added backport Feature should be contributed to bitcoin utils/logs labels Nov 20, 2018
@scravy scravy added this to the Initial polish for maintenance milestone Nov 20, 2018
src/util.cpp Outdated
Copy link
Member

@kostyantyn kostyantyn Nov 20, 2018

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)

Copy link
Member

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).

Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

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.

Copy link
Member

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.

Copy link
Member

@kostyantyn kostyantyn Nov 21, 2018

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.

Copy link
Member

@kostyantyn kostyantyn left a 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]>
@Ruteri Ruteri force-pushed the logprintthreadnames branch from fe5b7e7 to df1c464 Compare November 21, 2018 10:15
@castarco
Copy link

utACK

@scravy
Copy link
Member

scravy commented Nov 21, 2018

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 cut, tr, sed, and awk and you'll be fine.

strong utACK df1c464

Copy link
Member

@kostyantyn kostyantyn left a comment

Choose a reason for hiding this comment

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

utACK df1c464

@scravy scravy merged commit eb54b47 into dtr-org:master Nov 21, 2018
@Ruteri Ruteri deleted the logprintthreadnames branch November 21, 2018 14:57
@cornelius cornelius added the tools Development tools label Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Feature should be contributed to bitcoin tools Development tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants