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

Use LoggerMessage.Define overloads that disable the IsEnabled check #31332

Closed
gfoidl opened this issue Mar 28, 2021 · 5 comments · Fixed by #31562
Closed

Use LoggerMessage.Define overloads that disable the IsEnabled check #31332

gfoidl opened this issue Mar 28, 2021 · 5 comments · Fixed by #31562
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@gfoidl
Copy link
Member

gfoidl commented Mar 28, 2021

Depends on dotnet/runtime#45290

There are some places that benefit from this change.

Everywhere logger.IsEnabled is checked, can profit by removing the double-check.

Then there are some places where "heavier objects" need to be created, even if logging is disabled for that level.
(Can use this regex search).

Just from the first results-page (didn't check all / exhaustive) e.g.:

var formattedArguments = GetFormattedArguments(arguments);
_viewComponentExecuting(logger, context.ViewComponentDescriptor.DisplayName, formattedArguments, null);

@mkArtakMSFT mkArtakMSFT added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Mar 29, 2021
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Mar 29, 2021
@ghost
Copy link

ghost commented Mar 29, 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 Author

gfoidl commented Mar 30, 2021

The runtime-PR dotnet/runtime#50334 got merged.
When will this change be available here? (so I can start to work on this).

@davidfowl
Copy link
Member

Generally, you can look [here[(https://github.com/dotnet/aspnetcore/blob/main/eng/Version.Details.xml) to tell where any repo is at dependency wise. As of writing, we're here https://github.com/dotnet/aspnetcore/blob/d1403ac8ed19f9effd65062ab46e7bdaf37d8ea3/eng/Version.Details.xml#L282 for dotnet/runtime.

So we can do:

https://github.com/dotnet/runtime/commits/main?before=8cc37cb9d8405be0b15bdc8ae9cf2ddc31dbd998+35&branch=main

To see what's in there. The new APIs are available!

@gfoidl
Copy link
Member Author

gfoidl commented Apr 1, 2021

Great! I'll have a look into this over the weekend.

@davidfowl
Copy link
Member

I've assigned it to you @gfoidl

@davidfowl davidfowl linked a pull request Apr 7, 2021 that will close this issue
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2021
@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-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants