-
Notifications
You must be signed in to change notification settings - Fork 246
Add a way to get configuration section associated with logger provider #706
Conversation
namespace Microsoft.Extensions.Logging | ||
{ | ||
/// <inheritdoc /> | ||
public class LoggerProviderOptionsChangeTokenSource<TOptions, TProvider> : ConfigurationChangeTokenSource<TOptions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useful for provider implementations but not sure if it's worth having here.
{ | ||
internal class ProviderAliasUtilities | ||
{ | ||
private const string AliasAttibuteTypeFullName = "Microsoft.Extensions.Logging.ProviderAliasAttribute"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is by name so that 3rd party providers don't have to take an unneeded dependency ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of them cannot build against aspnetcore 2.0 (AI)
|
||
namespace Microsoft.Extensions.Logging.Configuration | ||
{ | ||
internal class LoggingConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so we don't assume that we can just take IE<IConfiguration>
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I don't want random IConfiguration's in DI, especially if they were passed into logging.
{ | ||
internal class LoggerProviderConfigurationFactory : ILoggerProviderConfigurationFactory | ||
{ | ||
private readonly IEnumerable<LoggingConfiguration> _configurations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty weird. Usually configuration is composed at the provider level not by using multiple IConfiguration instances. Why do we do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We allow composition of logging configurations for light-up scenarios (AI is adding another logging config file), so when you call AddConfiguration
multiple times filtering rules get flattened in one list. I just simulated the same behavior for configuration sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few design questions I'd like to discuss but this looks good overall.
And fix
IncludeScopes
. Templates would need to be updated to moveIncludeScopes
toConsole
section.