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

Change w3c correlation context to custom header #517

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Mar 17, 2020

After discussions with the W3C Distributed Tracing Working Group, it has been determined that the Correlation-Context header is not ready for implementation and is being actively changed. As recommended by the working group, we should implement our own header until the specification is an official recommendation.

/cc @tedsuo @mwear

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this. QQ: should this be part of the default API?

@dyladan
Copy link
Member Author

dyladan commented Mar 18, 2020

I am fine with this. QQ: should this be part of the default API?

Think this discussion is in #428

@bogdandrutu
Copy link
Member

@SergeyKanzhelev please review. also @open-telemetry/specs-approvers :)

@dyladan
Copy link
Member Author

dyladan commented Mar 25, 2020

The W3C Distributed Tracing WG is discussing this issue this week. Please do not merge this PR yet.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting changing to W3C editor draft format

@dyladan
Copy link
Member Author

dyladan commented Mar 26, 2020

@tedsuo see w3c/baggage#18

The WG decided not to go with IETF dict, and since this isn't merged yet it isn't too late to change

@dyladan dyladan force-pushed the correlation-header branch 2 times, most recently from fe91d70 to 951bd06 Compare March 27, 2020 15:05
@dyladan
Copy link
Member Author

dyladan commented Mar 30, 2020

@yurishkuro would appreciate a ✅ here

@yurishkuro
Copy link
Member

btw, why not otcorreations to match the Correlations API?

@dyladan
Copy link
Member Author

dyladan commented Apr 7, 2020

Originally I had been trying to keep the header name as similar as possible to the name I thought the w3c would likely end up with. Because of the recent naming discussions, I think it's clear that we should just use whatever header name makes sense for us.

@dyladan
Copy link
Member Author

dyladan commented Apr 10, 2020

Rebased with name otbaggage PTAL and merge when you're happy

@MrAlias
Copy link
Contributor

MrAlias commented Apr 10, 2020

@dyladan: it seems like the above poll shows otcorrelations as the prefered header name for the time being. Why did we switch to otbaggage?

@dyladan
Copy link
Member Author

dyladan commented Apr 10, 2020

@MrAlias oh man I just had a brain fart I guess. updating now.

@dyladan
Copy link
Member Author

dyladan commented Apr 15, 2020

Can't see why the build is failing. Just getting a spinner in circle ci that says 0 jobs in the workflow.

image

@MrAlias
Copy link
Contributor

MrAlias commented Apr 15, 2020

What I'm seeing:

#!/bin/bash -eo pipefail
make markdown-lint
make enforce-markdown-link-check
markdownlint -c .markdownlint.yaml '**/*.md'
markdown-link-check FAILED => errors:

Run 'make markdown-link-check' to see the errors
Makefile:30: recipe for target 'enforce-markdown-link-check' failed
make: *** [enforce-markdown-link-check] Error 1

Exited with code exit status 2

@MrAlias
Copy link
Contributor

MrAlias commented Apr 15, 2020

That Makefile command is not really helpful. Running manually:

$ find . -name \*.md -exec markdown-link-check {} 2>&1 \;
...
FILE: ./specification/context/api-propagators.md
[✓] #overview
[✓] #http-text-format
[✓] #fields
[✓] #inject
[✓] #setter
[✓] #set
[✓] #extract
[✓] #getter
[✓] #get
[✓] #composite-propagator
[✓] #global-propagators
[✖] api-correlationcontext.md#serialization

12 links checked.

ERROR: 1 dead links found!
[✖] api-correlationcontext.md#serialization → Status: 400
...

Copy link
Member

@lizthegrey lizthegrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "ot" as a prefix potentially confusing when opentracing still exists but is deprecated?

@dyladan
Copy link
Member Author

dyladan commented Apr 28, 2020

@lizthegrey would you rather we use otelbaggage? It's only another 2 bytes on the header so I don't think we need to worry too much about payload size. I typically think of ot as OpenTelemetry not OpenTracing, but that's just me and I know not everyone feels that way.

@yurishkuro
Copy link
Member

there was never otbaggage in opentracing, so I don't think there's much confusion.

@mwear
Copy link
Member

mwear commented Apr 28, 2020

There seems to be some continued confusion about what we're actually changing the header name to. I see discussions about otbaggage, but this PR currently changes it to otcorrelations. I think that is due to #517 (comment).

@dyladan
Copy link
Member Author

dyladan commented Apr 29, 2020

There seems to be some continued confusion about what we're actually changing the header name to. I see discussions about otbaggage, but this PR currently changes it to otcorrelations. I think that is due to #517 (comment).

Yes my fault. otcorrelations is the name we decided on in my informal poll, I just can't get baggage out of my head because that's what the WG is going towards.

@dyladan
Copy link
Member Author

dyladan commented May 5, 2020

Is this waiting on something to merge?

@MrAlias
Copy link
Contributor

MrAlias commented May 14, 2020

@open-telemetry/technical-committee can we merge this so language SIGs have direction on this?

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

Successfully merging this pull request may close these issues.

10 participants