-
Notifications
You must be signed in to change notification settings - Fork 893
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
Change event.name
attribute into top-level event name field
#4320
Conversation
3f68017
to
e994c1b
Compare
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.
LGTM, I left a nonblocking comment.
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.
💯 Looks good!
(coming from .NET, C++, Rust clients where EventName was a top-level field all the time.)
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.
I like this design ❤️
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.
+1 for this. Rust tokio tracing library already has "event name" as top-level field. Would be easy to model this into otel-rust with this change.
@@ -137,6 +137,7 @@ The API MUST accept the following parameters: | |||
- [Severity Text](./data-model.md#field-severitytext) (optional) | |||
- [Body](./data-model.md#field-body) (optional) | |||
- [Attributes](./data-model.md#field-attributes) (optional) | |||
- **Status**: [Development](../document-status.md) - [Event Name](./data-model.md#event-name) (optional) |
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.
Looks like I'm late to the game, but here's some thoughts...
-
In .NET ILogger API we have CategoryName always and optional EventId structure (which contains Name & Id). So what goes here? EventId.Name may not be present and if it is, presumably, it is only unique the context of a CategoryName. So should this be prefixed? EventName = $"{CategoryName}.{EventName}" or do we also need something like EventNamespace defined?
-
Seems odd to introduce "Event" into the Log API given there is an "Event" API defined right below. Seems to invite confusion 😄 Why not just "Name" here?
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.
Seems odd to introduce "Event" into the Log API given there is an "Event" API defined right below
check out discussions at https://cloud-native.slack.com/archives/C062HUREGUV/p1733358994263159
since it's low level API it should support all logs features (including events), so it should take a name
and https://cloud-native.slack.com/archives/C062HUREGUV/p1733398117155609
The whole reason why the Go SIG was asking that the "Emit an Event" should be optional in the OTEP (see: open-telemetry/oteps#265 (comment)) is that after spending about a year designing the Logs API and SDK we felt that adding a separate method is a wrong decision.
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.
Why not just "Name" here?
it's going to map to new proto field event_name
(open-telemetry/opentelemetry-proto#600)
Fixes #4260
Changes
Converts event name from an attribute (
event.name
) into a top-level field.