Skip to content

Add managed thread id to log output to add debugging threading issues #790

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

Merged
merged 2 commits into from
Nov 12, 2018

Conversation

rkeithhill
Copy link
Contributor

Also update format to display method before filename, makes it easy to
visually scan the log to see what is going on.

Here is log output before this change:

2018-11-03 17:26:10.560 [NORMAL] C:\PowerShellEditorServices\src\PowerShellEditorServices.Host\EditorServicesHost.cs: In method 'StartLanguageService', line 194:

And after:

2018-11-03 17:21:55.858 [NORMAL] [tid:15] In method 'StartLanguageService' C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices\src\PowerShellEditorServices.Host\EditorServicesHost.cs:194:

Also update format to display method before filename, makes it easy to
visually scan the log to see what is going on.
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Great idea! Thanks @rkeithhill

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM! This will be very handy

@TylerLeonhardt
Copy link
Member

@rkeithhill can you double check that vscode still gives you syntax highlighting like it does today?

pretty sure it's just looking for dates and numbers but I just want to double check 😄

@rkeithhill
Copy link
Contributor Author

I don't see much difference in the old output. The new thread id number is colorized blue:

image

@rkeithhill
Copy link
Contributor Author

rkeithhill commented Nov 10, 2018

How about I simplify/lessen the output a bit more. I don't think the [] around the thread id is necessary and after looking at the logs for a long time, I don't think the word "method" adds much info. Considering how large these logs can get, it is probably worth being frugal with what we output. So with that in mind, this is what the above suggested changes look like:

image

For 2.0.0, if Serilog supported ReadOnlySpan<char> I'd love to slice into the source paths, changing this:

C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices\src\PowerShellEditorServices.Host\EditorServicesHost.cs

to

src\PowerShellEditorServices.Host\EditorServicesHost.cs

I'd compute the slice offset once and stash it in a static variable. But alas, Serilog doesn't appear to support ReadOnlySpan<char>. We could use Substring but that would allocate another string on every call to the logger. Not a good trade-off IMO.

If folks agree with these changes, I'll push my commit with them and then I'd like to merge this.

@rkeithhill
Copy link
Contributor Author

OK, going for the presumptive close on the previous post. :-) If folks object, I'll rollback that last commit.

@rkeithhill rkeithhill merged commit abfffca into master Nov 12, 2018
@rkeithhill rkeithhill deleted the rkeithhill/add-tid-to-log branch November 12, 2018 03:00
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.

4 participants