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

Consider renaming the header from Correlation-Context to correlationcontext #13

Closed
mwear opened this issue Dec 16, 2019 · 17 comments
Closed

Comments

@mwear
Copy link
Member

mwear commented Dec 16, 2019

Proposal

For trace-context we decided to use to use all lowercase alphabetic characters to make the header usable in non HTTP scenarios. Message queues such as JMS are more restrictive than HTTP in regards to header names. If we expect to propagate Correlation Context over protocols other than HTTP, we should consider using the same naming conventions as used for trace-context and name this header correlationcontext.

The rationale for this change is the same as the rationale for Trace Context headers. See: https:/w3c/trace-context/blob/master/http_header_format_rationale.md#lowercase-concatenated-header-names

Some benefits of having an alphabetic lowercase header name:

  • Maximizes the scenarios that this header name can be used in
    • HTTP headers,
    • key-value based systems of all kinds
  • Reduces friction to adoption
    • There is only one canonical representation of the header that needs to looked up

Known Issues

Incompatible as a JMS Header

Correlation-Context is incompatible as a JMS header due to the - character.

Section 3.8.1.1 (Message Selector Syntax) of the JMS Specification states:

Identifiers:

  • An identifier is an unlimited length character sequence that must begin with a Java identifier start character and all following characters must be Java identifier part characters. An identifier start character is any character for which the method Character.isJavaIdentifierStart returns true. This includes ‘_’ and ‘$’. An identifier part character is any character for which the method Character.isJavaIdentifierPart returns true.

Example:

import java.lang.*;

public class IdentifierDemo {

   public static void main(String[] args) {
      char dash = '-';
      boolean dashValid = Character.isJavaIdentifierPart(dash);
      System.out.println("Dash valid?: " + dashValid);    
   }
}

Output:

$javac IdentifierDemo.java
$java -Xmx128M -Xms16M IdentifierDemo
Dash valid?: false
@SergeyKanzhelev
Copy link
Member

As was discussed during the call - the decision on name must balance between prior art - what was implemented in .NET based on Editor Draft of this spec and the benefits of different name. Prior art and implementations that are already live will speed up adoption of a spec. While better name can make support of some protocols like JMS better.

I'd suggest to keep the current name for the faster adoption. As I mentioned in PR #14 it's not just .NET, we see people using this specification already in other places and scenarios.

@dyladan
Copy link
Member

dyladan commented Jan 27, 2020

Opentelemetry is beginning work on this in earnest now. We should probably talk about this issue during tomorrow's call.

@mwear
Copy link
Member Author

mwear commented Jan 29, 2020

The current header is incompatible with JMS, not MQTT as I thought yesterday. I updated the description of the issue with an example.

@mtwo
Copy link
Contributor

mtwo commented Mar 26, 2020

I am indifferent towards the name that we choose

@SergeyKanzhelev
Copy link
Member

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

@codefromthecrypt
Copy link

If you choose to make this Correlation-Context zipkin will make a different header that can be used in JMS. your call

@codefromthecrypt
Copy link

PS @SergeyKanzhelev please don't use google docs, they aren't visible in some countries due to blocking of google.

@codefromthecrypt
Copy link

FWIW we've discussed b3ext a long time ago, but punted on it due to this spec. This is likely the default name if the spec here becomes unusable.

@codefromthecrypt
Copy link

also renaming isn't a choice of just downcasing. for example, in trace-context iirc NR suggested different names hence traceparent and tracestate (no one I recall felt strongly enough to debate those choices, which is amazing)

ex simply correlation could also work

@codefromthecrypt
Copy link

There's a separate issue tracking another potential name here #17

@felixbarny
Copy link
Contributor

A few thoughts.
From a consistency standpoint, it’s strange to both see lower cased, non-hyphenated headers like traceparent and the exact opposite in the same umbrella of specs.

I can understand that sticking with the current proposal would be easier for some. But this is in a draft state so everyone relying on the current version should anticipate backwards incompatible changes.

Backwards compatibility in drafts should not weight over compatibility with technologies like JMS and consistency with the rest of the spec.

The earlier this change is made, the less painful it will be.

@dyladan
Copy link
Member

dyladan commented Mar 27, 2020

@adriancole we discussed the name correlation in the meeting but decided that it had too many other meanings, particularly a few of the tracing vendors use the term internally for their process of linking spans together, and we didn't want to explode the naming choices and make the decision drag on for too long of a time.

@nicmunroe
Copy link

[felixbarny] From a consistency standpoint, it’s strange to both see lower cased, non-hyphenated headers like traceparent and the exact opposite in the same umbrella of specs.

Agreed.

[felixbarny] Backwards compatibility in drafts should not weight over compatibility with technologies like JMS and consistency with the rest of the spec.

Agreed. And it's not just JMS - I'd be surprised if there weren't other things out there that would choke on Correlation-Context, either now or in the future. Picking something with a maximum likelihood of compatibility in as many situations as possible feels like something this spec should shoot for.

[felixbarny] The earlier this change is made, the less painful it will be.

Agreed.

[SergeyKanzhelev] I'd suggest to keep the current name for the faster adoption.

The problem is once this is finalized, it can't realistically be changed. Sticking with Correlation-Context feels like a short term win but a long-term loss.

I do sympathize with wanting to disrupt existing stuff as little as possible. It's a valid concern and it almost swayed me. But taken as a whole I'd vote for changing it to correlationcontext or another similar name that's consistent with traceparent and tracestate, and maximizes the likelihood of compatibility with other non-HTTP techs/protocols/etc.

@SergeyKanzhelev
Copy link
Member

@nicmunroe thank you for the comment. One thing disappoints me with the traceparent and tracestate that I mentioned in the document:

As a note here, the promise of Trace Context specification doesn’t hold. Using header names tracestate and traceparent as a single word causes many issues like syntax error highlighting in IDEs. So in many places they are already treated as two words in variables naming and protocols.

So renaming to correlationcontext, tempting to keep consistent with the tracestate, may not be used the way we anticipate and recommend. So end users and protocol owners will start inserting delimiters anyway.

Renaming to baggage doesn't have this problem.

@danielkhan
Copy link
Contributor

The header will be renamed to baggage.

@dyladan
Copy link
Member

dyladan commented Jun 2, 2020

Should this be closed since we have #17 ?

@yurishkuro
Copy link
Member

+1 to close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants