Skip to content

Conversation

@DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Apr 29, 2024

Fixes #10326

This change makes our AbstractLoggerFactory take a set of Lazy<ILoggerProvider> instances and only realize them the first time a message is logged.

🤔 I wonder if it would be possible to change our ILoggerProvider instances to declare their minimum LogLevel via MEF and import that as metadata? That would allow us to avoid instantiating all the logger providers just to call IsEnabled.

This change makes our AbstractLoggerFactory take a set of Lazy<ILoggerProvider> instances and only realize them the first time a message is logged.
@DustinCampbell DustinCampbell requested a review from a team as a code owner April 29, 2024 22:37
@ryzngard
Copy link
Contributor

🤔 I wonder if it would be possible to change our ILoggerProvider instances to declare their minimum LogLevel via MEF and import that as metadata? That would allow us to avoid instantiating all the logger providers just to call IsEnabled.

How would that work with runtime logging level change?

@DustinCampbell
Copy link
Member Author

DustinCampbell commented Apr 29, 2024

How would that work with runtime logging level change?

Yeah, maybe it couldn't be done. At least, I can't visualize how we'd make output window logger track the log level. I suppose we could make deeper changes to make the runtime log part of the core logger infrastructure.

Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. In the meeting I was thinking "We'll have to make an ILogger that represents a lazy set of providers", which would be a bunch of work, but turns out we already almost had that!

@ryzngard
Copy link
Contributor

Yeah, maybe it couldn't be done. At least, I can't visualize how we'd make output window logger track the log level. I suppose we could make deeper changes to make the runtime log part of the core logger infrastructure.

I wouldn't even be opposed to having specialized interfaces for IInformationLoggerProvider or something that could just lazy import whenever the log level meets the requirements. It's a bit verbose in a way but may be worth it

@DustinCampbell DustinCampbell requested a review from a team as a code owner April 30, 2024 18:05
@DustinCampbell
Copy link
Member Author

I wouldn't even be opposed to having specialized interfaces for IInformationLoggerProvider or something that could just lazy import whenever the log level meets the requirements. It's a bit verbose in a way but may be worth it

I went ahead and added support for exported ILoggerProviders to include a minimum log level. This is used to avoid instantiating an ILoggerProvider until a message is logged with an appropriate log level. At the moment, this just helps avoid instantiating the ActivityLogLoggerProvider, but it could be used for other things. Future work could add another bit to the MEF metadata that allows a provider to avoid being instantiated until a message is logged that is meets the Razor log level settings.

@DustinCampbell
Copy link
Member Author

@dotnet/razor-compiler: You shouldn't need to review this PR. The only compiler relevant change I made is to remove an unused method before realizing that it was in the shared library. 😄

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared code LGTM.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make logging lazy load

5 participants