Add OTel-native YARPC tracing interceptor - #2523
Conversation
|
|
e273ce1 to
54ae0c8
Compare
54ae0c8 to
b750e5a
Compare
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/codes" | ||
| "go.opentelemetry.io/otel/propagation" | ||
| semconv "go.opentelemetry.io/otel/semconv/v1.26.0" |
There was a problem hiding this comment.
is this the most recent version that we might use? (latest is v1.45)
| // commonOTelAttributes are the static attributes set on every span, matching | ||
| // commonTracingTags on the OpenTracing side. | ||
| var commonOTelAttributes = []attribute.KeyValue{ | ||
| attribute.String("go.version", runtime.Version()), |
There was a problem hiding this comment.
we can omit go.version, the tracer itself adds this tag
| // commonTracingTags on the OpenTracing side. | ||
| var commonOTelAttributes = []attribute.KeyValue{ | ||
| attribute.String("go.version", runtime.Version()), | ||
| attribute.String("component", tracingComponentName), |
There was a problem hiding this comment.
does this name differentiate from the existing instrumentation?
| func (t *tracedOTelClientStream) ReceiveMessage(ctx context.Context) (*transport.StreamMessage, error) { | ||
| msg, err := t.clientStream.ReceiveMessage(ctx) | ||
| if err != nil { | ||
| return nil, t.closeWithErr(err) |
There was a problem hiding this comment.
What is the lifecycle of this span, is it per stream or per message?
If it is created during stream start, then it should be Ended when the stream finishes; and not when a message errors.
| func wrapOTelClientStream(s *tracedOTelClientStream) *transport.ClientStream { | ||
| wrapped, err := transport.NewClientStream(s) | ||
| if err != nil { | ||
| s.span.End() |
| // An application error without a code has no numeric equivalent, so the | ||
| // status code attribute is a string here and an int elsewhere. This | ||
| // matches the OpenTracing interceptor. | ||
| span.SetAttributes(attribute.String(rpcStatusCodeTag, applicationError)) |
There was a problem hiding this comment.
this string/int split is a bit confusing, maybe we should use a different key
| // The error message is deliberately not recorded on the span. It can carry | ||
| // request data, and the OpenTracing interceptor never emitted it either. | ||
| span.SetStatus(codes.Error, "") | ||
|
|
There was a problem hiding this comment.
it is desirable to record the error message on the span; could we make this configurable?
|
could you take a quick peek at https://github.com/grpc/grpc-go/tree/538bb2f7af6e36be46ec1d2ae90f65b972f14083/stats/opentelemetry to see if there is anything we can learn? |
3 participants
Prerequisite for dropping the OpenTracing bridge. tracingcore already builds a native trace.TracerProvider, but wraps it in opentracingbridge.NewUberBridgeTracer because yarpcfx only accepts an opentracing.Tracer, so every YARPC span goes through the bridge. This gives YARPC an OTel entry point so jaegerfx can pass the provider straight through.
PR is still in progress as I put together the full picture.