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

Try out new .NET 6 logging generator #32087

Closed
3 of 7 tasks
JamesNK opened this issue Apr 23, 2021 · 10 comments · Fixed by #40439
Closed
3 of 7 tasks

Try out new .NET 6 logging generator #32087

JamesNK opened this issue Apr 23, 2021 · 10 comments · Fixed by #40439
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions help wanted Up for grabs. We would accept a PR to help resolve this issue

Comments

@JamesNK
Copy link
Member

JamesNK commented Apr 23, 2021

.NET 6 has a feature to generate logging classes from attributes. Could reduce the amount of boilerplate in our logging types while also validating log event IDs aren't duplicated.

See dotnet/runtime#36022

We should try replacing some small static Log types with the new feature.

  • HttpAbstractions
  • Kestrel
  • Middleware
  • HttpSys
  • Mvc
  • Components
  • SignalR
@JamesNK JamesNK added help wanted Up for grabs. We would accept a PR to help resolve this issue area-runtime labels Apr 23, 2021
@davidfowl
Copy link
Member

I asked @maryamariyan to give it a try. I think there are some issues that will prevent full adoption but we should I aim to move.

cc @gfoidl

@gfoidl
Copy link
Member

gfoidl commented Apr 23, 2021

If @maryamariyan's try succeedes I'd like to broaden that usage here (this generator seems really cool).
Further it's a good opportunity to detect some potential flaws and improve that generator before shipping .NET 6.

@ghost
Copy link

ghost commented Apr 23, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@gfoidl
Copy link
Member

gfoidl commented Apr 26, 2021

I made a quick test with the logging generator and it looks very promising, so I believe we can use that for some logging that needs to written new or to be rewritten (like the Kestrel trace).

@alefranz
Copy link
Contributor

alefranz commented May 8, 2021

Looking forward for this. Let me know if you are looking for help.

@gfoidl
Copy link
Member

gfoidl commented May 8, 2021

ATM there are two (?) points that should get addressed before we can use the logging generator:

@alefranz
Copy link
Contributor

alefranz commented May 8, 2021

Thanks for the quick reply! Is it worth removing the help wanted label here and add it to the linked issues if/when appropriate?

@maryamariyan
Copy link
Member

Opened this issue dotnet/runtime#52549 to list up logging generator related issues. Not all need to be addressed right away.

@gfoidl I think dotnet/runtime#51927 depends on how we end up with the API review on dotnet/runtime#50913

@gfoidl
Copy link
Member

gfoidl commented May 10, 2021

IMO dotnet/runtime#51927 is quite independent from dotnet/runtime#50913. The logging generator can skip the IsEnabled-check if there's a LoggerMessage.Define overload, otherwise (in the fallback case) ignore that "hint" which is merely an optimization.

I know that dotnet/runtime#51927 needs to be reviewed too, but when touching the logging for Kestrel, etc. it would be nice that it can be done once and use the goodness from the new logging generator.
It looks very promising, just some tiny pieces are missing (linked issues / also your list).

pranavkm added a commit that referenced this issue Aug 31, 2021
* Update Components to use LoggerMessage

Contributes to #32087
@adityamandaleeka
Copy link
Member

Thanks @pranavkm for making a bunch of progress here. Moving to .NET 7 milestone to cover the remainder.

pranavkm pushed a commit that referenced this issue Jan 20, 2022
Updates the Mvc project to use the LoggerMessageAttribute for logging generation.

## Description

The following files contained duplicate log event ids which had to be renumbered:
- MvcCoreLoggerExtensions
- MvcRazorLoggerExtensions
- MvcViewFeaturesLoggerExtensions
- RazorRuntimeCompilationLoggerExtensions

Also fixes a typo in a method name in RazorRuntimeCompilationLoggerExtensions:
ViewCompilerInvalidingCompiledFile --> ViewCompilerInvalidatingCompiledFile

Contributes to #32087
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this issue Jan 22, 2022
Updates the Mvc project to use the LoggerMessageAttribute for logging generation.

## Description

The following files contained duplicate log event ids which had to be renumbered:
- MvcCoreLoggerExtensions
- MvcRazorLoggerExtensions
- MvcViewFeaturesLoggerExtensions
- RazorRuntimeCompilationLoggerExtensions

Also fixes a typo in a method name in RazorRuntimeCompilationLoggerExtensions:
ViewCompilerInvalidingCompiledFile --> ViewCompilerInvalidatingCompiledFile

Contributes to dotnet#32087
pranavkm added a commit that referenced this issue Feb 22, 2022
pranavkm added a commit that referenced this issue Feb 23, 2022
* Use LoggerMessageAttribute in more places

* Servers
* Antiforgery
* Missed Blazor and Security projects

Contributes to #32087
pranavkm added a commit that referenced this issue Feb 25, 2022
Contributes to #24055
Contributes to #32087

* This updates most of the repo to use LoggerMessageAttributes. MvcCoreLoggerExtensions is the only standout but it is fairly involved since it mixes logging from several types, re-uses logger ids etc
pranavkm added a commit to pranavkm/aspnetcore that referenced this issue Feb 27, 2022
MvcCoreLoggerExtensions includes the log messages for a whole slew of MVC types. Consequently
it re-uses event ids and is hard maintain. This PR simultaenously updates the type to use
LoggerMessageAttribute, while also attempts to move as many of the individual log statements to the
appropriate type.

This change unearthed incorrect reuse of event ids within the same type that has also been corrected by assigning
new ids.

Fixes dotnet#32087
pranavkm added a commit that referenced this issue Mar 1, 2022
MvcCoreLoggerExtensions includes the log messages for a whole slew of MVC types. Consequently
it re-uses event ids and is hard maintain. This PR simultaenously updates the type to use
LoggerMessageAttribute, while also attempts to move as many of the individual log statements to the
appropriate type.

This change unearthed incorrect reuse of event ids within the same type that has also been corrected by assigning
new ids.

Fixes #32087
@ghost ghost locked as resolved and limited conversation to collaborators Mar 31, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants