[Performance] Boxing in OpenTelemetryLogger

[CodeBlanch]

We have State as an object on LogRecord:

    public sealed class LogRecord
    {
        public object State { get; }
    }

The main ILogger extensions (eg LogInformation, LogWarning, etc.) all flow through a single method which uses struct FormattedLogValues to store the state.

So that object State will cause a lot of boxing for users.

There are solutions to this, but it would be breaking. So @reyang @cijothomas @alanwest are we open to breaking the log API in order to fix this?

[reyang]

We cannot break the public API (in this case, it is the SDK package) within version 1.x.
What we can do is to introduce a StateStruct { get; } which does not require boxing, and update State { get; } to retrieve the struct.

[CodeBlanch]

We cannot break the public API (in this case, it is the SDK package) within version 1.x.

I figured you would say that 😄

What we can do is to introduce a StateStruct { get; } which does not require boxing, and update State { get; } to retrieve the struct.

I don't think that will work, because the ILogger interface is generic:

public void Log<TState> (Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, TState state, Exception exception, Func<TState,Exception,string> formatter);

I have a couple ideas let me mess with it and I'll reply back. Could we unseal LogRecord if needed? That wouldn't be breaking would it?

[reyang]

I don't think that will work, because the ILogger interface is generic

Looks like you're right. I will take a look at the code and see if I have other ideas.

I have a couple ideas let me mess with it and I'll reply back. Could we unseal LogRecord if needed? That wouldn't be breaking would it?

Yes we could! Unseal a class is not considered as a breaking change 😄

[CodeBlanch]

@reyang OK I pushed a potential solution to #1833 check it out and let me know what you think. It's tricky you really have to process the TState right in the Log call otherwise you will end up boxing it. I chatted with @cijothomas a bit on Slack he said (more or less) the plan was to always support scope on LogRecord so I decided to add scope + parsed state + message features on there and some options to control them. By default the options give you the behavior in 1.0. If you turn ParseStateValues ON, we will no longer box the state but instead convert it into an IReadOnlyList<KeyValuePair<string, object>> in an allocation-free way (for the common case).