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

Restore aspnet-request-posted-body with middleware #781

Merged
merged 25 commits into from
Jun 1, 2022

Conversation

bakgerman
Copy link
Contributor

@bakgerman bakgerman commented May 24, 2022

This restores aspnet-request-posted-body layout renderer using middleware for asp.net core and httpmodule for legacy asp.net

@bakgerman
Copy link
Contributor Author

bakgerman commented May 24, 2022

Attempt to restore aspnet-request-posted-body layout renderer in NLog.Web 5.0.
The asp.net core middleware is based on a middleware i have had live for over a year.
The request body is then saved to HttpContext.Items with a key prefixed with __nlog_xxxxxx
This is in response to my question on
stackoverflow.com

@bakgerman bakgerman closed this May 24, 2022
@bakgerman bakgerman reopened this May 24, 2022
@bakgerman bakgerman changed the title Hopefully restore aspnet-request-posted-body using middleware approac… Restore aspnet-request-posted-body with middleware May 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #781 (8603c6d) into master (9fd5bd9) will increase coverage by 3%.
The diff coverage is 85%.

@@          Coverage Diff          @@
##           master   #781   +/-   ##
=====================================
+ Coverage      63%    66%   +3%     
=====================================
  Files          48     49    +1     
  Lines         790    810   +20     
  Branches      196    201    +5     
=====================================
+ Hits          498    535   +37     
+ Misses        223    197   -26     
- Partials       69     78    +9     
Impacted Files Coverage Δ
...Renderers/AspNetRequestPostedBodyLayoutRenderer.cs 85% <85%> (ø)
src/NLog.Web/NLogRequestLoggingModule.cs 59% <0%> (+59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fd5bd9...8603c6d. Read the comment docs.

@snakefoot
Copy link
Contributor

Thank you for this great contribution. Really amazing that you include both NLog.Web (HttpModule) and NLog.Web.AspNetCore (Middleware). I like the idea of using delegates, that allows the users to plugin functionality without having to inherit and override methods.

Would it make sense if we created one AspNetCore-middleware for NLog, where one of the features is correctly capturing ${aspnet-request-posted-body} ? Was thinking to merge it together with NLogRequestLoggingMiddleware, so request-body capture is one extra feature (And change NLogRequestLoggingMiddleware so its current exception-/error-logging also become plugin-style).

Was also thinking to make the Enable-Buffering-Condition-Logic a little "smarter", since the EnableBuffering()-buffering is super expensive. So maybe check the ContentType / ContentLength before actually doing the buffering. And if the buffering is below 64 KByte then just use a byte-array to avoid using the heavy tmp-file-buffering implemented by EnableBuffering()

@bakgerman
Copy link
Contributor Author

bakgerman commented May 24, 2022

Thank you, I worked very hard on the PR. :)

If you want a single middleware class that is clearly the proper goal, I did not want to change an existing class since this is my first commit to NLog.Web, but yes i thought when I was doing the PR that having > 1 middleware is not the best idea.
Can you please explain 'become plugin style' in some more detail before I feel I can make the change?

I can do the EnableBuffering() only if > 64K change quickly, later today or tomorrow.
I will have to test it thru an actual http flow as I recall sometimes bad things happened if this was off, but that ight have been only for > 64K case.

I have used NLog heavily and would like to help enhance one of my favorite github projects.

@bakgerman
Copy link
Contributor Author

I examined the documentation for EnableBuffering(), it states a limit of 30K, should we try that?

"Ensure the can be read multiple times. Normally buffers request bodies in memory; writes requests larger than 30K bytes to disk."

@snakefoot
Copy link
Contributor

snakefoot commented May 24, 2022

If you want a single middleware class that is clearly the proper goal, I did not want to change an existing class since this is my first commit to NLog.Web

I was also guessing that was your approach (to keep things simple and isolated before interfering with existing classes).

Can you please explain 'become plugin style' in some more detail before I feel I can make the change?

You have created NLogRequestPostedBodyMiddlewareConfiguration where one can plugin custom delegates, that will be used instead of the default-delegate.

I was thinking a similar thing could be done for NLogRequestLoggingMiddleware, so it calls a plugin-method defined on NLogRequestLoggingOptions that decides how it should perform exception-/error-logging (when assigned null then it is skipped).

@bakgerman
Copy link
Contributor Author

"You have created NLogRequestPostedBodyMiddlewareConfiguration where one can plugin custom delegates, that will be used instead of the default-delegate.

I was thinking a similar thing could be done for NLogRequestLoggingMiddleware, so it calls a plugin-method defined on NLogRequestLoggingOptions that decides how it should perform exception-/error-logging (when assigned null then it is skipped)."

I see what you mean now.

For the EnableBuffering(), it seems like Microsoft has already done the in memory part for us:
"Normally buffers request bodies in memory; writes requests larger than 30K bytes to disk."

