serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
313 stars 100 forks source link

Logging scope when using abstractions v3.0.0 #158

Closed ArthurHNL closed 4 years ago

ArthurHNL commented 4 years ago

On .NET Core 2.2 and Serilog.Extensions.Logging version 3.0.1:

Please fix this, this makes using Serilog with libraries that depend on Microsoft.Logging.Abstractions version 3.0.0 very difficult.

nblumhardt commented 4 years ago

Hi Arthur! Thanks for the report. This looks at first glance like it may be a project configuration problem, as far as I'm aware, the code in this repository doesn't use or reference NullScope at all (which is an internal type in the 2.x versions of MEL).

Do you see any package downgrade warnings on build?

Are you configuring this with UseSerilog() from either https://github.com/serilog/serilog-extensions-hosting or https://github.com/serilog/serilog-aspnetcore ? Thanks!

ArthurHNL commented 4 years ago

The soulution in which this problem occurs currently consists of a library project and a test project. All tests classes inherit from a base class that contains a dependency injection container where logging is set up like this:

namespace Test
{
    using System;
    using Microsoft.Extensions.DependencyInjection;
    using Microsoft.Extensions.Logging;
    using Serilog;
    using ILogger = Microsoft.Extensions.Logging.ILogger;

    /// <summary>
    /// Base class for tests.
    /// </summary>
    public abstract class BaseTest
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="BaseTest"/> class.
        /// </summary>
        /// <param name="serviceRegistrations">Service registrations.</param>
        protected BaseTest(Action<IServiceCollection> serviceRegistrations = default)
        {
            // Setup Serilog
            Log.Logger = new LoggerConfiguration()
                .MinimumLevel.Verbose()
                .Enrich.FromLogContext()
                .WriteTo.Console()
                .WriteTo.Debug()
                .CreateLogger();

            // Bootstrap dependency injection.
            var collection = new ServiceCollection()
                .AddLogging(loggingBuilder => loggingBuilder
                    .AddSerilog(Log.Logger, true))
                .AddSingleton(sp => sp.GetService<ILoggerFactory>().CreateLogger("TEST"));

            serviceRegistrations?.Invoke(collection);

            this.ServiceProvider = collection.BuildServiceProvider();
        }

        /// <summary>
        /// Gets the service provider.
        /// </summary>
        protected ServiceProvider ServiceProvider { get; }

        /// <summary>
        /// Gets the logger.
        /// </summary>
        protected ILogger Logger => this.ServiceProvider.GetService<ILogger>();
    }
}

The test project has the following logging-related dependencies:

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Serilog" Version="2.9.0" />
    <PackageReference Include="Serilog.Extensions.Logging" Version="3.0.1" />
    <PackageReference Include="Serilog.Sinks.Console" Version="3.1.1" />
    <PackageReference Include="Serilog.Sinks.Debug" Version="1.0.1" />
  </ItemGroup>

The actual library only uses Microsoft.Extensions.Logging.Abstractions by design to ensure that it is as portable as possible:

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="3.0.0" />
  </ItemGroup>

And the library does use the ILogger.BeginScope(string) method to create scopes.

Whenever the dependency in the library is set to version 2.2.0, everything works fine:

    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.2.0" />
> dotnet test
Test run for Test.dll(.NETCoreApp,Version=v2.2)
Microsoft (R) Test Execution Command Line Tool Version 16.3.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

A total of 1 test files matched the specified pattern.

Test Run Successful.
Total tests: 171
     Passed: 171
 Total time: 4,0641 Seconds

However, if the dependency in the library is set to version 3.0.0, all hell breaks loose:

    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.2.0" />
> dotnet test
Test run for Test.dll(.NETCoreApp,Version=v2.2)
Microsoft (R) Test Execution Command Line Tool Version 16.3.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

A total of 1 test files matched the specified pattern.
  X TestA [190ms]
  Error Message:
   System.TypeLoadException : Could not load type 'Microsoft.Extensions.Logging.Abstractions.Internal.NullScope' from assembly 'Microsoft.Extensions.Logging.Abstractions, Version=3.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'.
  Stack Trace:
     at Microsoft.Extensions.Logging.Logger.BeginScope[TState](TState state)
     // Rest of stack trace hidden

Test Run Failed.
Total tests: 171
     Passed: 167
     Failed: 4
 Total time: 3,3529 Seconds

On build / restore, there are no package warnings for any of these situations.

ArthurHNL commented 4 years ago

After all, it could be a problem with Microsoft.Extensions.Logging.Abstractions, the Serilog project depends on version 2.2.0 so using version 3.0.0 does mean that there are some breaking changes, assuming that the sementic versioning scheme is followed in a correct manner.

nblumhardt commented 4 years ago

Yes, seems like in this setup, you end up with a mix of 2.x and 3.x Microsoft.Extensions.Logging* packages, when these two have internal dependencies set up between them.

Adding a Microsoft.Extensions.Logging 3.0 dependency to the test project should fix it.

Serilog.Extensions.Logging will run successfully on either version - it sticks to the public API surface and the maintainers haven't broken it between versions yet. But the app will still need to be configured to specify matching versions of these packages for them to work together (you'd get the same error using mixed versions, even without Serilog in the mix).

ArthurHNL commented 4 years ago

@nblumhardt Adding an explicit dependency on Microsoft.Extensions.logging.Abstractions version 3.0.0 for both the library and the test project results in the same TypeLoadException. It also brings up the question why the maintainers of the Microsoft Logging package bumped up the major version when there were no breaking API changes.

sungam3r commented 4 years ago

They (.NET Core / ASP.NET Core teams) have such a versioning policy. When new versions of the platform are released, all packages are recompiled and receive the same version (now 3.0.0). This is on the one hand even more convenient.

Numpsy commented 4 years ago

@nblumhardt Adding an explicit dependency on Microsoft.Extensions.logging.Abstractions version 3.0.0 for both the library and the test project...

Think the suggestion is that you need to add v3 of Microsoft.Extensions.Logging to the test project rather than the Abstractions package, otherwise you might end up with v2.2 of Microsoft.Extensions.Logging and v3.0 of Microsoft.Extensions.logging.Abstractions, which aren't compatible?

ArthurHNL commented 4 years ago

Adding a dependency on Microsoft.Extensions.Logging, version 3.0.0 on the test project does indeed fix the problem without any package warnings, thanks for all the effort!

It might still be useful to upgrade the dependency of this library on Microsoft.Extensions.Logging to the latest version: https://github.com/serilog/serilog-extensions-logging/blob/9f5a6a14a3059fcf9bf8e100aec32fd0d3967e9d/src/Serilog.Extensions.Logging/Serilog.Extensions.Logging.csproj#L32. Feel free to close this issue though.

nblumhardt commented 4 years ago

Great, thanks for the follow up @ArthurHNL, and the help, @Numpsy (edit: oh, and @sungam3r!) 👍

Currently the package here can serve the most users by keeping 2.x compatibility; we might do as you suggest and bump the dependency version in the future, but for now I think we should maintain the status quo while 2.x is most popular out in the wild. Thanks again for the input!