w3c / baggage

Propagation format for distributed context: Baggage
https://w3c.github.io/baggage/
Other
46 stars 18 forks source link

Add rationale for why libraries MAY propagate or otherwise change to MUST #25

Closed anuraaga closed 3 years ago

anuraaga commented 4 years ago

In the overview, the spec states Libraries and platforms MAY propagate this header.. It also says This is a companion header for the traceparent.

In the spec for traceparent. At a minimum they MUST propagate the traceparent and tracestate headers.

So intuitively, it seems that if it's a companion to traceparent, and that MUST be propagated, this MUST be too and I'm confused why correlationcontext suggest it MAY propagate.

dyladan commented 4 years ago

I think we should probably just remove the wording that states this is a companion to traceparent. It solves an entirely different problem and could be propagated with or without trace context.

justinfoote commented 4 years ago

I agree. They're two separate things, with mostly non-overlapping use cases.

But even if we don't remove the line about correlation context being a companion to traceparent, this doesn't feel too inconsistent to me. Some parts of the spec are mandatory (traceparent and tracestate), and some parts of the spec are optional (correlation context).

codefromthecrypt commented 4 years ago

Thanks @anuraaga the spec you mentioned is the only reason why brave's impl is nonsensically dropping good data in tracestate when traceparent doesn't exist.

To carry B3 (in tracestate) without traceparent is much easier than with it as the traceparent header, especially how it defines sampled is of no use except ID correlation. We have to ignore it basically.

anuraaga commented 4 years ago

It's an interesting point about keeping tracestate even if parent gets lost to at least have something to work with.

It feels like that also fits the MUST propagate model well - header in, header out should generally give more functionality to users.

I guess I'm still not clear on, is this one of the larger bucket of w3c trace header, so it's an optional feature (leaving it up to the user to manage libraries to get the features they need)? Or is it an independent feature. The replies give me both impressions. If it is an independent feature, surely we want that to mandate propagation so context parameters don't get lost?

dyladan commented 4 years ago

I see correlation context as a separate feature from trace context.

@adriancole if we allow propagation of tracestate without traceparent, that would violate what I view as one of the most important parts of the trace context spec which is interoperability between tracing systems. If one implementation only needs tracestate and doesn't create a traceparent, they are potentially breaking the traces of other systems.

In any case, I don't think that should apply to correlation context, which should be allowed to be propagated without a trace context.

danielkhan commented 3 years ago

We will remove traceparent wording and change to SHOULD