Should BeginScopeImpl be a generic method

[davidfowl]

For all the same reasons that Log<T> is generic https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Extensions.Logging.Abstractions/ILogger.cs#L36.

/cc @lodejard @rynowak @Eilon

[benaadams]

Yes; or better lazy-factory based

e.g.

interface ILogScope : IReadOnlyList<KeyValuePair<string, object>>
{
    ...
}

IDisposable BeginScopeImpl<T>(Func<T, ILogScope> scopeFactory, T state);

[ejsmith]

Yes, please!

[lodejard]

Yeah, scope where T is value typed is the probably the last place to squeeze an allocation out of the logging system

[benaadams]

Also class types https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNetCore.Hosting/Internal/HostingLoggerExtensions.cs#L20

If that could change from

public static IDisposable RequestScope(this ILogger logger, HttpContext httpContext)
{
    return logger.BeginScopeImpl(new HostingLogScope(httpContext));
}

to

public static IDisposable RequestScope(this ILogger logger, HttpContext httpContext)
{
    return logger.BeginScopeImpl<HostingLogScope, HttpContext>(
        (state) => new HostingLogScope(state), 
        httpContext);
}

and https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Extensions.Logging/Logger.cs#L103-L115

from

public IDisposable BeginScopeImpl(object state)
{
    if (_loggers == null)
    {
        return _nullScope;
    }

    if (_loggers.Length == 1)
    {
        return _loggers[0].BeginScopeImpl(state);
    }

    var loggers = _loggers;

to

public IDisposable BeginScopeImpl<TState, TScope>(Func<TState, TScope> scopeFactory, TState state)
{
    if (_loggers == null)
    {
        return _nullScope;
    }

    return BeginScopeImpl( scopeFactory(state) );
}

public IDisposable BeginScopeImpl<T>(T state)
{
    if (_loggers == null)
    {
        return _nullScope;
    }

    if (_loggers.Length == 1)
    {
        return _loggers[0].BeginScopeImpl(state);
    }

    var loggers = _loggers;

Or similar...

[benaadams]

Hmm; first could be an extension method

IDisposable BeginScopeImpl<TState, TScope>(this ILogger logger, 
    Func<TState, TScope> scopeFactory, 
    TState state)

Though Logger._loggers would have to be internal or public for it to work

[davidfowl]

Does it really matter that much if we made HostingLogScope a struct? The amount of time you'll have 0 loggers is really a benchmark scenario 😄

[benaadams]

And if you use WebSockets currently as they create lots of fail spew

e.g. aspnet/WebSockets#63

[ejsmith]

This change is really more about consistency of a simple interface that is going to be used by a LOT of libs and probably become the new Common.Logging. So please keep that in mind and get these changes in to simplify the interface now before we are stuck with it for a LONG time.

Personally, I also hate that the method got renamed because the signature conflicted with an extension method that was added. If you change the method to @benaadams suggestion then you wouldn't have that conflict and we could go back to the nice simple and logical BeginScope method name.

I vote for this signature:

IDisposable BeginScope<TState, TScope>(Func<TState, TScope> scopeFactory, 
    TState state)

Then there can be extension methods that sit on top of it for all the common easy simple use cases.

[niemyjski]

I agree with @ejsmith

[lodejard]

Having TState which may be a value-type, without a Func callback, is better. Having a Func to avoid execution only makes sense if the TState is more expensive to construct than the TScope - and at the end of the say if the TState is a caller defined value-type with multiple fields, the TScope will also be a caller defined value-type with multiple fields.

So this would boil down to IDisposable BeginScope<TScope>(TScope scope) where you expect TScope to be boxed if loggers which track scope are added and in effect, and no allocations will happen otherwise.

[benaadams]

The Func<TState, TScope> variety is just an extra overload that would call though to
BeginScope<TScope>(TScope scope) only issue is it being on an interface and needing private data; so it couldn't be added after the fact.