Introduce IBufferedLogger

[geeknoid]

Fixes #104129

[tarekgh]

@geeknoid LGTM. Thanks for getting this that fast. I left a minor comment there.

@noahfalk @samsp-msft any feedback?

[noahfalk]

@noahfalk @samsp-msft any feedback?

I think we should do the transform you suggested so that we can extend this in the future if needed, but otherwise LGTM!

EDIT: We also need to figure out what we're doing with EventSourceLoggerProvider and OpenTelemetry. At minimum we should make sure OTel folks are on-board with the design and its implications for their logger prior to taking this for review.

[tarekgh]

At minimum we should make sure OTel folks are on-board with the design and its implications for their logger prior to taking this for review.

@CodeBlanch was in the meeting. Will be good to understand the plan for any OTel logger for that though.

[samsp-msft]

@kalyanaj for FYI,
and open-telemetry/opentelemetry-specification#4044 for this group

[noahfalk]

LGTM

[CodeBlanch]

@geeknoid

A few comments...

• Should we add ActivityTraceFlags on here as well? OTel spec requires TraceId, SpanId, & TraceFlags for correlation: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#log-and-event-record-definition

• Suggest making attributes nullable...

  -  IReadOnlyList<KeyValuePair<string, object?>> Attributes
  +  IReadOnlyList<KeyValuePair<string, object?>>? Attributes

  Just thinking some logs won't have any attributes so the model should match.

• The abstract and virtual stuff feels a bit odd. For really a DTO could we do...

    public sealed class BufferedLogRecord
    {
        public required System.DateTimeOffset Timestamp { get; init; }
        public required Microsoft.Extensions.Logging.LogLevel LogLevel { get; init; }
        public required Microsoft.Extensions.Logging.EventId EventId { get; init; }
        public string? Exception { get; init; }
        public System.Diagnostics.ActivitySpanId? ActivitySpanId { get; init; }
        public System.Diagnostics.ActivityTraceId? ActivityTraceId { get; init;  }
        public int? ManagedThreadId { get; init; }
        public string? FormattedMessage { get; init; }
        public string? MessageTemplate { get; init; }
        public System.Collections.Generic.IReadOnlyList<System.Collections.Generic.KeyValuePair<string, object?>> Attributes { get; init; }
    }

[geeknoid]

@noahfalk Should we add trace flags? Just another prop, not a big deal...

I thought it's fine to have the attributes non-nullable, it would just point to an empty attribute list if there is nothing there.

As for the virtual/abstract duality, that was a result of refactoring and not paying attention. This is now all abstract. This means whomever implements this abstraction gets to decide how to manage the underlying storage of each property.

[noahfalk]

I'm happy to add trace flags if Blanch believes they would be useful.

[noahfalk]

The abstract and virtual stuff feels a bit odd. For really a DTO could we do...

The design goal behind this type currently is that it isn't an intermediate DTO, it is likely the type that was used for backing storage. If you guys want to change this into a DTO and copy all the property values in from some private implementation type that does the storage I'm fine with that too.