[feature request] Support populating new `LogRecord.EventName` from log record EventId

[lmolkova]

Package

OpenTelemetry

Is your feature request related to a problem?

Since LogRecord now has top-level EventName proto field, OTel should support mapping EventId.EventName provided to ILogger.Log.

What is the expected behavior?

This example

opentelemetry-dotnet/docs/logs/complex-objects/Program.cs

Lines 17 to 38 in b508b84

	var foodRecallNotice = new FoodRecallNotice
	{
	BrandName = "Contoso",
	ProductDescription = "Salads",
	ProductType = "Food & Beverages",
	RecallReasonDescription = "due to a possible health risk from Listeria monocytogenes",
	CompanyName = "Contoso Fresh Vegetables, Inc.",
	};
	logger.FoodRecallNotice(foodRecallNotice);
	// Dispose logger factory before the application ends.
	// This will flush the remaining logs and shutdown the logging pipeline.
	loggerFactory.Dispose();
	internal static partial class LoggerExtensions
	{
	[LoggerMessage(LogLevel.Critical)]
	public static partial void FoodRecallNotice(
	this ILogger logger,
	[LogProperties(OmitReferenceName = true)] in FoodRecallNotice foodRecallNotice);
	}

should produce a LogRecord (over OTLP) with EventName property set instead of event.name.

Which alternative solutions or features have you considered?

n/a

Additional context

No response

[cijothomas]

#5462 (comment)

[alanwest]

@lmolkova I'm not very familiar with how ILogger's EventId is used and whether it matches OpenTelemetry's notion of an event. Can you share a little about why you think they are a good match?

I suspect there may be many usages of ILogger's EventId out there in the wild that does not conform to the event conventions, specifically:

The event name uniquely identifies event structure: the set of attributes and the type of the body.

What's your opinion, is this a concern for you?

Also, in this week's SIG meeting this issue was shared. There seems to be discussion internal to Microsoft about the potential of introducing an event-specific ILogger API. A concern I have here is if ILogger introduces an API for supporting OpenTelemetry events will we have painted ourselves into an awkward corner by also supporting events using the existing ILogger APIs?

I could be convinced that supporting LogRecord.EventName from ILogger's EventId is the correct step towards supporting OpenTelemetry events, but I just want to get more of your thoughts first.

[cijothomas]

I suspect there may be many usages of ILogger's EventId out there in the wild that does not conform to the event conventions, specifically:

The event name uniquely identifies event structure: the set of attributes and the type of the body.

This is not a concern in my view. Neither Ilogger nor OTelLog has any way of enforcing that. In a normal, well-behaved application, Events/Logs with a given EventName will have predictable shape of attributes/body. If a user follows our own logging doc, they'd be getting this for free. The EventName will be "FoodRecallNotice" and its body/attributes will be predictable. It takes an explicit action by the developer to break it.

A concern I have here is if ILogger introduces an API for supporting OpenTelemetry events will we have painted ourselves into an awkward corner by also supporting events using the existing ILogger APIs?

+1. And I don't think ILogger should be introducing a totally new API just for support Events, as Events are already well-supported by ILogger.

[alanwest]

This is not a concern in my view. Neither Ilogger nor OTelLog has any way of enforcing that.

👍 I agree this seems fine.

I don't think ILogger should be introducing a totally new API just for support Events, as Events are already well-supported by ILogger.

One way in which OpenTelemetry events may not be well supported by ILogger events is that the primary key between the two differ. For OpenTelemetry LogRecord.EventName is the identifying key, but with ILogger it's EventId.Id not EventId.Name because name is simply considered an optional description. So, for example you could have:

[LoggerMessage(EventId = 10, EventName ="Recall Notice", Message ="This is a food recall notice for: {food}")]
public static partial void LogFoodRecallNotice(this ILogger log, string foo, int bar);

[LoggerMessage(EventId = 20, EventName ="Recall Notice", Message ="This is a car recall notice for: {car}")]
public static partial void LogCarRecallNotice(this ILogger log, string foo, int bar);

These are two distinct types of events in ILogger, but they'd be considered the same in OpenTelemetry since the proposal is that we only include EventId.Name in LogRecord.EventName. Should we instead consider something like setting LogRecord.EventName to the concatenation of EventId.Id and EventId.Name?

