OpenTelemetry: Specify span kind and don't set error type for benign exception #2048
Conversation
|
|
||
| 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.
Open tracing is deprecated, not sure we should bother fixing it here/ do a release to publish this
| // 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.
| } | ||
| } | ||
|
|
||
| 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)
| } | ||
| } | ||
|
|
||
| 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.
| func isBenignApplicationError(err error) bool { | ||
| var appErr *temporal.ApplicationError | ||
| if temporal.IsApplicationError(err) { | ||
| if errors.As(err, &appErr) { |
There was a problem hiding this comment.
Per #1925 (comment), we should only look at the top-level error to determine whether benign. Mimic the isBenignApplicationError already in internal/error.go.
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