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

Event payload lives in the LogRecord body #566

Merged
merged 26 commits into from
Feb 12, 2024

Conversation

breedx-splk
Copy link
Contributor

Changes

In the Events Working Group on 2023-11-17, we decided that the Event payload will exist in the Body of the LogRecord. 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

@breedx-splk breedx-splk requested review from a team November 30, 2023 01:44
@breedx-splk breedx-splk changed the title payload lives in the log body Event payload lives in the LogRecord body Nov 30, 2023
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
breedx-splk and others added 4 commits November 30, 2023 09:21
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>
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Show resolved Hide resolved
@trask
Copy link
Member

trask commented Jan 23, 2024

@trask
Copy link
Member

trask commented Jan 24, 2024

Adding reason for my approval:

Putting the payload in the LogRecord body makes a clear delineation between

  • attributes (metadata), and
  • body (payload)

(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

@yurishkuro
Copy link
Member

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.

@lmolkova
Copy link
Contributor

lmolkova commented Jan 26, 2024

I'd like to see a story for span events. It can look as simple as:

  • define event.payload attribute (for event semconv type which is span event) - this is a plain string
  • define other similar attributes for properties that Event has but span event doesn't (seem to be SeverityNumber only)

Then semconv authors would be able to describe simple events using schemas/mds the moment this PR is merged.
Exporters could start translating span events to events if they want to.

Events WG can keep working on defining the future of Event/Log semconv in parallel.
event.payload and any other mapping attributes can stay experimental forever and can be removed after the future is defined.

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:
as an alternative to supporting span-events, I'd love to see current event type repurposed to log events with a way to specify all Event properties and have an example of how to represent a simple blob-of-text payload in semconv.

@breedx-splk
Copy link
Contributor Author

I'd like to see a story for span events.

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?

@trask
Copy link
Member

trask commented Jan 29, 2024

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. :)

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"?

docs/general/events.md Show resolved Hide resolved
@breedx-splk
Copy link
Contributor Author

@jsuereth As requested, I have opened #695 to help track/discuss the span event vs. standalone event distinction. 🙏🏻 Thanks thanks.

@breedx-splk
Copy link
Contributor Author

Adding purely informative link to related spec PR.

@jsuereth jsuereth merged commit 85ea994 into open-telemetry:main Feb 12, 2024
10 checks passed
ChrsMark pushed a commit to ChrsMark/semantic-conventions that referenced this pull request Feb 21, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.