Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename the header to Baggage #17

Closed
SergeyKanzhelev opened this issue Mar 26, 2020 · 37 comments
Closed

Rename the header to Baggage #17

SergeyKanzhelev opened this issue Mar 26, 2020 · 37 comments
Labels
i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response.

Comments

@SergeyKanzhelev
Copy link
Member

this is an alternative proposal to #13. During the workshop today there was a proposal to rename header to the Baggage. Creating issue to discuss this proposal

@dyladan
Copy link
Member

dyladan commented Mar 26, 2020

I would definitely prefer this to correlationcontext. If we're going to change the name, then there is no reason to keep the long two-word format.

@mtwo
Copy link
Contributor

mtwo commented Mar 26, 2020

I am indifferent towards the name that we choose

@SergeyKanzhelev
Copy link
Member Author

Please see this document for more context: https://docs.google.com/document/d/1Crnp9XguH3BY5b1hcAV2QNiHZV0SKyIuC3moZgdpgiE/edit#

@shakuzen
Copy link

What about baggage? It would avoid issues with protocols that do not support case sensitivity - same reasoning as traceparent and tracestate.

@codefromthecrypt
Copy link

We should get buy in from @JonathanMace and/or @rfonseca as I believe the term Baggage was coined at Brown University with a different meaning albeit propagation related. This is less about this header being used for something lightly inspired by Baggage, rather any unlikely IP concerns that might exist

https://groups.google.com/forum/#!msg/distributed-tracing/m2ztbNAPsjY/4-mQkrfvBwAJ

@codefromthecrypt
Copy link

There's some baggage around the term baggage, but it has some weight to it.

I know the tracing inner circle is familiar with the name baggage, as it was suggested to b3 some time back openzipkin/b3-propagation#22

Some end users may also be familiar with this as spring-cloud-sleuth uses a similar terminology from the early days of OpenTracing. https://cloud.spring.io/spring-cloud-sleuth/reference/html/#propagating-span-context

@JonathanMace
Copy link

There's some baggage around the term baggage, but it has some weight to it.

🤦‍♂

Baggage is an appropriate alternative name for correlation context, and it would be consistent with the way that other distributed tracing tools (Jaeger, OpenTracing) use the term (since they also use it to refer to K-V pairs).

Originally we picked the name baggage to refer to "arbitrary stuff" that you propagate with a request because we liked the metaphor. In this case the metaphor is the same, so I don't see why not to call it baggage.

For some background on baggage:

  1. We first introduced it in Pivot Tracing as maps, sets and tuples.
  2. We then spun baggage out as a standalone component that we called BaggageContext and considered some of the nuances of making it general purpose.

It's true that the implementations we proposed in these papers are different to the K-V pair implementation proposed here, but conceptually the goal is the same.

@dyladan
Copy link
Member

dyladan commented Mar 27, 2020

Personally, I would suggest baggage (all lowercase) as it is consistent with the naming/casing of the other specs this WG is working on, it is short, and it conveys the meaning in a clear and obvious way.

@yurishkuro
Copy link
Member

+1 for baggage

@discostu105
Copy link
Member

+1 for baggage as well

@rfonseca
Copy link

rfonseca commented Mar 27, 2020 via email

@mwear
Copy link
Member

mwear commented Mar 28, 2020

+1 to baggage as well. A single word header avoids the pitfalls of a longer concatenated header, or one with a -.

@codefromthecrypt
Copy link

I thumbs upped the original suggestion to use baggage, but doing more explicitly here +1

Thanks @JonathanMace and @rfonseca for clearing the name! I recall the SPDY days which turned into http/2 over name related stuff. nice to be able to avoid that.

@codefromthecrypt
Copy link

FYI: In Zipkin Brave we'll rename our stuff around baggage naming, since it is general purpose now. will slide into concepts here nicely.

@mtwo
Copy link
Contributor

mtwo commented Mar 31, 2020

Nice! Sounds like we have consensus?

@codefromthecrypt
Copy link

Also, we can consider if renaming the spec itself makes sense..

@yurishkuro
Copy link
Member

How are we going to decide & close this?

@tedsuo
Copy link

tedsuo commented Mar 31, 2020

Jumping in at the last minute here.

I'm fine with a changing the header name, so long as the semantic meaning is also changing. When this working group started, there was an intention to say that correlations were for observability only. Crucially, that meant it was acceptable to trim or drop correlations - or at least that was the thinking at the time.

If we are going to propose a more general purpose header, then users will begin relying on this propagation mechanism for features beyond observability. That makes baggage a critical system, and dropping baggage could have a serious impact.

My main concern is that this debate appears to be focused on a naming preference, and whether correlation or baggage sounds better. That makes me uncomfortable. I think both names are fine, but they are for different features, with different scopes and responsibilities.

Therefore, @yurishkuro I suggest the decision to close these issues should be made based on what scope and responsibility this header should have. If it is only for observability, and potentially lossy, the name should be correlations. If this header is for a general purpose context propagation mechanism, which must be propagated or risk changing the functioning of the underlying system, it should be called baggage.

Open to other interpretations, but want to verify that this working group has consensus on which target they are trying to shoot for, and not just which name they prefer. :)

@yurishkuro
Copy link
Member

@tedsuo I agree with that. The current wording about the purpose of this header is very vague, and makes many references to trace context.

A correlation context header is used to pass the name-value context properties for the trace. This is a companion header for the traceparent. The values should be passed along to any child requests. Note that uniqueness of the key within the Correlation-Context is not guaranteed. Context received from upstream service may be altered before passing it along.

@codefromthecrypt
Copy link

codefromthecrypt commented Apr 1, 2020

