sdk-trace: add EventData

[iRevive]

Reference	Link
Spec	https://opentelemetry.io/docs/specs/otel/trace/api/#add-events
Java implementation	EventData.java

Note

An Event is structurally defined by the following properties:

• Name of the event.
• A timestamp for the event. Either the time at which the event was added or a custom timestamp provided by the user.
• Zero or more Attributes further describing the event.

[armanbilge]

We often use a FiniteDuration in these contexts e.g. Clock#realTime.

[armanbilge]

I feel like there's a lot of inefficient conversions here: toList, .result(), : _* ...

It can be a follow-up, but I wonder if we can either have a mutable AttributesBuilder that is backed by a MapBuilder or we could have Attributes.unsafeFromMap style builder.

[iRevive]

Yeah, we should explore the alternatives.

[armanbilge]

final vals without type annotations get inlined.

Suggested change

	private val ExceptionEventName = "exception"
	private final val ExceptionEventName = "exception"

[armanbilge]

Can getName() on aClass ever actually return null?

[iRevive]

Hm. I don't think so.

[armanbilge]

What kinds of non-fatal exceptions could we get here? Sorry just nit-picking, it's not a big deal. Is the Java implementation like this?

[iRevive]

Valid point. AFAIK printStackTrace doesn't throw any

[armanbilge]

Some exceptions have no stack traces e.g. NoStackTrace. In those cases should the attribute value be empty or should the attribute be omitted entirely?

[iRevive]

Java SDK doesn't care and sets an empty string. Should we do

if (exception.getStackTrace.nonEmpty) {
  ...
  
}

?

[armanbilge]

Interesting. I guess NoStackTrace is a Scala-ism 😅 yeah, maybe better to omit in that case, I don't see the value.

[armanbilge]

Sorry, on second look I'm not sure. I'm reading this page.

https://opentelemetry.io/docs/specs/otel/trace/exceptions/#attributes

Additionally, the following attributes SHOULD be filled out:

• exception.escaped
• exception.message
• exception.stacktrace
• exception.type

It's SHOULD not MUST. Also it says "filled out" and not "present", so I suspect we can't satisfy it either way if we don't have a stack trace. Are we missing the escaped attribute?

[armanbilge]

More spec: https://opentelemetry.io/docs/specs/semconv/exceptions/exceptions-spans/

[iRevive]

Hm.

exception.escaped - boolean - SHOULD be set to true if the exception event is recorded at a point where it is known that the exception is escaping the scope of the span. [1] - Recommended

I believe it's possible only with the manual instrumentation:

escaped:

Tracer[F].span("span").use { span =>
  io.onError { error =>
    span.recordException(new Exception, Attribute("exception.escaped", true))
  }
}

not escaped:

Tracer[F].span("span").use { span =>
  io.handleErrorWith { error =>
    span.recordException(new Exception, Attribute("exception.escaped", false))
  }
}

[armanbilge]

I believe it's possible only with the manual instrumentation:

I'm not caught up with the implementation details, but if recordException is called automatically can't we just check the ExitCase after the use?

[iRevive]

I guess we can find a way, for example, here: https://github.com/typelevel/otel4s/pull/325/files#diff-9bac43fa0f17631759132ba638deda5d102310637d050a1a6495d58fe44b96e2R119-R125

We can explore these options when we start working on the SdkSpanBuilder implementation.

[armanbilge]

Would timestamp be a better name? That's how its referred to in the spec.

Suggested change

	def epoch: FiniteDuration
	def timestamp: FiniteDuration

[armanbilge]

Should we add escaped: Boolean as a parameter?