open-telemetry / opentelemetry-proto

OpenTelemetry protocol (OTLP) specification and Protobuf definitions
https://opentelemetry.io/docs/specs/otlp/
Apache License 2.0
606 stars 263 forks source link

Add `event_name` to logs proto #600

Open lmolkova opened 4 days ago

lmolkova commented 4 days ago

Based on the recent discussions in Logs and Spec SIGs, we'd like to bring the event_name back as a top-level property to logs proto definition.

See https://github.com/open-telemetry/opentelemetry-specification/issues/4260 for the context

Spec meeting notes (11/19/24)

Related:

cijothomas commented 4 days ago

If needing any help with prototype, we can use OTel Rust, C++, .NET's OTLP Exporter - all these 3 languages already have EventName in the LogRecord, but no place to put it in OTLP, so they are either ignored or stored as attributes.

Is there anything in particular we are looking for in the prototype? @tigrannajaryan

cijothomas commented 4 days ago

confirmation from all other SIGs that we are indeed adding the new field to Log data model.

As mentioned in the earlier comment, I can confirm that OTel .NET, C++, Rust already store EventName as a top-level field in their LogRecord.

OTel .NET EventId which is a struct with EventName and Id : https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Logs/LogRecord.cs#L218

OTel Rust EventName: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/logs/record.rs#L25-L27

OTel C++ : https://github.com/open-telemetry/opentelemetry-cpp/blob/main/sdk/src/logs/read_write_log_record.cc#L84

tigrannajaryan commented 4 days ago

Here is a possible checklist, extended from @lmolkova's list.

We want an implementation in at least one language of these features:

In the spec:

In the Collector:

There is probably more that I am missing.

I want to be confident we have good answers before we add a field to the proto, so these things need to be prototyped in implementations and added as unstable docs to the spec.

We have a similar situation in Entities SIG and need to add a new field to Resource. We have had a recent discussion in the SIG about how we would approach addition of new fields to Stable OTLP components. We have been doing analogous prototyping in multiple languages and in the Collector and defining specification. My goal is be consistent in what we require when adding new fields to a stable proto.

lmolkova commented 4 days ago

I don't mind following the process and happy to follow up with the data model and prototypes.

We should recognize though that event name problem is well-known and prototyping boils down to simple data structure update. Given that event.name attribute is experimental there is no back-compat story necessary at all which cuts that check list in half or more.

Plus based on today's and past discussions Logs SIG, spec, and language maintainers has already expressed their support for the change.

tigrannajaryan commented 3 days ago

We should recognize though that event name problem is well-known and prototyping boils down to simple data structure update.

Sounds good, let's keep it as simple as we can while gaining the confidence level needed.

To me the most important questions to answer is that we are certain about adding the field, about its type, its interoperability requirement with older OTLP versions and that we are not going to change our mind on these decisions.

tigrannajaryan commented 3 days ago

@lmolkova do add a bit more, I think you are right, since this is such a simple field we don’t need to see the implementations to know that it is possible to implement. I think it is sufficient that we simply describe how we think the new field will work in the SKDs and in the Collector and that likely should be enough to go ahead.