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

Added AspNet-Request-ConnectionId and AspNet-Response-Has-Started #796

Merged
merged 14 commits into from
Jun 11, 2022

Conversation

bakgerman
Copy link
Contributor

@bakgerman bakgerman commented Jun 9, 2022

Added render for (Core only) HttpContext.Connection.Id
Added render for both Core and Framework for HttpResponse HeadersWritten(Framework) and HasStarted(Core) named AspNetResponseHeadersWrittenLayoutRenderer
Added public bool Verbose option for ClientCertificate renderer
Added extension methods for HttpContext for TryGetConnection and TryGetWebSocket

@snakefoot
Copy link
Contributor

snakefoot commented Jun 9, 2022

Though it seems tempting, then you cannot repair past mistakes unless doing a major-version-bump (Ex. NLog.Web v6)

Please revert class-renames, then we can make an issue about fixing this with v6.

@bakgerman
Copy link
Contributor Author

Yes I will revert the class names commits and keep them local.

@bakgerman
Copy link
Contributor Author

Though it seems tempting, then you cannot repair past mistakes unless doing a major-version-bump (Ex. NLog.Web v6)

Please revert class-renames, then we can make an issue about fixing this with v6.

reverted as requested, kept changes local to my laptop

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #796 (c93f694) into master (0b1f2a8) will increase coverage by 2%.
The diff coverage is 82%.

❗ Current head c93f694 differs from pull request most recent head 99b7342. Consider uploading reports for the commit 99b7342 to get more accurate results

@@          Coverage Diff           @@
##           master   #796    +/-   ##
======================================
+ Coverage      67%    69%    +2%     
======================================
  Files          60     62     +2     
  Lines        1011   1141   +130     
  Branches      249    284    +35     
======================================
+ Hits          680    787   +107     
- Misses        225    236    +11     
- Partials      106    118    +12     
Impacted Files Coverage Δ
src/Shared/Internal/HttpContextExtensions.cs 61% <ø> (+7%) ⬆️
...enderers/AspNetRequestIsWebSocketLayoutRenderer.cs 100% <ø> (ø)
...outRenderers/AspNetRequestLocalIpLayoutRenderer.cs 67% <ø> (ø)
...tRenderers/AspNetRequestLocalPortLayoutRenderer.cs 67% <ø> (ø)
...Renderers/AspNetRequestRemotePortLayoutRenderer.cs 67% <ø> (ø)
...equestWebSocketRequestedProtocolsLayoutRenderer.cs 78% <ø> (ø)
...rs/AspNetRequestClientCertificateLayoutRenderer.cs 57% <50%> (+7%) ⬆️
...d/LayoutRenderers/AspNetUserClaimLayoutRenderer.cs 62% <83%> (ø)
...rers/AspNetResponseHeadersWrittenLayoutRenderer.cs 100% <100%> (ø)
...utRenderers/AspNetRequestDurationLayoutRenderer.cs 56% <0%> (-9%) ⬇️
... and 4 more

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 3c5e983...99b7342. Read the comment docs.

@bakgerman bakgerman changed the title Cleanup and 1 layout render 2 extension methods and 1 layout render Jun 9, 2022
@bakgerman bakgerman changed the title 2 extension methods and 1 layout render 2 extension methods and 2 layout render Jun 10, 2022
@bakgerman bakgerman changed the title 2 extension methods and 2 layout render 2 extension methods and 2 layout renderers Jun 10, 2022
@bakgerman
Copy link
Contributor Author

w3c layout unit test fail again, but this should be able to be pulled successfully.

@snakefoot snakefoot changed the title 2 extension methods and 2 layout renderers Added AspNet-Request-ConnectionId and AspNet-Response-Headers-Written Jun 10, 2022
@snakefoot snakefoot changed the title Added AspNet-Request-ConnectionId and AspNet-Response-Headers-Written Added AspNet-Request-ConnectionId and AspNet-Response-Has-Started Jun 10, 2022
@bakgerman
Copy link
Contributor Author

After this I cannot think of more NLog.Web layout renderers. Hopefully someone will find them useful. We are not trying to be equal to Visual Studio debugger in NLog, but I just try to find properties of HttpContext that we did not have already, that might be necessary to be logged.

@sonarqubecloud
Copy link

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

88.5% 88.5% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit 4eb86e4 into NLog:master Jun 11, 2022
@snakefoot snakefoot added this to the 5.0.1 milestone Jun 11, 2022
@bakgerman bakgerman deleted the AddtionalCoreOnlyLayoutRenders branch June 11, 2022 17:51
@snakefoot
Copy link
Contributor

w3c layout unit test fail again, but this should be able to be pulled successfully.

Thank you for nudging. Have tried to make W3CLoggerLayoutTests more stable, by replacing finalizer with dispose and forced AccurateUtcTimeSource

@snakefoot
Copy link
Contributor

snakefoot commented Jul 6, 2022

Have now merged #813 that will ensure future class-names will always end with "LayoutRenderer" (And matches their type-alias)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants