Calling AddDataProtection before AddLogging pollutes service collection with NullLoggerFactory

[natemcmaster]

Order shouldn't matter. The call to AddLogging should win.

This was broken by #134.

    public class Program
    {
        public static void Main(string[] args)
        {
            // Call AddDataProtection first
            var services = new ServiceCollection()
                .AddDataProtection()
                .Services
                .AddLogging(config => config.AddConsole())
                .BuildServiceProvider();

            // Call AddLogging first
            var services2 = new ServiceCollection()
                .AddLogging(config => config.AddConsole())
                .AddDataProtection()
                .Services
                .BuildServiceProvider();

            Console.WriteLine(services.GetService<ILoggerFactory>().GetType().AssemblyQualifiedName);
            Console.WriteLine(services2.GetService<ILoggerFactory>().GetType().AssemblyQualifiedName);
        }
    }

output

Microsoft.Extensions.Logging.Abstractions.NullLoggerFactory, Microsoft.Extensions.Logging.Abstractions, Version=2.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60
Microsoft.Extensions.Logging.LoggerFactory, Microsoft.Extensions.Logging, Version=2.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60

cc @ajaybhargavb @BrennanConroy

[ajaybhargavb]

We use TryAddSingleton to add the NullLoggerFactory if there is no existing logger factory https://github.com/aspnet/DataProtection/blob/dev/src/Microsoft.AspNetCore.DataProtection/DataProtectionServiceCollectionExtensions.cs#L67. This was needed because having an ILoggerFactory service in DI was optional before the refactor and we just wanted to be consistent with that.
What do you suggest is the right fix here? If we don't add a NullLoggerFactory things will just blow up at places like these https://github.com/aspnet/DataProtection/blob/dev/src/Microsoft.AspNetCore.DataProtection/KeyManagement/XmlKeyManager.cs#L64

[natemcmaster]

We can create constructors that don't require an ILoggerFactory. DI will use the most appropriate constructor. Example of this pattern: HostingApplicationDiscriminator.cs

[natemcmaster]

Other options:

• AddLogging could call .Replace() when ILoggerFactory has a service descriptor with NullLoggingFactory already added
• Instead creating many duplicate constructors, we remove direct dependency on ILoggerFactory from DP types and wrap it with an internal service IDataProtectionTrace. This would forward calls to ILoggerFactory.
• Use default values in constructors. public XmlKeyManager(ILoggerFactory factory = null)

[ajaybhargavb]

AddLogging could call .Replace() when ILoggerFactory has a service descriptor with NullLoggingFactory already added

@BrennanConroy @pakrym Thoughts on this?

[pakrym]

I would prefer adding constructors that do not require logging and let DI figure it out.