open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.23k stars 1.05k forks source link

What’s the design intent of the Fields() method of TextMapPropagator? #5355

Closed Cirilla-zmh closed 4 months ago

Cirilla-zmh commented 5 months ago

I'm currently using the tracing capabilities provided by the OTel-go SDK to monitor a multi-language microservices application system. However, the tracing protocol for our application's traffic needs to be based on Jaeger, meaning we need to develop a propagator tailored for the Jaeger protocol.

My challenge arises when trying to implement a TextMapPropagator for Jaeger's baggage, specifically with the implementation of the Fields method. Given that Jaeger protocol's baggage keys are dynamic, I'm unable to provide a static implementation of the Fields() method without relying on any context.

I've explored the opentelemetry-go and opentelemetry-go-contrib projects, but it appears there isn't any information available that could assist me.

How should I approach this issue? I'm eagerly looking forward to your suggestions.

dmathieu commented 5 months ago

We do provide a Baggage implementation, as well as a Baggage Propagator, which follows the W3C baggage convention. See the doc: https://opentelemetry.io/docs/concepts/signals/baggage/

Cirilla-zmh commented 5 months ago

We do provide a Baggage implementation, as well as a Baggage Propagator

Thanks for your reply! I have noticed the baggage Propagator implementation of W3C. However, what I actually need is an implementation of Jaeger, which allows us to perform tracing using the Jaeger protocol.

I have found the implementation of the trace_context Propagator in opentelemetry-go-contrib, but I couldn't locate one for baggage.

See the doc: https://opentelemetry.io/docs/concepts/signals/baggage/

I am certain that I have understood the definition of baggage, which is similar to the definition of Jaeger's baggage. This is also confirmed by the Jaeger propagator in the opentelemetry-java.

pellared commented 5 months ago

@Cirilla-zmh I guess you are looking for https://pkg.go.dev/go.opentelemetry.io/contrib/propagators/jaeger

Cirilla-zmh commented 5 months ago

@Cirilla-zmh I guess you are looking for https://pkg.go.dev/go.opentelemetry.io/contrib/propagators/jaeger

Thank you for your help. I had already noticed this implementation in https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/propagators/jaeger, and indeed, it provides the capability for SpanContext propagation, but it doesn't offer the propagation abilities for Baggage, which is what I really need.

In fact, we have already implemented Jaeger's Baggage Propagator, modeled after its Java implementation. However, while implementing the TextMapPropagator interface, I've found that I'm unable to provide an effective override for the Fields method — and this is the reason why I brought up this issue.

Cirilla-zmh commented 5 months ago

@pellared @dmathieu
I apologize for any confusion caused – it seems I may not have clearly communicated the dilemma I am facing. Please allow me to explain the issue through a code example.

const (
    baggageHeaderPrefix      = "uberctx-"
    baggageHeaderPrefixWidth = 8
)

// Baggage is a propagator that supports the Jaeger Baggage format.
//
// This propagates user-defined baggage associated with a trace. The complete
// specification is defined at https://www.jaegertracing.io/docs/1.57/client-libraries/#tracespan-identity/.
type Baggage struct{}

var _ sdk_propagation.TextMapPropagator = Baggage{}

// Inject sets baggage key-values from ctx into the carrier.
func (b Baggage) Inject(ctx context.Context, carrier sdk_propagation.TextMapCarrier) {
    baggage := sdk_baggage.FromContext(ctx)
    fmt.Printf("[debug] Get %d baggages %s\n", baggage.Len(), baggage.String())
    for _, member := range baggage.Members() {
        carrier.Set(baggageHeaderPrefix+member.Key(), member.Value())
    }
}

// Extract returns a copy of parent with the baggage from the carrier added.
func (b Baggage) Extract(ctx context.Context, carrier sdk_propagation.TextMapCarrier) context.Context {
    keys := carrier.Keys()
    // pre-allocate 10 size for members
    members := make([]sdk_baggage.Member, 0, 10)
    for _, key := range keys {
        if strings.HasPrefix(key, baggageHeaderPrefix) {
            actualKey := key[baggageHeaderPrefixWidth:]
            m, err := sdk_baggage.NewMember(actualKey, carrier.Get(key))
            if err != nil {
                continue
            }
            members = append(members, m)
        }
    }
    bag, err := sdk_baggage.New(members...)
    if err != nil {
        return ctx
    }
    return sdk_baggage.ContextWithBaggage(ctx, bag)
}

// Fields returns the keys who's values are set with Inject.
func (b Baggage) Fields() []string {
    // FIXME: Actually, I cannot return anything without context here.
    return []string{}
}

Here is our implementation of Jaeger's Baggage Propagator (similar to the one specified by W3C), which illustrates the issue: given the dynamic nature of Jaeger's baggage headers uberctx-{baggage-key}, it is not possible for me to specify an exact override method for the Fields().

dmathieu commented 4 months ago

Have you taken a look at the specification description for Fields.

From reading that spec, one thing you could do is store a slice of fields whenever an export happens, and return the values that were stored from previous exports.

If you look at the java implementation that you linked above however, they have a static list of fields: https://github.com/open-telemetry/opentelemetry-java/blob/2e3e04c7e882722c52e1c56e4e8604e78594d87d/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java#L80

Which contains a single value: https://github.com/open-telemetry/opentelemetry-java/blob/2e3e04c7e882722c52e1c56e4e8604e78594d87d/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java#L41

Not having a fully complete list of fields in your propagator shouldn't be a problem for it to work. However, the propagation API is stable, and described in the spec, not in here. So we won't be able to change it.

Cirilla-zmh commented 4 months ago

Have you taken a look at the specification description for Fields.

Thanks a lot for your help! This documentation resolved my questions!

In my implementation, I will return an empty array of string. As the documentation states:

Observe that some Propagators may define, besides the returned values, additional fields with variable names. To get a full list of fields for a specific carrier object, use the Keys operation.

Cirilla-zmh commented 4 months ago

By the way, is there a need for a Jaeger propagator implementation in the opentelemetry-go-contrib project? I would be very happy to contribute.