per above mentions about opentracing, jaeger, sleuth etc. baggage is indeed already used in practice to implement random fields. They are already lossy in practice and there are a couple years of this in practice. There are other lossy variants out there like Amazon's trace format which permits arbitrary fields and no means to identify if they are correlation or not.

I think the header should be renamed as regardless of who is expecting loss (observability or otherwise) it exists. Also even in observability there is state that precedes a trace, such as sampling details etc, which are not correlation in nature.

The name "correlations" if somehow restricted to things only correlation in nature would simply result in something creating the baggage header separately and repeating the work.

Currently more people are interested in this spec using the name baggage, still.. so maybe rewording the description is the best way out vs fork another name.

@jcchavezs
Copy link

jcchavezs commented Apr 1, 2020

Despite the semantics of baggage vs correlation is I think the "de facto" name for such information is baggage and it makes sense that what users/practitioners do is what becomes the standard hence I think baggage is a good choice here. As @adriancole pointed out (with the sampling example) and I think @yurishkuro also told me when in NYC baggage is not only used for correlation but also information that you want to propagate and that comes along with the request (as far as I remember Yuri told me this was used for flagging synthetic traffic, correct me if I am wrong). Not everything being propagated is used for correlation (e.g. synthetic traffic flag is not meant to correlate anything but mostly about propagating information) but everything being propagated is actually the baggage of the request.

@basvanbeek
Copy link

This kind of reasoning reminds me of XKCD standards... https://xkcd.com/927/

There is value in standardizing what's already the custom / expectation in the wild because it has heavy push moving forward. Let's not change or walk away from that because we want to put the cat back into the bag at quite the cost. Propagating baggage context properly is the largest and most difficult part to get right of all instrumentation concerns. Obviously platform owners want to reuse instead of having to reinvent the wheel for their own baggage concerns while the transport for it is already loaded up. There is quite the brownfield out there already stuffing their own baggage into the instrumentation context because it was available and works well. Let's not break their ecosystems and assumptions.

baggage +1

@tedsuo
Copy link

tedsuo commented Apr 1, 2020

Great, glad to hear it. And to be clear, I'm all for baggage as well. I just want to make sure we're not redefining the term in the process, and moving forwards with standardizing existing practices.

@mtwo
Copy link
Contributor

mtwo commented Apr 7, 2020

Are there any dissenting opinions to renaming the header baggage? @SergeyKanzhelev are you satisfied with the rename?

@dyladan is filing a PR for the rename, but please comment here or on the forthcoming PR if you disagree

@SergeyKanzhelev
Copy link
Member Author

If the overwhelming majority likes the baggage name - I'm supportive of this decision. I understand the reasoning for switch and if started from scratch would vote for baggage myself.

Again, my reasoning towards keeping the Correlation-Context is not much of a prior art, rather speed of adoption we can demonstrate. My understanding is that not many APM vendors today actively using and allowing "user baggage" propagation. So even when OpenTelemetry will support it, we may not find many in-the-wild use of the new header. With the Correlation-Context we can show the use by Microsoft.

As we moving this forward we need to start collecting information on who will be actually using the header so we will demonstrate adoption.

@SergeyKanzhelev
Copy link
Member Author

@rfonseca thank you for clarifying IP side of things.

Reading comments I realized, we may want to run the name against possible internationalization concerns. @plehegar do you know if anybody in W3C can help with this?

@codefromthecrypt
Copy link

any updates on this?

@danielkhan
Copy link
Contributor

danielkhan commented Jun 3, 2020

@adriancole it was concluded that the header should be renamed to baggage as we see a broad consent for it. The change will be made within the next few days.

@codefromthecrypt
Copy link

any update?

@plehegar plehegar added the i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. label Jun 29, 2020
@plehegar
Copy link
Member

@r12a , any thoughts on the use of the name "baggage" (see previous comment from @SergeyKanzhelev ).

@r12a
Copy link

r12a commented Jul 1, 2020

[disclaimer: I'm not familiar with this technology, and have only scanned the long discussion above]

Could someone (perhaps @SergeyKanzhelev) give me an idea of what kinds of i18n issues might be at issue here? Is it a question of whether the word 'baggage' has undesirable connotations, or something else? (fwiw, in the UK this word can have a pejorative connotation - indicating uninteresting stuff that it's annoying to have to lug around).

@yurishkuro
Copy link
Member

indicating uninteresting stuff that it's annoying to have to lug around

nailed it! That's exactly what we want it to mean :-)

@danielkhan
Copy link
Contributor

Could someone (perhaps @SergeyKanzhelev) give me an idea of what kinds of i18n issues might be at issue here? Is it a question of whether the word 'baggage' has undesirable connotations

Yes - exactly that. If there is no obvious offensive connotation, I think we are good.
I second what @yurishkuro says.

@codefromthecrypt
Copy link

I'm a little surprised this is not merged. For example, it appears that open telemetry is using Lightstep's "Correlations" term in the header name otcorrelations.

While OpenTelemetry are free to use vendor terms, it will bring pressure to continue using them, and then not use the cleared term "baggage". Plus, using a word used in trade seems a severe step backwards, especially considering there's no notable reason that this shouldn't have been complete months ago.

What else needs to happen here?

@mwear
Copy link
Member

mwear commented Jul 8, 2020

See this PR for more discussion regarding the header change in OpenTelemetry: open-telemetry/opentelemetry-specification#517.

The name was selected by the poll in this comment: open-telemetry/opentelemetry-specification#517 (comment).

@jcchavezs
Copy link

jcchavezs commented Jul 9, 2020 via email

@danielkhan
Copy link
Contributor

See w3c/distributed-tracing-wg#42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response.
Projects
None yet
Development

No branches or pull requests