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

Add ILogger support for TryRefreshAsync in .NET Core package #273

Merged
merged 10 commits into from Sep 8, 2021

Conversation

avanigupta
Copy link
Member

@avanigupta avanigupta commented Aug 17, 2021

This PR adds a new public property to the IConfigurationRefresher interface to support logging any exceptions that occur during refresh operations:
ILoggerFactory IConfigurationRefresher.LoggerFactory { get; set; }

Using ILoggerFactory with Dependency Injection

If you want to get your ILoggerFactory through dependency injection, the default ILoggerFactory will be added automatically when services.AddAzureAppConfiguration() is invoked in your ConfigureServices method. No code changes are needed.

Using ILoggerFactory without Dependency Injection

If you don't use dependency injection, you can set your own instance of ILoggerFactory on the IConfigurationRefresher instance.

IConfigurationRefresher refresher = null;
var config = new ConfigurationBuilder()
            .AddAzureAppConfiguration(options =>
            {
                options.ConfigureRefresh(refreshOptions =>
                {
                    refreshOptions.Register("Sentinel", true)
                        .SetCacheExpiration(TimeSpan.FromSeconds(30));
                })

                refresher = options.GetRefresher();
            })
            .Build();

refresher.LoggerFactory = _loggerFactory;
bool success = await refresher.TryRefreshAsync();

Logging Category
The category of refresh logs will be: Microsoft.Extensions.Configuration.AzureAppConfiguration.Refresh

Breaking Change
This is a breaking change because we added a new dependency on Microsoft.Extensions.Logging package and the public interface has changed.

The AspNetCore middleware doesn't need any special changes to support ILogger.

@jimmyca15
Copy link
Member

Instead of having ILogger as an argument of TryRefresh, can we have ILogger registered on our configuration provider?

@avanigupta
Copy link
Member Author

Instead of having ILogger as an argument of TryRefresh, can we have ILogger registered on our configuration provider?

Our configuration provider is created during app startup but dependency injection takes place after startup has completed. So AzureAppConfigurationProvider will not be able to get ILogger from DI. Or maybe I'm missing something? What is the benefit of registering ILogger with config provider instead of as an argument of TryRefreshAsync?

@jimmyca15
Copy link
Member

It is a bit of a chicken and egg problem in the context of asp.net core dependency injection. Passing ILogger to each invocation of TryRefreshAsync just seems a bit odd to me. I expect ILogger to be more of a state of the refresher object than an argument to operations on the refresher.

Having a settable logger property on IConfigurationRefresher would seem more reasonable to me, but having it on the provider itself sounds even better.

@jimmyca15
Copy link
Member

jimmyca15 commented Sep 2, 2021

Your PR description is great! The one thing I would add is to the part about dependency injection to call out that the logger factory will be wired up automatically as long as services.AddAzureAppConfiguration is called.

Also, the category that we log with.

@avanigupta avanigupta linked an issue Sep 2, 2021 that may be closed by this pull request
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.

Consider adding logging for TryRefreshAsync method
3 participants