-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add event_name
to logs proto
#600
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is a Stable component in proto I think we need high confidence level before adding a field.
To me that means seeing prototypes with implementations that show how it works and demonstrate the dual-write path, etc and getting confirmation from multiple SIGs (language SIGs and logging/events SIG).
I am not opposed to the change, but I am going to block temporarily to prevent merging prematurely.
I believe the changes to proto need to come as the last step after we have the full confirmation from all other SIGs that we are indeed adding the new field to Log data model.
We also need to add the field to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md
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 Is there anything in particular we are looking for in the prototype? @tigrannajaryan |
As mentioned in the earlier comment, I can confirm that OTel .NET, C++, Rust already store EventName as a top-level field in their OTel .NET OTel Rust EventName: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/logs/record.rs#L25-L27 |
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. |
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 Plus based on today's and past discussions Logs SIG, spec, and language maintainers has already expressed their support for the change. |
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. |
@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. |
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 open-telemetry/opentelemetry-specification#4260 for the context
Spec meeting notes (11/19/24)
opentelemetry-proto/opentelemetry/proto/logs/v1/logs.proto
Line 196 in 3c2915c
Related: