Fix span tagging for application error in GO YARPC/gRPC #2287
Conversation
|
|
| @@ -496,6 +499,17 @@ func (c call) logStreamEvent(err error, success bool, succMsg, errMsg string, ex | |||
| ce.Write(fields...) | |||
| } | |||
|
|
|||
| // emitSpanErrorTag sets the error information as tags on the current span | |||
| func (c call) emitSpanErrorTag(res callResult) { | |||
| if span := opentracing.SpanFromContext(c.ctx); span != nil { | |||
There was a problem hiding this comment.
My concern of using obs middleware is that the required inbound/ outbound spans are not available in go context when you are trying to obtain spans here.
Could you double check?
There was a problem hiding this comment.
After reviewing the code more thoroughly, I believe you're right. It doesn’t seem straightforward to pass the original spans here. Do you have any suggestions on how we might approach this?
| @@ -23,6 +23,7 @@ package observability | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "github.com/opentracing/opentracing-go" | |||
There was a problem hiding this comment.
All such imports should be sorted, thx.
| // The error message is intentionally omitted to prevent exposing | ||
| // personally identifiable information (PII) in tracing systems. | ||
| // Returns the given error | ||
| func UpdateSpanWithApplicationErr(span opentracing.Span, err error, errCode yarpcerrors.Code) { |
There was a problem hiding this comment.
Please return err from this func.
| // emitSpanApplicationErrTag sets the application error information as tags on the current span | ||
| func (c call) emitSpanApplicationErrTag(res callResult) { | ||
| // Check if application error is applicable first | ||
| if !res.isApplicationError { |
There was a problem hiding this comment.
My question was left unanswered: seems like span will have tags already (from this changes: https://github.com/yarpc/yarpc-go/pull/2282/files#diff-2c03b550b54bc0459d694d1aa8ae408c392280ff11932ff44e0be1654eeb6a4eR233).
So do we need this func at all?
Fix span tagging for application error in GO YARPC/gRPC