@lmolkova has a comment in her example on this issue

EventId() eventId, // () modified to encourage fully-qualified human-readable name

What does this mean? Is the ILogger team considering changing EventId in some way? Or does "encourage" simply mean better documentation for how to use the EventId emphasizing that the name field should be identifying rather than just an optional description?

[cijothomas]

These are two distinct types of events in ILogger, but they'd be considered the same in OpenTelemetry since the proposal is that we only include EventId.Name in LogRecord.EventName. Should we instead consider something like setting LogRecord.EventName to the concatenation of EventId.Id and EventId.Name?

Excellent point. If user by-hand sets EventName like the example you showed, yes then it becomes their responsibility to put unique name (EventId.Name), not just the EventId.Id. The fact that ILogger uses just the "Id" part for equality has already caused confusion in the past, but given the history, it is less likely to get changed.

LogRecord.EventName to the concatenation of EventId.Id and EventId.Name

I haven't fully thought through the implications of this. It may look odd to see LogRecord.EventName as "344095174. FoodPriceChanged". (The auto-generated id is some hash applied over the FQN)

[alanwest]

It may look odd to see LogRecord.EventName as "344095174. FoodPriceChanged".

Yea I agree, I'm not excited at all about this suggestion, but I had to put it out there anyways 😄.

Maybe what we need is the ability for the user to configure the LoggerProvider how to derive event name.

builder.Services.AddOpenTelemetry()
    .WithLogging(logging => logging
        .EventNameResolver(logRecord => logRecord.EventId.Name)
        .AddOtlpExporter());

This would allow existing systems which may only leverage EventId.Id leaving EventId.Name blank to implement more complex logic to bridge their solution to OpenTelemetry.

builder.Services.AddOpenTelemetry()
    .WithLogging(logging => logging
        .EventNameResolver(logRecord => {
                if (logRecord.EventId.Id == 1) return "recall_notice.food"
                if (logRecord.EventId.Id == 2) return "recall_notice.car"
                ...
         }
        .AddOtlpExporter());

The resolver could be evaluated at export so as not to slow down the hot path.

[cijothomas]

Maybe what we need is the ability for the user to configure the LoggerProvider how to derive event name.

Yes that sounds reasonable. Maybe do ILogger.EventId.Name as default, and users can override it. Instead of callback (which could be accidentally misused), it's safer to offer just an Enum - 1. EventId.Name (default) 2. EventId.Id 3. EventId.Id+Name, and so on.

(Co-incidentally, we are adding a related/similar thing in OTel Rust, precisely for the event-name - a callback for users to determine EventName from the OTelLogRecord.)

[CodeBlanch]

Full disclosure I have not read everything. @alanwest pinged me on Slack so I just wanted to drop some thoughts 😄

When it comes to Event.Id & Event.Name ya runtime doesn't guarantee anything about uniqueness. I think it is even worse than what is above. Consider...

public static Library1Events
{
   public static EventId Started { get; } = new(1);
   public static EventId Recalled { get; } = new(2, "Recall");
}

public static Library2Events
{
   public static EventId SaleNotice { get; } = new(1);
   public static EventId RecallNotice { get; } = new(2, "Recall");
}

logger.Log(...eventId: Library1Events.Started...)
logger.Log(...eventId: Library1Events.Recalled...)
logger.Log(...eventId: Library2Events.SaleNotice...)
logger.Log(...eventId: Library2Events.RecallNotice...)

That log stream is going to be all kinds of weirdness. Feels to me EventId really should have something like a namespace 🤷

Regarding the API proposal...

builder.Services.AddOpenTelemetry()
    .WithLogging(logging => logging
        .EventNameResolver(logRecord => logRecord.EventId.Name)
        .AddOtlpExporter());

There technically is a spot for things specific to the ILogger integration. It could be:

builder.Services.AddOpenTelemetry()
    .WithLogging(
       logging => logging.AddOtlpExporter(),
       ilogger => ilogger.EventNameResolver(logRecord => logRecord.EventId.Name));

Which is to say EventNameResolver would be part of OpenTelemetryLoggerOptions.