feat(otlp): introduce "kind" to spans

[mjq]

OTLP spans have a property called kind that helps specify the relationship between that span and its parent or child. Sentry users should be able to see the span kind and search/filter on it when it is sent (via OTLP or the OtelSpan envelope item type). During ingestion, write a string representation of the kind to the span.kind attribute.

(A string representation is chosen over the default integer one because it is both self-describing and also makes search straightforward (no need for translations between string and integer in the frontend or EAP. We are relying on Clickhouse's compression to make the additional data usage reasonable.)

[Dav1dde]

Please add a small doc string, ideally what values it can have and when it should be filled.

[jan-auer]

See opentelemetry-proto:

  // Distinguishes between spans generated in a particular context. For example,
  // two spans with the same name may be distinguished using `CLIENT` (caller)
  // and `SERVER` (callee) to identify queueing latency associated with the span.

[jan-auer]

In opentelemetry-proto this is a top-level field on the span rather than an attribute. Could we align with their schema and create a field on our Span, too? Note we did the same with is_remote (mapping it out from flags).

[mjq]

@jan-auer Can do. Just to confirm, I'm assuming that would mean adding kind as a field to both Span and SpanKafkaMessage for transfer to Snuba? (And then EAP can choose to store it however it makes sense, column or attribute?)

Also, in that case the choice of how to save the value is being made by EAP instead - in that case should we transfer it as the original integer value rather than our string representation, maybe? (Leaving it to Snuba to stringify, in storage or via RPC?)

[jan-auer]

that would mean adding kind as a field to both Span and SpanKafkaMessage for transfer to Snuba?

That's right.

And then EAP can choose to store it however it makes sense, column or attribute?

We can consider aligning with how we want kind to show up in the product & how otel SDKs track it. In my previous comment, I assumed we would then also show it as span.kind rather than span.data.kind. If we want to show it as an attribute, then we should probably keep your current approach. Otherwise, we have a strange drift between the SDK protocol, pipeline, and the product-side data model.

in that case should we transfer it as the original integer value rather than our string representation, maybe?

Definitely possible, though I'd advise against it in this instance. In both cases - string or integer - would need to share an enum between Relay, Sentry (pipeline) and Snuba. However, if we convert to string representation right away, we can now use the same enums in the processing pipeline and in all workers/endpoints that pull data out of EAP. Essentially, the pre- and post-EAP data model would be identical, which I think would be desirable.

[mjq]

@jan-auer I've updated the PR to use a new kind field on Span and SpanKafkaMessage. I couldn't find a way to profitably use an explicit enum inside Relay for this (although we might still be able to later in the pipeline or in workers/etc) - but please let me know if you had anything specific in mind.

[jan-auer]

Thanks, I think using the string representation as a top-level field is the right choice.

[jan-auer]

If you like, we could also make this an enum with a From<i32> implementation for known kinds like here. Maybe @Dav1dde wants to chime in if there's a preference.

[Dav1dde]

I like that suggestion, more strong types are always good down the road 👍

[mjq]

Thanks for the feedback, I'll make that change 👍

[mjq]

@jan-auer @Dav1dde I've updated the PR to use an enum.

I used a From<i32> implementation in c86423b. The only thing I didn't like about this implementation was that we lost the match on opentelemetry-proto's SpanKind enum (henceforth OtelSpanKind) - in other words, we would no longer be notified (due to exhaustiveness checking) if new kind values were added upstream in the future.

You can see in commit 2f764bc my changes to instead use From<OtelSpanKind>, effectively first transforming the value from the incoming i32 to OtelSpanKind, then to our own SpanLink type. This gives us an exhaustiveness check on OtelSpanKind matches again, and even though the code to read kind in otel_to_sentry_span became a bit longer, I personally like the explicitness of the second in this particular context.

// From<i32> version

kind: SpanKind::from(kind).into(),

vs

// From<OtelSpanKind> version

kind: OtelSpanKind::try_from(kind)
        .map_or(SpanKind::Unspecified, SpanKind::from)
        .into(),

I can always revert the second commit to get back to the more direct i32 check if you prefer. Curious if you have a preference either way.