Logs exporter: flattening EventId into Id and Name causes avoidable attribute name conflicts

[nblumhardt]

Feature Request

EventId is part of .NET's stable log data model. Log events carry an event id, which may contain an Id for the log event (type), and an additional human-readable Name.

Currently, the OTLP exporter flattens these structure members into Id and Name attributes on resulting log records. These are extremely common attribute names to find in user log messages, for example:

logger.LogInformation(new EventId(42, "UserCreated"), "Created user {Name} with id {Id}", user.Name, user.Id);

generates two important attributes on the resulting log event, Name and Id, yet these will conflict with the EventId-derived Name and Id attached by the exporter.

Describe the solution you'd like:

Something akin to the way category names are handled could work? I.e.:

dotnet.ilogger.event_id.id
dotnet.ilogger.event_id.name

Describe alternatives you've considered.

--

Additional Context

See also #3491

[cijothomas]

This should be addressed before stable release, so adding to that milestone.

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/pull/4781/files Has removed exporting eventid completely to unblock stable release. Removing this issue from 1.6 milestone, but this issue is still open and need fix.

[cijothomas]

#4925 (comment) There is a desire to not use any language specific attribute name, instead propose this as an OTel Sem. Convention, so every other language can leverage this (only OTel CPP needs this currently. OTel Rust would need it in the future as well).

[paulomorgado]

How about just event_id and event_name?

[ghelyar]

There is an OpenTelemetry Events API that is related to Logs.

An Event is a specialized type of LogRecord, not a separate concept.

Which seems to line up - a log record with an event id/name is an event.

This defines event.name in its semantic conventions
https://opentelemetry.io/docs/specs/semconv/general/events/

While it's not stable yet, it seems like a reasonable name for it, and event.id could be used as well but is not defined by the spec.

The current names, hidden behind the OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES option, are logrecord.event.name and logrecord.event.id, so this would just remove the logrecord. prefix from those.

It looks like these current names were chosen just to unblock it and move forward and it also looks like there are plans to stabilise these so that they can be safely used but that seems to be making a temporary decision permanent without revisiting what it should be long term, and it seems like these names should really be OTel semantic conventions.

[cijothomas]

@ghelyar you are right. Leveraging Events convention was one of the suggestions. (Note that the whole notion of Events was not very clear until recently, so initially, there was reluctance to follow events convention. With recent clarification on Events purpose, it needs a revisit.)

Stabilizing the feature is not simply emitting the current values by default. It is figuring out the correct way to do it and then making that the default. The right way could very well be based on Event semantic convention.

[ghelyar]

Thanks for explaining @cijothomas

For what it's worth and in case it helps anyone else, this is what I'm currently doing in a processor that is added before the OTLP exporter, although it's a bit long winded due to LogRecord.Attributes being an IReadOnlyList:

internal sealed class EventIdLogProcessor : BaseProcessor<LogRecord>
{
    public override void OnEnd(LogRecord data)
    {
        var attributes = new List<KeyValuePair<string, object?>>(2 + (data.Attributes?.Count ?? 0));

        if (data.EventId.Id != default)
        {
            attributes.Add(new KeyValuePair<string, object?>("event.id", data.EventId.Id));
        }

        if (!string.IsNullOrEmpty(data.EventId.Name))
        {
            attributes.Add(new KeyValuePair<string, object?>("event.name", data.EventId.Name));
        }

        if (attributes.Count > 0)
        {
            if (data.Attributes is not null)
            {
                attributes.AddRange(data.Attributes);
            }
            data.Attributes = attributes;
        }
    }
}

[cijothomas]

Thanks. The above approach, which works, forces another List heap allocation/copying and is not acceptable for perf sensitive apps. :(

The open issue about semantic convention for a event.id field. Most discussions occurred on this while Events was unclear, so this also requires re-discussion!
open-telemetry/semantic-conventions#372