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

Introduce Ergonomic EmitEvent API #4357

Open
trask opened this issue Jan 9, 2025 · 3 comments
Open

Introduce Ergonomic EmitEvent API #4357

trask opened this issue Jan 9, 2025 · 3 comments
Labels
sig-issue A specific SIG should look into this before discussing at the spec spec:logs Related to the specification/logs directory

Comments

@trask
Copy link
Member

trask commented Jan 9, 2025

As part of #4352, we are removing the EmitEvent API, since events can now be emitted using the generic Emit API and so the EmitEvent API isn't necessary for moving forward anymore.

We (Java SIG at least) would like to revisit introducing a more ergonomic Event API in the future once other parts of Events have settled down more (but potentially before we document on the website how to emit Events using the Java API).

We support #4352, because the EmitEvent API as specified today is not very ergonomic anyways, as it includes SeverityText and ObservedTimestamp which are more targeted at bridging.

We'd potentially like something more like the previous Event API: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.38.0/specification/logs/event-api.md#emit-event

cc @jack-berg

@trask trask added the spec:logs Related to the specification/logs directory label Jan 9, 2025
@pellared
Copy link
Member

pellared commented Jan 9, 2025

I am not sure if there is even a need for specifying an ergonomic API unless there are more SIGs interested in it.
AFAIK ergonomics enhancements are often language specific and allowed without bringing it to the specification.
On the other hand, I think that the JavaScript SIG may be also interested in it so it may be worth specifying.

@jack-berg
Copy link
Member

We support #4352, because the EmitEvent API as specified today is not very ergonomic anyways, as it includes SeverityText and ObservedTimestamp which are more targeted at bridging.

Also, and critically, the "emit log record" operation doesn't require the event name parameter. With the "emit log record" operation as the canonical way to emit events, its too easy to do the wrong thing. We'll end up with many more structured logs (and which are not events) that we imagined.

I'm trying to stay outside this convo as best I can, but my role as maintainer of opentelemetry-java brings me back in occasionally. Sorry about that - nobody likes a drive-by critic. But... while I'm here, might I suggest taking another look at the Introducing Events and Logs API OTEP and consider if this vision of a single "emit log record" operation aligns with the consensus that was hard-won in that OTEP. Specifically:

Logs have a mandatory severity level as a first-class parameter that events do not have, and events have a mandatory name that logs do not have. Further, logs typically have messages in string form and events have data in the form of key-value pairs. It is due to this that their API interface requirements are slightly different.

and..

However, it was noted that the Log Appender API is very similar to the API for Events and so instead of having API for Events and API for Log Appenders separately it was agreed to have one API for Events and Logs, and that the API for Logs is targeted only to Log Appenders

I'm not suggesting that the OTEP is binding. Rather, consider whether the separation of emitting a log and emitting an event was a critical part of the consensus. I.e. would the consensus have been reached if the current single "emit log record" operation had been proposed?

@svrnm svrnm added the sig-issue A specific SIG should look into this before discussing at the spec label Jan 13, 2025
@trask
Copy link
Member Author

trask commented Jan 14, 2025

Based on feedback in today's Spec SIG meeting I think it's worth defining this ergonomic API prior to stabilizing Events. I'm hoping consensus on the shape of this ergonomic API will become clearer once we get a bit further with semantic convention and instrumentation support for events (which can leverage the current minimal event API which relies on LogRecordBuilder.setEventName).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig-issue A specific SIG should look into this before discussing at the spec spec:logs Related to the specification/logs directory
Projects
Status: No status
Development

No branches or pull requests

4 participants