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

SpanEvent and Event through LogRecord should be "unified" #3406

Open
jsuereth opened this issue Apr 17, 2023 · 10 comments
Open

SpanEvent and Event through LogRecord should be "unified" #3406

jsuereth opened this issue Apr 17, 2023 · 10 comments
Labels
spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory triage:accepted:needs-sponsor

Comments

@jsuereth
Copy link
Contributor

What are you trying to achieve?

Whether or not a user leverages SpanEvent or LogRecord to fire events should be orthogonal to the representation of the data. If I decide to add an event to a Span, then I should have similar data-model to when I fire off an "unadorned" event via the Event API.

What did you expect to see?

Right now there are fields on LogRecord that do not exist on SpanEvent:

  • SeverityText
  • SeverityNumber
  • Body

I believe this need to be added to (or have a specified encoding in) SpanEvent.

Additional context.

I think we should not try to workaround the issue in particular domains, like: #2926

Instead, allowing events to flow as SpanEvent or LogEvent should both be viable choices.

@jsuereth jsuereth added spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory labels Apr 17, 2023
@jkwatson
Copy link
Contributor

I don't think the as-of-yet-incomplete "event" API will support severities, or bodies. Just names and attributes, like our span events do.

@scheler
Copy link
Contributor

scheler commented Apr 18, 2023

  1. In Define the shape of a event by encapsulating the payload in event.data. #2926, we wanted to introduce the notion of payload for Events, via attribute event.data. It is for this payload that @jsuereth and @jack-berg recommend using LogRecord Body instead.
  2. The current Events API and Events SemConv do not talk about Severity. However, in case Events end up needing Severity the recommendation was to use the in-built fields in the LogRecord and not introduce new attributes. That is the reason for including SeverityNumber and SeverityText in this issue. Event API can continue to not have these until the use-cases emerge.
    @jsuereth ObservedTimestamp is another field in LogRecord that could be added to SpanEvent. Only the Context fields could be omitted.

@joaopgrassi
Copy link
Member

joaopgrassi commented Apr 18, 2023

Was it considered if we even need the SpanEvent in the long run? I already saw users getting confused on why "events" are shown in two places and questions as : When I use a span event vs when I use a log.

@jsuereth
Copy link
Contributor Author

@joaopgrassi I believe there was discussion here. The main (huge) difference right now is in how SAMPLING works between the two. Span Events are sampled exactly as Spans are. Log-based events tend to be sampled by severity / log-ingestion configuration.

I suspect we need to wait for the unification between these two channels before we could drop SpanEvents, and in practice I expect we'll have to deal with both for quite some time.

@tigrannajaryan
Copy link
Member

Duplicate/related to #622

@tedsuo
Copy link
Contributor

tedsuo commented Apr 26, 2023

Right now there are fields on LogRecord that do not exist on SpanEvent:

  • SeverityText
  • SeverityNumber
  • Body

Since SpanEvents are a name plus a bag of attributes, it is straight forward to make SpanEvents equivalent to logs. The SpanEvent.Name field can match the event.name log attribute. For log fields where there is no SpanEvent equivalent, we can define SpanEvent attributes using semantic conventions:

  • log.severity_text
  • log.severity_number
  • log.body
  • For writing non-event logs to SpanEvents, the SpanEvent.name can be log.

This would allow data recorded through either API to be emitted using either data model.

I do not recommend adding more fields to the SpanEvents data model, and I do not recommend adding additional API surface area for exposing additional fields on SpanEvents. This is due to the fact that tracing backends do not understand these fields, and would make no use of them.

Once we have a stable Event API and data model, I recommend that we deprecate the SpanEvents API. This will alleviate confusion. Instead of trying to explain why you would use one API versus another, we just say "SpanEvents are deprecated, use Events."

Under the hood, users can choose where they would like data to be recorded based on whether they have a unified backend, or a separate logging and tracing backend. The SDK can be configured to allow users to emit any data recorded through either API to either data model. This is where the above semantic conventions come into play: translating data from one model to another when you have two separate backends.

@jkwatson
Copy link
Contributor

Once we have a stable Event API and data model, I recommend that we deprecate the SpanEvents API. This will alleviate confusion. Instead of trying to explain why you would use one API versus another, we just say "SpanEvents are deprecated, use Events."

I don't know about this... A SpanEvent is explicitly associated to the span to which it is attached. An Event would only potentially be associated with a span via some unknown mechanism. Not all Events that are emitted in the course of a span's execution should be associated to the span, so how would we figure out when we do and do not make this association?

I do not recommend trying to make a SpanEvent and an Event the same thing. Although they are structurally similar, they are not semantically equivalent, and trying to force them to be the same will only lead to user confusion, I fear.

@trask
Copy link
Member

trask commented Apr 26, 2023

Not all Events that are emitted in the course of a span's execution should be associated to the span, so how would we figure out when we do and do not make this association?

it seems ok for the default to use the "current span" (you can still ignore the association in your backend event query), and you can opt-out with EventBuilder.setContext(Context.root())

Although they are structurally similar, they are not semantically equivalent

I agree with this. I think of SpanEvent as more semantically equivalent to a Log.

Instead of trying to explain why you would use one API versus another, we just say "SpanEvents are deprecated, use Events."

Maybe we refine this to be "SpanEvents are deprecated, use Events or Logs"?

@scheler
Copy link
Contributor

scheler commented Apr 27, 2023

For log fields where there is no SpanEvent equivalent, we can define SpanEvent attributes using semantic conventions:

  • log.severity_text
  • log.severity_number
  • log.body
  • For writing non-event logs to SpanEvents, the SpanEvent.name can be log.

What would be the type of value for the log.body attribute - any? So now it's between adding additional fields in SpanEvent vs allowing attribute values of type map?

This is due to the fact that tracing backends do not understand these fields, and would make no use of them.

I know very little about tracing backends, but do they understand SpanEvents as they exist today? Does adding additional fields make it worse?

@MSNev
Copy link

MSNev commented Aug 22, 2023

Going over this it seems to me that there is an argument for #2994 to remove the domain and just have the event.name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory triage:accepted:needs-sponsor
Projects
Development

No branches or pull requests

10 participants