Go-kit does not propagate incoming `traceparent`

[drewtoto]

What version of Go and opentelemetry-go-contrib are you using? go 1.17 opentelemetry-go-contrib v0.31.0

What operating system and processor architecture are you using? linux/amd64

What did you do? Added tracing middleware to go-kit endpoints (gRPC) provided by go.opentelemetry.io/contrib/instrumentation/github.com/go-kit/kit/otelkit package using otelkit.EndpointMiddleware(otelkit.WithOperation("operation"))(my-gokit-endpoint)

What did you expect to see? Expected to see the otelkit package extract the traceparent from the incoming context and make the go-kit created span a child of the parent (the gRPC caller in my case). Expected that otelkit.EndpointMiddleware would use Extract from the go.opentelemetry.io/otel/propagation package to parse and use the traceparent.

What did you see instead? The tracing span from the caller (gRPC client from a separate process) was NOT propagated and used as the trace parent for the go-kit created span. NOTE: traceparent header is present in the incoming context.

Additional comments This feels like I'm doing something wrong as distributed tracing is a core benefit for implementing tracing to begin with. I understand that this community is relatively small at the moment which may be the reason for the limited documentation, but, trying to figure out if this functionality is baked in or if I'm just doing something wrong was quite an effort.

Ultimately, my solution was to implement a propagation middleware to wrap the otelkit.EndpointMiddleware. Though this solution is relatively simple, it still feels like this functionality should be baked in.

What am I doing wrong?

My solution below:

package endpoint

import (
	"context"

	"github.com/go-kit/kit/endpoint"
	"go.opentelemetry.io/contrib/instrumentation/github.com/go-kit/kit/otelkit"
	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/propagation"
	"google.golang.org/grpc/metadata"
)

func MakeTracingEndpoint(endpoint endpoint.Endpoint, operation string) endpoint.Endpoint {
	return PropagationMiddleware()(
		otelkit.EndpointMiddleware(otelkit.WithOperation(operation))(endpoint),
        )
}

type MetadataCarrier metadata.MD

// Compile time check that MapCarrier implements the TextMapCarrier.
var _ propagation.TextMapCarrier = MetadataCarrier{}

// Get returns the value associated with the passed key.
func (c MetadataCarrier) Get(key string) string {
	v := metadata.MD(c).Get(key)
	if len(v) > 0 {
		return v[0]
	}
	return ""
}

// Set stores the key-value pair.
func (c MetadataCarrier) Set(key, value string) {
	metadata.MD(c).Set(value)
}

// Keys lists the keys stored in this carrier.
func (c MetadataCarrier) Keys() []string {
	keys := make([]string, 0, len(c))
	for k := range c {
		keys = append(keys, k)
	}
	return keys
}

func PropagationMiddleware() endpoint.Middleware {
	return func(next endpoint.Endpoint) endpoint.Endpoint {
		return func(ctx context.Context, request interface{}) (response interface{}, err error) {
			md, _ := metadata.FromIncomingContext(ctx)
			ctx = otel.GetTextMapPropagator().Extract(ctx, MetadataCarrier(md))
			return next(ctx, request)
		}
	}
}

[MrAlias]

This indeed looks like a missing feature. Propagation logic similar to our other instrumentation ^1^3^5 should be added to the otelkit instrumentation.

[Aneurysm9]

I think this might be related to the go-kit design philosophy. The EndpointMiddleware godoc includes this:

// This endpoint middleware should be used in combination with a Go kit Transport
// tracing middleware, generic OpenTelemetry transport middleware or custom before
// and after transport functions.

Perhaps we should include the PropagationMiddleware from @drewtoto's solution as well.

[drewtoto]

I can add PropagationMiddleware to otelkit/endpoint.go and open a PR if it would be helpful.

[MrAlias]

I can add PropagationMiddleware to otelkit/endpoint.go and open a PR if it would be helpful.

Thanks! I would put the new middleware in its own file, but otherwise this sounds good to me.

[drewtoto]

While creating this PR (#2167), it occurred to me that in the go-kit philosophy, the endpoint layer should be agnostic to the transport layer, and this is likely why trace context propagation isn't part of otelkit/endpoint.go.

I believe additional middleware(s) with separation of transport layer stay true to this philosophy while bringing usability to developers. With gRPC and HTTP being the two majorly supported/used transports, I renamed PropagationMiddleware to GrpcPropagationMiddleware and added a HttpPropagationMiddleware.

I am curious about your thoughts on this? I wanted to get feedback on this before proceeding further with tests, etc.

[drewtoto]

I'm becoming a bit more familiar with the contrib project and have a few more comments and suggestions that might expand this issue a bit. It seems that tracing is commonly implemented on the HTTP transport layer, which is why opentelemetry-go has included a HeaderCarrier^1 which adapts http.Header^2 for ease-of-use and reuse. No such carrier implementing TextMapCarrier^3 exists for gRPC metadata.MD^4, which would be useful for all projects providing OpenTelemetry instrumentation via gRPC.

In my (admittedly superficial) search of projects under the contrib umbrella, I found otelgrpc which implements an adapter (metadataSupplier^5) for the metadata.MD struct, but does not export it. This effectively duplicates the MetadataCarrier struct included in my original post.

If you agree with this proposal, I can work on another, separate PR to add the MetadataCarrier to go.opentelemetry.io/otel/propagation for all current and future gRPC-based projects to use.

[Aneurysm9]

If you agree with this proposal, I can work on another, separate PR to add the MetadataCarrier to go.opentelemetry.io/otel/propagation for all current and future gRPC-based projects to use.

I generally support this idea, but I believe that we may want to do it in the contrib repo in a new module. Adding it in go.opentelemetry.io/otel/propagation would pull a gRPC dependency into the main API module, which would then propagate to everyone using that API, regardless whether they actually need gRPC.

[drewtoto]

Ah ok, I forgot to check and didn't realize that gRPC wasn't already a dependency. I suppose another option would be to change MetadataCarrier to a MultimapCarrier to avoid the gRPC dependency.

Therefore: type MetadataCarrier metadata.MD would be type MultimapCarrier map[string][]string

I will finish this work in contrib as suggested and open the PR for review. Thanks!

[pellared]

Side note: shouldn't we consider moving the library instrumentation here per

we have the following recommendations for where instrumentation packages should live.

1. Native to the instrumented package
2. A dedicated public repository
3. Here in the opentelemetry-go-contrib repository

from our recommendations

[pellared]

otelkit was removed from the repository. Closing.