-
Notifications
You must be signed in to change notification settings - Fork 174
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
Event payload lives in the LogRecord body #566
Conversation
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
nm, I see it's already in the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/703a0af47acac67775f42b9c86570eebb9283b21/specification/logs/event-api.md?plain=1#L98-L100 |
Adding reason for my approval: Putting the payload in the LogRecord body makes a clear delineation between
(ok, it's still a bit messy whether some fields should go in payload or metadata, but I think that discussion can happen when we start adding semantic conventions for specific events). Further (and importantly), we can then restrict the Event API to only support non-nested attributes, which allows us to bypass the can of worms in open-telemetry/opentelemetry-specification#2888 |
The Event API already says that payload is placed in the body. It also says event may have attributes. Neither the spec nor this PR specify what a "payload" is. |
I'd like to see a story for span events. It can look as simple as:
Then semconv authors would be able to describe simple events using schemas/mds the moment this PR is merged. Events WG can keep working on defining the future of Event/Log semconv in parallel. Span events would never be good enough to describe complicated structured events, and for this we can always refer developers to Events/Logs. But simple back-compat story is necessary anyway. Update: |
Thanks @lmolkova. A while back, @jsuereth also brought up span events as a potential point of confusion. Did you see Trask's comment up there? This page is intended only to describe stand-alone events, and makes no reference to Spans anywhere. The only reasonable way I could understand a user being confused between events and span events is if they came from a tracing background and just ignored the fact that span events have the word span in them. :) Do we really think this is a blocker for this PR, or is it something we can work on after to unify/clarify the relationship between events and span events in follow-up work? |
I think users will definitely be confused between events and span events 😅. We could probably continue discussing that in open-telemetry/opentelemetry-specification#3406, but maybe in the short-term (this PR), we could update the page title here from "Semantic Conventions for Events" to "Semantic Conventions for (log-based) Events"? |
70df848
to
5bab8a7
Compare
Adding purely informative link to related spec PR. |
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com> Co-authored-by: Joao Grassi <joao.grassi@dynatrace.com> Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com> Co-authored-by: Josh Suereth <joshuasuereth@google.com>
Changes
In the Events Working Group on 2023-11-17, we decided that the Event payload will exist in the
Body
of theLogRecord
. This provides that clarity and also provides some additional descriptive wording about what an event is.This also relates to open-telemetry/opentelemetry-specification#3772
Merge requirement checklist