Skip to content
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

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Conversation

lmolkova
Copy link
Contributor

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)

Related:

@lmolkova lmolkova requested a review from a team as a code owner November 19, 2024 19:56
Copy link
Member

@tigrannajaryan tigrannajaryan left a 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

@cijothomas
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

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

  • Dual publish: allow emitting event name in both places. Copy one from the other, don't require the user to set both. Help with transition.
  • Opt-in (or opt-out?) flag for dual publish. Should it be an SDK config, i.e. selectable by end user or an instrumentation config, selectable by instrumentation authors?
  • What's the deprecation policy for dual publishing?
  • LogProcessor to convert from one to another.

In the spec:

  • Describe the behaviors listed above in the spec so that all languages can implement them consistently.
  • Add the field to Log Data Model document.
  • Decide on deprecation policy for "event.name" attribute.
  • What's the recommendation for translating from/to OTLP to other formats?

In the Collector:

  • Does OTLP receiver accept both fields? Which one takes precedence? What's the choosing logic? Do we keep both if both are set or we reset one?
  • Does OTLP exporter set both fields even if only one is set in the pipeline?
  • Implement spec recommendation for translating to/from other formats.
  • Processors. Should reading LogRecord.EventName automagically return the value of Attributes["event.name"]? The opposite? None?
  • What's the deprecation policy for these behaviors in the Collector? Are they hard-coded or configurable?

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
Copy link
Contributor Author

lmolkova commented Nov 20, 2024

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
Copy link
Member

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
Copy link
Member

@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.

@reyang
Copy link
Member

reyang commented Nov 28, 2024

@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.

@tigrannajaryan does this mean you will approve/unblock the PR?

@lmolkova
Copy link
Contributor Author

I summarized the plan and work items in the open-telemetry/opentelemetry-specification#4260 (comment). Logs SIG is working on the individual pieces.

TL;DR - there is no backward compatibility or expectation that SDK or collector will need to support event.name attribute. Instrumentations may provide back-compat and users may write custom processors to populate one from another.

Let me know if anything is missing @tigrannajaryan.

@tigrannajaryan tigrannajaryan dismissed their stale review November 28, 2024 16:51

Transition plan is now posted

@tigrannajaryan
Copy link
Member

Thanks @lmolkova, looks mostly good, I only had one comment on the plan.

@lmolkova
Copy link
Contributor Author

The change was discussed at the SemConv and Maintainers calls on 12/9 and the Spec call on 12/10 including the breaking aspect of it. No pushback was received, so I'm going to merge it.

Note: event.name attribute is the contract between the instrumentation and consumer. API/SDK/proto would pass it around before or after this change for as long as instrumentation emits it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants