NLogBufferingTargetWrapperMiddleware for AspNetBufferingTargetWrapper

[bakgerman]

Resolves #304

• Instead of each AspNetBufferingTargetWrapper injects itself into HttpContext.Items, then HttpModule / HttpMiddleware prepares an anchor-node where each AspNetBufferingTargetWrapper can register itself.
  • To avoid modifying HttpContext.Items during logging (since not threadsafe), and avoid AspNetBufferingTargetWrapper having to know about HttpModule / HttpMiddleware.
• Introduces lazy allocation of request-buffering until target-logging actually occurs (possible performance benefit)

[snakefoot]

@bakgerman Have attached minimal-changes version of AspNetBufferingTargetWrapper.cs here (Should resolve most of my comments):

AspNetBufferingTargetWrapper.zip

Hopefully GitHub will recognize it as a standard move-operation with minimal changes.

[bakgerman]

Committed your version of AspNetBufferingTargetWrapper after reading the comments, changed NLogHttpModule to use OnEndRequest again

[snakefoot]

Looks like GitHub is happy now, and sees AspNetBufferingTargetWrapper.cs as being moved.

But also need to revert the breaking changes in NLogHttpModule.cs.

[bakgerman]

good comments, I will work on the unit tests later this week.

[bakgerman]

Thank you for pushing me for better unit tests. This did find 2 defects, one in null HttpContext case for NLogHttpModule, one for main success path in the core buffering target. DebugTarget and MemoryTarget were used to verify count of messages, that all all messages were written, and in proper order. A test case of 3 targets each having a memory target was the most complicated, and showed the messages in the proper order. When done with a valid HttpContext, the use of the buffer limit was proven. With a null HttpContext, the proof of skipping the buffer target and writing all messages even above the buffer limit was proven.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

90.8% Coverage
0.0% Duplication

[snakefoot]

Think the new test-cases looks great with asserting on the target-output, and not just probing the HttpContext-Items-Dictionary.

Unless you have other things you would like to add, then I think it is ready to merge (I will probably make a followup commit to merge the 2 test-classes into one common, and investigate not allocating HttpContextWrapper for every begin- / end-request.)

[snakefoot]

@bakgerman After the merge the total code-coverage moved from 80pct to 84 pct, so nice improvement.

I guess the wiki-page needs to be updated, about how to use it with NLog.Web.AspNetCore:

https://github.com/NLog/NLog/wiki/AspNetBufferingWrapper-target

And maybe also the SandCastle docs:

https://nlog-project.org/documentation/v5.0.0/html/T_NLog_Web_Targets_Wrappers_AspNetBufferingTargetWrapper.htm

[bakgerman]

Yes, I will update the wiki and SandCastle

[snakefoot]

Think we can skip the SandCastle-docs for now, and focus on the wiki-page.

Still curious if there are any users waiting for AspNetBufferingTargetWrapper on ASP.NET Core, but I really like the new unit-tests.

[bakgerman]

yes I am curious as well, i will focus on the wiki page.

[bakgerman]

wiki page is updated

[snakefoot]

Excellent. Lets see if anyone will try the new rocket :)