As I am using my middleware for rest and soap, i do not have a test case > 30K.
We did test the performance of the middleware and async NLog.Database logging, with the commercial DynaTrace tool, and it was found to not harm any performance.

@snakefoot
Copy link
Contributor

snakefoot commented May 24, 2022

For the EnableBuffering(), it seems like Microsoft has already done the in memory part for us:

"Normally buffers request bodies in memory; writes requests larger than 30K bytes to disk."

Then I think 30K should be the default-max-limit for NLog-midddleware, and if ContentLength is larger, then it will not attempt EnableBuffering().

We did test the performance of the middleware and async NLog.Database logging, with the commercial DynaTrace tool, and it was found to not harm any performance.

Very impressed.

@bakgerman
Copy link
Contributor Author

Well, I found that if I leave EnableBuffing() off, stream.CanSeek returns false and we cannot capture the request body since we have to reset Position property to zero, read to end, then rest the Position back to original. The unit test does not show this but actual end to end test does show this.

@bakgerman
Copy link
Contributor Author

ok I will make limit = 30k instead of 8 k

@bakgerman
Copy link
Contributor Author

Default limit changed from 8KB to 30KB per Microsoft comments on EnableBuffering() method.

@bakgerman
Copy link
Contributor Author

Will need help testing the IHttpModule version end to end, I do not have any legacy 4.6.2 Framework web application anymore at my team where I work.

@bakgerman
Copy link
Contributor Author

Will examine squeezing 2 middleware into 1 class later this week.

@bakgerman
Copy link
Contributor Author

I think I am ready for the next review. The last commit unit tests failed due to an unrelated nuget.org error and I am researching how to rerun the checks for the last commit.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

97.4% 97.4% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit 3aee9bb into NLog:master Jun 1, 2022
@snakefoot
Copy link
Contributor

snakefoot commented Jun 1, 2022

Thank you so much for being persistent and completing all my requests.

I will try to add the ASP.NET Classic support using HttpModule, at least as first attempt. See also #785

@bakgerman
Copy link
Contributor Author

Thank you for all of the reviews, I know you are very busy with all of the NLog work! If you need assistance with HttpModule, please let me know. Hopefully the code in the middleware gives a direction to develop the HttpModule. Microsoft makes it very hard to unit tests pre-ASP.NET Core middleware (meaning HttpModule) or HttpContext class. Maybe I can find a legacy Framework ASP.NET app at my employer that I can test with, my direct department does not but the departments we work with may have one.

@bakgerman bakgerman deleted the Middleware branch June 1, 2022 22:09
@snakefoot
Copy link
Contributor

snakefoot commented Jun 4, 2022

@bakgerman Would it make sense to also have a Content-Type filter, so only trying to handle posted-body when in expected-formats (Avoid images / binaries / blobs / etc.)

CharSet - Ex. Contains "charset" (Then it text of some kind)
application/json - Ex. Starts with "application/" and contains "json"
application/xml - Ex. Starts with "application/" and contains "xml"
application/xhtml - Ex. Starts with "application/" and contains "html"
text/javascript - Ex. Starts with "text/"
text/html - Ex. Starts with "text/"
text/plain - Ex. Starts with "text/"
text/xml - Ex. Starts with "text/"

Alternative use https://docs.microsoft.com/en-us/dotnet/api/system.net.http.headers.mediatypeheadervalue.tryparse

        if (!MediaTypeHeaderValue.TryParse(contentType, out var mediaType))
        {
            return false;
        }

        if (mediaType.Charset.HasValue)
        {
           return true;
        }

@snakefoot
Copy link
Contributor

snakefoot commented Jun 4, 2022

@bakgerman Created #787 with a very primitive ContentType-filter. Would it work for you?

@snakefoot snakefoot added this to the 5.0.1 milestone Jun 4, 2022
@snakefoot snakefoot added the ASP.NET Core ASP.NET Core - all versions label Jun 6, 2022
@snakefoot
Copy link
Contributor

Would it be possible for you to update this wiki-page, and describe the recommended way for enabling ${aspnet-request-posted-body} using middleware:

https://github.com/NLog/NLog/wiki/AspNet-Request-posted-body-layout-renderer

You are very wellcome to remove all the old noise about previous version/attempts, and just state that one must use NLog.Web.AspNetCore v5.1 (or newer).

@bakgerman
Copy link
Contributor Author

Certainly

@bakgerman
Copy link
Contributor Author

I have changed it as requested.

@snakefoot
Copy link
Contributor

snakefoot commented Jul 18, 2022 via email

@bakgerman
Copy link
Contributor Author

Sure.

Should we add the response body middleware as example to another page?

@bakgerman
Copy link
Contributor Author

Added a large section about the middleware options to the end of the page

@snakefoot
Copy link
Contributor

snakefoot commented Jul 18, 2022 via email

@bakgerman
Copy link
Contributor Author

Ok I will work on that next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Core ASP.NET Core - all versions feature size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants