-
Notifications
You must be signed in to change notification settings - Fork 151
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
Load JSON based NLog config (e.g. declared in appsettings.json) + BeginScopeParser performance improvements #263
Load JSON based NLog config (e.g. declared in appsettings.json) + BeginScopeParser performance improvements #263
Conversation
47db0d9
to
2a370b2
Compare
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 69.75% 69.92% +0.16%
==========================================
Files 10 12 +2
Lines 873 941 +68
Branches 158 165 +7
==========================================
+ Hits 609 658 +49
- Misses 203 221 +18
- Partials 61 62 +1
Continue to review full report at Codecov.
|
2a370b2
to
857d859
Compare
Performance Test using NLog 4.6-rc1 and NLog.Extension.Logging 1.5-rc1: Really like how the ConcurrentQueue in NetCore21 is able to shine with heavy concurrency (See Each test is logging 500000 msgs, and for each msg it first calls BeginScope with two parameters.
|
857d859
to
05213af
Compare
src/NLog.Extensions.Logging/Config/ExtensionLoggingConfiguration.cs
Outdated
Show resolved
Hide resolved
src/NLog.Extensions.Logging/Config/ExtensionLoggingConfiguration.cs
Outdated
Show resolved
Hide resolved
src/NLog.Extensions.Logging/Config/ExtensionLoggingConfiguration.cs
Outdated
Show resolved
Hide resolved
src/NLog.Extensions.Logging/Config/ExtensionLoggingConfiguration.cs
Outdated
Show resolved
Hide resolved
src/NLog.Extensions.Logging/Config/ExtensionLoggingConfiguration.cs
Outdated
Show resolved
Hide resolved
f12c796
to
f40a87f
Compare
@304NotModified Maybe considering the name aspnetcore. Yes it's wrong, but Ms is also doing that sometimes, eg. https://github.com/aspnet/Extensions
Think you are mixing the aspnet team-name together with their extensions-project. This is a NLog-library for the extensions-project.
Still puzzled why the naming cannot be ExtensionsLoggingConfiguration (And ExtensionsIlogggerTarget)
|
Because extensions is too generic and not bound to MEL? |
Think the class-name and namespace-name is helping a lot.
But what about NLogLoggingConfiguration and MicrosoftILoggerTarget?
|
Good for me! |
e400943
to
6c8c197
Compare
@304NotModified Resolved merge conflicts |
10079b9
to
660b8e6
Compare
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 refactored a bit. Used only tooling for it (and I replace all), I'm pretty sure I didn't break a thing :)
If you like to review it, that OK with me, otherwise I will merge it now.
test/NLog.Extensions.Logging.Tests/NLogLoggingConfigurationTests.cs
Outdated
Show resolved
Hide resolved
Everything looks good. |
🎉 merged! |
Excellent. Then just need to ensure that |
@304NotModified Added support for NLog variables: #282 |
Load NLog config from appsettings.json using NLogLoggingConfiguration
Loading configuration like this:
Example JSON-config:
Also updated the BeginScopeParser to make use of NLog 4.6 optimizations.
Custom nuget-package can be found here:
https://ci.appveyor.com/project/nlog/nlog-framework-logging/builds/22600809/artifacts