Obsolete old-style logging APIs

[pakrym]

Exploratory PR for obsoleting old logging APIs in 2.2 preparing for their removal in 3.0.

What is being obsolete:

1. ILoggerProvider extension methods for adding loggers
2. ILogger concrete implementations and classes that are part of implementation details
3. LoggerSettings classes that have Option class alternative.

/cc @muratg @Eilon

[davidfowl]

@Eilon can you review the messages?

[Eilon]

It would be best to follow our obsoletion guidelines: https://github.com/aspnet/AspNetCore/wiki/Engineering-guidelines#breaking-changes

[SimonCropp]

is there an upgrade guide for this change? or an associated doco PR i can look at?

[pakrym]

Filed https://github.com/aspnet/Docs/issues/9841

[muratg]

@SimonCropp You should be getting warnings if you're using obsolete APIs, with a recommendation to what to use instead.

[SimonCropp]

@muratg

take this piece of code

using (var loggerFactory = new LoggerFactory())
{
    loggerFactory.AddConsole();

the obsolete message you get is

'ConsoleLoggerExtensions.AddConsole(ILoggerFactory)' is obsolete: 'This method is obsolete and will be removed in a future version. The recommended alternative is AddConsole(this ILoggingBuilder builder).'

which in no way helps me get to what the new code should be

var serviceCollection = new ServiceCollection();

serviceCollection.AddLogging(loggingBuilder => { loggingBuilder.AddConsole(); });
using (var serviceProvider = serviceCollection.BuildServiceProvider())
using (var loggerFactory = serviceProvider.GetService<Microsoft.Extensions.Logging.ILoggerFactory>())
{

note that i also had to add a new nuget dependency Microsoft.Extensions.DependencyInjection and a new using statement using Microsoft.Extensions.DependencyInjection;

is there any chance of reverting this change. the new api is significantly more complex than the old one.

[SimonCropp]

see here for some of the potential friction it will cause https://github.com/Particular/docs.particular.net/pull/4162#discussion_r238890834

[ajcvickers]

@pakrym @muratg @Eilon @divega @davidfowl It feels like we can/should do a lot better here. We're not just obsoleting methods and telling people to call different methods. We're instead removing a whole way of using logging--i.e. without D.I.--when it was designed originally to explicitly support this way of using it. This is exactly the kind of break that causes problems for people--that is, we broke you, but it's in no way obvious what you need to do to recover. Compare this to what we strive for, which is, "this has changed; do this instead now."

Please keep in mind that this is an Extensions package, not an ASP.NET package, and as such must support programming patterns that are common in non ASP.NET apps. It's not common for people to use ASP.NET-style D.I. in non-ASP.NET apps.

I'm not saying we should keep the obsolete APIs. I am saying we should have a conceptually equivalent way of replacing code that current uses them.

[muratg]

@ajcvickers we had a chat with Pavel about this this morning.

@pakrym please file a bug to track potential approaches to this.

[pakrym]

While I agree that we might improve things for logging I would like to note that having Extension in the package name doesn't meen things work well without DI.

If you look at the following snippet of how to setup logging without DI 3 out of 4 lines id setting up Options (there was also a bug filed to make things easier to use options without DI https://github.com/aspnet/AspNetCore/issues/2391 but we decided not to implement it.)

var configureNamedOptions = new ConfigureNamedOptions<ConsoleLoggerOptions>("", null);
var optionsFactory = new OptionsFactory<ConsoleLoggerOptions>(new []{ configureNamedOptions }, Enumerable.Empty<IPostConfigureOptions<ConsoleLoggerOptions>>());
var optionsMonitor = new OptionsMonitor<ConsoleLoggerOptions>(optionsFactory, Enumerable.Empty<IOptionsChangeTokenSource<ConsoleLoggerOptions>>(), new OptionsCache<ConsoleLoggerOptions>());
var loggerFactory = new LoggerFactory(new[] { new ConsoleLoggerProvider(optionsMonitor) }, new LoggerFilterOptions { MinLevel = LogLevel.Information });

And if you want to see an extreme example of the same: https://github.com/aspnet/Logging/blob/master/src/Microsoft.Extensions.Logging.AzureAppServices/AzureAppServicesLoggerFactoryExtensions.cs#L120-L190

[SimonCropp]

IMO this change will cause a chicken and egg problem. Ideally the first thing setup at app start is logging. if u cant configure logging u should crash the process, since with no logging any future errors u get will be lost. now with this change u need to setup DI before logging, in fact u need the full container instance before you can resolve something to log to. This means if anything goes wrong in the DI setup (which can be some non trivial code in many apps) then where do those log entries go?

[pakrym]

@SimonCropp logging in DI change happened long ago in 2.0.

[SimonCropp]

@pakrym this makes me sad :( DI should consume the logging api. logging should not consume the DI api.

[SimonCropp]

irrespective of what MS logging does internally, i would still prefer the option of using the simplified config api

using (var loggerFactory = new LoggerFactory())
{
    loggerFactory.AddConsole();

so that the fact that MS logging uses DI internally can optionally be abstracted away from the API surface area

[davidfowl]

We’ll need to do something here. Let’s make an issue to track this for 3.0

[pakrym]

https://github.com/aspnet/Logging/issues/944

[SimonCropp]

thanks for considering this :)