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

Capture structured logging parameter names #125

Merged
merged 1 commit into from
Oct 12, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Aug 5, 2017

Extracted from Microsoft FormattedLogValues. Allowing NLog to support structured logging using the Microsoft-parser-engine. Resolves #116

P.S. Funny detail discovered while digging around in the Microsoft-machine-room (Notice the static ConcurrentDictionary and limit of 1024)

https://github.com/aspnet/Logging/blob/cc503aaed0b9fb3e93345b28629e80581337d3c1/src/Microsoft.Extensions.Logging.Abstractions/Internal/FormattedLogValues.cs

P.P.S. Surprised that they are not using the StringComparer.Ordinal for the ConcurrentDictionary

@snakefoot snakefoot changed the title Added support for structured logging parameter names Capture structured logging parameter names Aug 5, 2017
@snakefoot
Copy link
Contributor Author

@304NotModified If approved, then please merge #121 first (Remember to change milestone to 1.0 RC), as these two will collide.

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 7, 2017

It could be nice if one could inject a custom format-function to the LogEventInfo (Gets LogEventInfo as input and returns string). Then it would be possible to assign LogEventInfo.Message to the {OriginalFormat} and ensure LogEventInfo.FormattedMessage doesn't attempt to using string.Format (But the custom format-function).

This would make it easier for custom targets supported structured logging to capture the LogEventInfo.Message. Maybe this custom format-function should be part of NLog/NLog#2208 ?

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 7, 2017

Created PR to improve NLog integration with Microsoft.Extensions.Logging: NLog/NLog#2244

@304NotModified 304NotModified self-assigned this Aug 8, 2017
@304NotModified 304NotModified added this to the RC 1 milestone Aug 11, 2017
@304NotModified
Copy link
Member

@304NotModified If approved, then please merge #121 first (Remember to change milestone to 1.0 RC), as these two will collide.

Thanks for the reminder!

Conflict is now here O:)

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 12, 2017

@304NotModified Will wait with this until NLog/NLog#2244 is released (For NLog ver. 4.4 and ver. 5.0). Btw. going on vacation the next 2 weeks.

@304NotModified
Copy link
Member

Have a good holiday!

@304NotModified
Copy link
Member

(waiting on NLog 4.5 beta)

@304NotModified
Copy link
Member

We can continue on this one, isn't?

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 19, 2017 via email

@304NotModified 304NotModified modified the milestones: Beta 6, 1.0 Sep 26, 2017
@304NotModified
Copy link
Member

@snakefoot do you think I have to delay RTM 1.0 for this PR?

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 29, 2017

@304NotModified Still waiting for NLog/NLog#2262 :) No idea if it is needed for RTM.

@304NotModified
Copy link
Member

@304NotModified Still waiting for NLog/NLog#2262 :)

Yes, I will take more time to give better feedback.

No idea if it is needed for RTM.

Is this a breaking change? (functional)

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 29, 2017

Is this a breaking change? (functional)

No idea. The structured logging properties provided by the Microsoft Extensions Logging are kind of secret. And since NLog doesn't support structured logging in its current state, then I guess it is okay that it is left out. But again I'm not using Microsoft Extensions Logging :)

@304NotModified
Copy link
Member

Please fix the conflict, thanks!

@snakefoot
Copy link
Contributor Author

@304NotModified Please fix the conflict, thanks!

Can see that you have done a partial update of the NLog-dependency, so now running all 3 major versions at the same time. Not the best environment to work in.

But anyway have updated NetStandard2.0 to use the new MesageTemplate-logic to capture the structured-logging parameters (Kept the original method for the other major versions).

@304NotModified
Copy link
Member

Thanks!

After 4.5 RTM I will update the dependencies

Ps now 2 major versions according to semver :p

@304NotModified 304NotModified merged commit 5fb3b93 into NLog:master Oct 12, 2017
@304NotModified 304NotModified modified the milestones: 1.0, 1.0 rc1 Oct 12, 2017
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.

2 participants