Log provider improvements

[armando-rodriguez-cko]

• Log provider improvements
• Add test to LogProvider

[david-chandler-cko]

What about a test to reproduce the original issue as well? Something like this:

[Fact]
public async Task ShouldCreateASingleLoggerInstanceForMultipleConcurrentRequests()
{
    LogProvider.SetLogFactory(_loggerFactory);
    Type[] loggerTypes = new[] { typeof(LogProviderTests), typeof(AnotherTestClass), typeof(NoInitializedType) };
    Task<ILogger>[] createLoggerTasks = Enumerable.Range(1, 50)
        .Select(async index =>
        {
            int randomDelayMs = new Random().Next(1, 5);
            await Task.Delay(randomDelayMs);
            return await Task.FromResult(LogProvider.GetLogger(loggerTypes[index % loggerTypes.Length]));
        }).ToArray();
    ILogger[] loggers = await Task.WhenAll(createLoggerTasks);
    Assert.Equal(loggerTypes.Length, loggers.Distinct().Count());
}

[IanKemp]

Any movement on this?

[armando-rodriguez-cko]

Any movement on this?

I have this task in the current sprint, but if I don't have time, next sprint I will try to implement it.

[leandro-tylkovitch-cko]

Since the logger factory already caches the logger instances, I think this class could be simplified right down to the following:

public static class LogProvider
{
    private static ILoggerFactory _loggerFactory = new LoggerFactory();

    public static void SetLogFactory(ILoggerFactory factory)
    {
        _loggerFactory?.Dispose();
        _loggerFactory = factory;
    }

    public static ILogger GetLogger(Type loggerType)
    {
        return loggerType is null ? NullLogger.Instance : _loggerFactory.CreateLogger(loggerType);
    }
}

David,

I tried your approach but Integration Testing is failing. I've added your proposed Unit Test for ShouldCreateASingleLoggerInstanceForMultipleConcurrentRequests and it's working OK with the actual propposal

[david-chandler-cko]

Since the logger factory already caches the logger instances, I think this class could be simplified right down to the following:

public static class LogProvider
{
    private static ILoggerFactory _loggerFactory = new LoggerFactory();

    public static void SetLogFactory(ILoggerFactory factory)
    {
        _loggerFactory?.Dispose();
        _loggerFactory = factory;
    }

    public static ILogger GetLogger(Type loggerType)
    {
        return loggerType is null ? NullLogger.Instance : _loggerFactory.CreateLogger(loggerType);
    }
}

David,

I tried your approach but Integration Testing is failing. I've added your proposed Unit Test for ShouldCreateASingleLoggerInstanceForMultipleConcurrentRequests and it's working OK with the actual propposal

Do you have an example of the failure when you simplify the code? What's the integration test trying to do?

[david-chandler-cko]

I can't approve this as it's the code I suggested.

[david-chandler-cko]

A couple of points:

1. The suggested test got removed: https://github.com/checkout/checkout-sdk-net/pull/375#issuecomment-1980865741
2. The code seems fine, but I don't think I should approve this PR as it's just the code that I suggested should be pasted in.

[armando-rodriguez-cko]

A couple of points:

1. The suggested test got removed: Log provider improvements #375 (comment)
2. The code seems fine, but I don't think I should approve this PR as it's just the code that I suggested should be pasted in.

Yes I forgot this file, sorry