NLogTraceListener - Reduce overhead by checking LogLevel #2137

snakefoot · 2017-05-27T20:04:44Z

Check the configured Trace-Filter before creating LogEventInfo object.

Check the LogLevel before creating LogEventInfo object.

codecov · 2017-05-27T20:17:31Z

Codecov Report

Merging #2137 into master will decrease coverage by <1%.
The diff coverage is 49%.

@@           Coverage Diff           @@
##           master   #2137    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         291     291            
  Lines       20113   20124    +11     
  Branches     2385    2392     +7     
=======================================
+ Hits        16351   16356     +5     
- Misses       3150    3152     +2     
- Partials      612     616     +4

snakefoot · 2017-05-28T22:49:37Z

Added support for TraceFilter. So resolving #799 and #798

304NotModified · 2017-05-29T15:27:03Z

Is this done or WIP?

snakefoot · 2017-05-29T17:00:43Z

Is this done or WIP?

It is done. Just trying to get the Appveyor-build working, but it is failing because of environment-issues (The network path was not found)

Just like this build after merging #2138 :

https://ci.appveyor.com/project/nlog/nlog/build/4.4.5326

304NotModified · 2017-05-29T17:30:26Z

It's an unstable test?

snakefoot · 2017-05-29T17:31:46Z

It's an unstable test?

Then all ImpersonatingTargetWrapperTests have suddenly become very unstable, as they are now constantly failing. Or maybe they have become stable ? :)

304NotModified · 2017-05-29T17:36:56Z

Lol

304NotModified · 2017-05-29T17:38:06Z

Will rebuild master to be sure

304NotModified · 2017-05-29T17:43:30Z

Failed also (same issue? - on mobile, hard to check)

https://ci.appveyor.com/project/nlog/nlog/build/4.4.5329

snakefoot · 2017-05-29T17:49:00Z

