OpenTelemetry: Specify span kind and don't set error type for benign exception #2048
Conversation
interceptor/tracing_interceptor.go
Outdated
| // from the header or write to the header, where outbound means the span should | ||
| // be placed on the Temporal header, and inbound means the parent span can be | ||
| // retrieved from the Temporal header. | ||
| Outbound bool |
There was a problem hiding this comment.
Note: A change like this would break compatibility between new SDK and old tracing modules since they are versioned independently
There was a problem hiding this comment.
This is also part of the SDK public API so technically our backwards compatibility guarantees apply.
There was a problem hiding this comment.
gotcha, reverting this change, although I added a check that ToHeader and FromHeader aren't both set. Technically that can be a breaking change, but this is correcting invalid usage, so should be fine?
| } | ||
| } | ||
|
|
||
| spanKind := trace.SpanKindServer |
There was a problem hiding this comment.
Does this line up roughly with what other SDKs like dotnet or python do?
There was a problem hiding this comment.
I think so (though I think I just spotted a bug in Python for outbound signal child workflow)
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| spanKind := trace.SpanKindServer |
There was a problem hiding this comment.
I think so (though I think I just spotted a bug in Python for outbound signal child workflow)
|
|
||
| func (t *tracerSpan) Finish(opts *interceptor.TracerFinishSpanOptions) { | ||
| if opts.Error != nil { | ||
| if opts.Error != nil && !isBenignApplicationError(opts.Error) { |
There was a problem hiding this comment.
I think our implementation may not have been doing the right thing here because we are not calling .RecordError, which I think we should always do on error regardless of whether benign (but status as error only for benign like you have here). I don't know if we consider now starting to record errors as a breaking/dangerous change, but probably not.
There was a problem hiding this comment.
Looking at the godoc of RecordError, I think you're right. This doesn't seem like a dangerous change, but kinda breaking? But I feel like something we probably want to do, to fix behavior
3 participants
What was changed
serverfor inbound, andclientfor outboundToHeaderandFromHeader, they appear to directly be associated with if inbound or outboundWhy?
Checklist
Closes Set Span Kinds in Otel tracer interceptors #2024 and [Feature Request] Reclassify Benign Application errors in OpenTelemetry #2034
How was this tested:
Added tests
Note
Sets span kind (client/server) based on header direction and skips setting error status for benign ApplicationErrors; adds tests.
contrib/opentelemetry/tracing_interceptor.go):trace.WithSpanKind(default server; client whenToHeaderis true); error if bothToHeaderandFromHeaderare set.Erroronly for non-benigntemporal.ApplicationError.contrib/opentelemetry/tracing_interceptor_test.go):TestSpanKindto verify client/server kinds.TestBenignErrorSpanStatusto verify status handling for benign vs regular errors.Written by Cursor Bugbot for commit f17f8b0. This will update automatically on new commits. Configure here.