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.

[ejsmith]

Yes, I just went through this exercise and if you use the scope factory approach it makes it easier to create extension methods on top of the interface.

[lodejard]

Guys, frankly unless you can think of a reason that TScope as a value-type doesn't give you zero allocations when you have no providers, I'm going to be honest and say there's almost no chance we'll add a method that with double-generic signature and extra Func argument when a single-generic signature without the Func does the job.

[benaadams]

Yes, it would give zero alloc when you have no providers; and it would resolve the issue I keep complaining about 👍

However, when there are loggers it might make it worse? Just some observations

There is an interest in Logging for a much wider .net use case than just aspnet - which likely also mean people will end up doing weird and wonderful things.

While you can easily create a valuetype shim that's mostly no cost. You can't use it to defer expensive execution since its a valuetype its not shared and if you had multiple loggers its deferred cost would now be incurred multiple times.

Looking at a specific examples (and it may be an exceptions rather than the rule)

HostingLogScope implements IReadOnlyList<KeyValuePair<string, object>>; if that interface is used changing it to a valuetype means it will be boxed on each use (not sure when its used - might not matter).

Currently ConsoleLogger will box scopes if they were value types on each write - so that would need a change.

[ejsmith]

I do very similar things in my logging for scopes. It is very common to create logging scopes with collections of property names and values that can be added to the logging output. If you use the scope factory approach then we can avoid doing any data transforms when the scope isn't actually used.

Also, if the method signature is just BeginScope<TState>(TState state), I believe that makes it a lot harder to have simple extension methods like BeginScope(string format, params object[] values) because it won't know what method to pick.

[lodejard]

While you can easily create a valuetype shim that's mostly no cost. You can't use it to defer expensive execution since its a valuetype its not shared and if you had multiple loggers its deferred cost would now be incurred multiple times.

Yeah, but if there's a T1/T2 pair with Func<T1,T2> then each logger provider will call the func, so it's also incurred multiple times. Unless the Func is also stateful and returns the same T2 each time, in which case it means there is a closure, so there's an allocation for the closure per scope even if the callback isn't called...

[BrennanConroy]

Fixed via 8387adb