Failed also (same issue?

Yes always the same 4 tests (Because the test-constructor throws exception)

Guess something has happened with the lookup of the NLogTestUser = "NLogTestUser" on localhost machine (127.0.0.1). Maybe IPv4 vs. IPv6, or reduced rights ?

NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.ImpersonatingWrapperTest [FAIL]
   System.Runtime.InteropServices.COMException : The network path was not found.
   Stack Trace:
      at System.DirectoryServices.DirectoryEntry.Bind(Boolean throwIfFail)
      at System.DirectoryServices.DirectoryEntry.Bind()
      at System.DirectoryServices.DirectoryEntry.RefreshCache()
      at System.DirectoryServices.AccountManagement.UserPrincipal.FindByIdentity(PrincipalContext context, IdentityType identityType, String identityValue)
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(314,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.CreateUserIfNotPresent()
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(56,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests..ctor()

NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.ImpersonatingWrapperNegativeTest2 [FAIL]
   System.Runtime.InteropServices.COMException : The network path was not found.
   Stack Trace:
      at System.DirectoryServices.DirectoryEntry.Bind(Boolean throwIfFail)
      at System.DirectoryServices.DirectoryEntry.Bind()
      at System.DirectoryServices.DirectoryEntry.RefreshCache()
      at System.DirectoryServices.AccountManagement.UserPrincipal.FindByIdentity(PrincipalContext context, IdentityType identityType, String identityValue)
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(314,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.CreateUserIfNotPresent()
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(56,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests..ctor()

NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.RevertToSelfTest [FAIL]
   System.Runtime.InteropServices.COMException : The network path was not found.
   Stack Trace:
      at System.DirectoryServices.DirectoryEntry.Bind(Boolean throwIfFail)
      at System.DirectoryServices.DirectoryEntry.Bind()
      at System.DirectoryServices.DirectoryEntry.RefreshCache()
      at System.DirectoryServices.AccountManagement.UserPrincipal.FindByIdentity(PrincipalContext context, IdentityType identityType, String identityValue)
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(314,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.CreateUserIfNotPresent()
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(56,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests..ctor()

NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.ImpersonatingWrapperNegativeTest [FAIL]
   System.Runtime.InteropServices.COMException : The network path was not found.
   Stack Trace:
      at System.DirectoryServices.DirectoryEntry.Bind(Boolean throwIfFail)
      at System.DirectoryServices.DirectoryEntry.Bind()
      at System.DirectoryServices.DirectoryEntry.RefreshCache()
      at System.DirectoryServices.AccountManagement.UserPrincipal.FindByIdentity(PrincipalContext context, IdentityType identityType, String identityValue)
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(314,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests.CreateUserIfNotPresent()
   NLog.UnitTests\Targets\Wrappers\ImpersonatingTargetWrapperTests.cs(56,0): at NLog.UnitTests.Targets.Wrappers.ImpersonatingTargetWrapperTests..ctor()

snakefoot · 2017-05-29T18:07:46Z

Could you rebuild master again ? I have change the hostname from IPv4 127.0.0.1 to Environment.MachineName and now this PR builds. Just want to make sure if the fix is valid, or something has been fixed at appveyor

304NotModified · 2017-05-29T18:26:20Z

Queued!

snakefoot · 2017-05-29T18:29:01Z

Queued!

Master still fails, so I guess IPv4 127.0.0.1 is no longer working, and using Environment.MachineName fixes the issue.

304NotModified · 2017-05-29T20:15:39Z

src/NLog/LogFactory.cs

@@ -1260,11 +1260,11 @@ private void ConfigFileChanged(object sender, EventArgs args)
        /// <summary>
        /// Logger cache key.
        /// </summary>
-        internal class LoggerCacheKey : IEquatable<LoggerCacheKey>
+        internal struct LoggerCacheKey : IEquatable<LoggerCacheKey>


What's the idea of the struct? AFAIK it's not in line with https://msdn.microsoft.com/en-us/library/ms229017(v=vs.110).aspx ?

Looks very much in line:

are commonly embedded in other objects (Dictionary key)

It logically represents a single value (Its a key)

It has an instance size under 16 bytes (check)

It is immutable (check)

It will not have to be boxed frequently (check)

The reason is to avoid garbage when doing dictionary-lookup, which is right now done for every Trace-event.

you're right! was reading it incorrectly! 👍

…bject

snakefoot · 2017-05-29T21:34:12Z

@304NotModified Squashed after merge with #2140

304NotModified · 2017-05-29T21:39:21Z

thanks!

snakefoot force-pushed the NLogTraceListenerCheckLevel branch 9 times, most recently from 7e779c0 to 7cb1975 Compare May 29, 2017 05:10

304NotModified added the performance label May 29, 2017

snakefoot force-pushed the NLogTraceListenerCheckLevel branch from 7cb1975 to fbf2e8c Compare May 29, 2017 16:58

304NotModified mentioned this pull request May 29, 2017

Fix ImpersonatingTargetWrapper unit tests #2140

Merged

304NotModified reviewed May 29, 2017

View reviewed changes

snakefoot force-pushed the NLogTraceListenerCheckLevel branch 2 times, most recently from b166eb4 to ff3d23e Compare May 29, 2017 21:29

snakefoot added 2 commits May 29, 2017 23:32

NLogTraceListener - Check the LogLevel before creating LogEventInfo o…

af7c158

…bject

NLogTraceListener - Added support for TraceFilter

78e7b31

snakefoot force-pushed the NLogTraceListenerCheckLevel branch from ff3d23e to 78e7b31 Compare May 29, 2017 21:32

304NotModified approved these changes May 29, 2017

View reviewed changes

304NotModified added this to the 4.4.10 milestone May 30, 2017

304NotModified merged commit 482bbdd into NLog:master May 30, 2017

snakefoot mentioned this pull request Jun 15, 2017

Filter Trace Information #799

Closed

snakefoot deleted the NLogTraceListenerCheckLevel branch October 10, 2017 20:41

snakefoot mentioned this pull request Aug 25, 2018

Introduce maxLogLevel for NLogTraceListener #978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLogTraceListener - Reduce overhead by checking LogLevel #2137

NLogTraceListener - Reduce overhead by checking LogLevel #2137

snakefoot commented May 27, 2017 •

edited

Loading

codecov bot commented May 27, 2017 •

edited

Loading

snakefoot commented May 28, 2017 •

edited

Loading

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017 •

edited

Loading

304NotModified commented May 29, 2017

304NotModified commented May 29, 2017

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017 •

edited

Loading

snakefoot commented May 29, 2017 •

edited

Loading

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017

304NotModified May 29, 2017

snakefoot May 29, 2017 •

edited

Loading

304NotModified May 29, 2017

snakefoot commented May 29, 2017

304NotModified commented May 29, 2017

NLogTraceListener - Reduce overhead by checking LogLevel #2137

NLogTraceListener - Reduce overhead by checking LogLevel #2137

Conversation

snakefoot commented May 27, 2017 • edited Loading

codecov bot commented May 27, 2017 • edited Loading

Codecov Report

snakefoot commented May 28, 2017 • edited Loading

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017 • edited Loading

304NotModified commented May 29, 2017

304NotModified commented May 29, 2017

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017 • edited Loading

snakefoot commented May 29, 2017 • edited Loading

304NotModified commented May 29, 2017

snakefoot commented May 29, 2017

304NotModified May 29, 2017

Choose a reason for hiding this comment

snakefoot May 29, 2017 • edited Loading

Choose a reason for hiding this comment

304NotModified May 29, 2017

Choose a reason for hiding this comment

snakefoot commented May 29, 2017

304NotModified commented May 29, 2017

snakefoot commented May 27, 2017 •

edited

Loading

codecov bot commented May 27, 2017 •

edited

Loading

snakefoot commented May 28, 2017 •

edited

Loading

snakefoot commented May 29, 2017 •

edited

Loading

snakefoot commented May 29, 2017 •

edited

Loading

snakefoot commented May 29, 2017 •

edited

Loading

snakefoot May 29, 2017 •

edited

Loading