Specify the initial propagators requirements. #735

carlosalberto · 2020-07-24T12:33:27Z

Fixes #637

Also specifies what's the expected location of TraceContext in the API.

anuraaga

This is informative and does indeed solve the original issue title of #637, but the recent comments seem to relate to components that aren't propagators, e.g., id generation. I guess people are hoping for a final word on those in the spec so I wouldn't eagerly close the issue with this.

specification/trace/sdk.md

carlosalberto · 2020-07-24T14:38:36Z

This is informative and does indeed solve the original issue title of #637, but the recent comments seem to relate to components that aren't propagators, e.g., id generation.

The issue is simply about Propagators, on which we had found an initial agreement a few weeks ago (during a SIG call). For the extra components (e.g. id generation), so should definitely start another issue and get the ball rolling ;)

anuraaga · 2020-07-24T14:40:41Z

Can you file the issue given you're about to close #637 without addressing the recent comments?

carlosalberto · 2020-07-24T14:48:34Z

@anuraaga Please open a new issue with what you want. I went and looked for id generation and saw nothing. Maybe you are talking about a different issue? The related issue title itself says: "Is it acceptable for AWS x-ray propagators to be hosted by the OpenTelemetry project?"

Other than that, I will fill an issue for updating the VENDORS section, as mentioned by @tsloughter (which, then again, might require its own iterations).

carlosalberto · 2020-07-24T14:49:56Z

Oh, never mind, I saw that the id generation part is included in Tristan's comment ;) So let's definitely cover that in another issue.

anuraaga · 2020-07-24T14:51:30Z

Thanks! If the PR didn't have the "Fixes #637" that wouldn't have mattered at all but want to make sure it's not lost.

carlosalberto · 2020-07-27T15:52:20Z

Updated this PR to NOT require the B3 Propagator to be included in the SDK, switching to have it included in an extension package (using the keyword SHOULD).

specification/trace/api.md

specification/correlationcontext/api.md

reyang

LGTM.

Co-authored-by: Reiley Yang <[email protected]>

specification/correlationcontext/api.md

carlosalberto · 2020-07-29T15:43:52Z

@yurishkuro Removed Baggage/CorrelationContext from the API requirements.

specification/trace/api.md

dyladan · 2020-07-30T15:44:23Z

specification/context/api-propagators.md

+`Propagator`s implementing officially supported protocols such as
+[B3 Propagation Specification](https://github.com/openzipkin/b3-propagation),
+as well as additionally offered `Propagator`s implementing well known
+protocols such as AWS X-Ray trace header protocol, SHOULD be part of


We talked about this in the JS SIG meeting this week, and the maintainers had a discussion together about this specific topic as well. For open protocols like B3 or zipkin, i think this is fine, but I am hesitant to merge support for proprietary protocols.

You can see my comment on a JS repository PR here: open-telemetry/opentelemetry-js-contrib#163 (comment)

I have discussed this with the other maintainers @obecny and @mayurkale22 and we would like to keep vendor-specific code out of our hosted repositories if at all possible. We are happy to code-review vendor propagators and exporters, but we would prefer that these code modules be hosted and deployed from vendor-owned repositories. As an example, you can look at what GCP has done for their cloud trace and cloud monitoring exporters and propagator here GoogleCloudPlatform/opentelemetry-operations-js. At the time this repository was created, we had similar discussions with the GCP folks.

@dyladan IIRC the old agreement we reached in a SIG call a few weeks ago was that having propagators will help with interoperability. Also, usually we don't have to be updating them, as they follow a specific, non-changing protocol.

Exporters are an entirely different thing, and we still do (and we will) document that such components have to exist in their vendor-specific repos.

FWIW, we have started bundling vendor-specific propagators (ottracer and xray) inside of the default Java auto-instrumentation distribution. I think it's a nice convenience for both vendors and users, but we can unbundle if the community reaches a different consensus.

@dyladan Any opinion? ;)

My objection is unchanged. If someone has special requirements for interop with different backends like xray or something, it will require them to do some additional setup anyway. Since vendors like AWS already need to host packages for their exporter anyway, this is just one additional thing for them to host in the same way.

@trask when you say ottracer do you mean OpenTracing? From my perspective, an OpenTracing propagator is fine to host, since it is an open initiative. xray is different because it is a proprietary, for-profit, vendor.

I see the point that @dyladan tries to make, but I agree with @trask and probably we should host them for the moment. Or maybe make a list of supported once.

I agree with @dyladan. How can we decide which vendor should be allowed and which one not ?. With general rule we would like to simply avoid such situation as otherwise we might endup with having dozens of propagators / exporters etc. connected with many different vendors in contrib repo. Maintaining them will be difficult and imho should be out of the otel scope. I have nothing against making a list of supported propagators etc. and simply add some links in readme so people can find them easily and use them based on their own responsibility.

@obecny I think you convinced me :). Maybe worth recommending vendors to write their propagators in similar way (reusable code that depends only on the API).

@trask Actually that's a format we started using in Lightstep prior to using B3 (with the intention of having it as a standard for OpenTracing) - hence, it can be considered a vendor specific one.

Also, @trask I think you still will be able to package the vendor propagator (AWS X-Ray, Lightstep or any other you guys want to support) without problems, similarly in how you are importing the trace-propagators package at this moment (hopefully!)

I think you still will be able to package the vendor propagator (AWS X-Ray, Lightstep or any other you guys want to support) without problems

If the decision is to not include vendor-specific propagators in the otel repos, then it seems like the same logic should apply to what we include in the javaagent distribution.

bogdandrutu · 2020-08-04T20:41:53Z

specification/context/api-propagators.md

+`Propagator`s implementing officially supported protocols such as
+[B3 Propagation Specification](https://github.com/openzipkin/b3-propagation),
+as well as additionally offered `Propagator`s implementing well known
+protocols such as AWS X-Ray trace header protocol, SHOULD be part of


I see the point that @dyladan tries to make, but I agree with @trask and probably we should host them for the moment. Or maybe make a list of supported once.

yurishkuro · 2020-08-05T02:02:23Z

+1 for not including vendor-specific propagation formats in the SDKs.

yurishkuro

For a small PR this invites a lot of controversy. I recommend closing it and discussing two different aspects separately:

should there be any propagation implementation included in the API?
vendor-specific extensions should not be part of API or SDK

yurishkuro · 2020-08-05T02:09:00Z

specification/context/api-propagators.md

@@ -266,3 +267,11 @@ Sets the global `Propagator` instance.
 Required parameters:

 - A `Propagator`. This usually will be a composite instance.
+
+## Included Propagators Distribution


This section looks very confused.

why does it discuss X-Ray here? If X-Ray format were to be discussed at all, would it not be in the trace/api file?

B3 format does support baggage so with a bit of a stretch I can see why it would be mentioned here (in "context" dir), but why is it not mentioned in trace/api file?

The section title is "Included", making you think these are included in the API. However, the paragraph ends with "SHOULD be part of official OpenTelemetry extension packages." - then what is it doing in the API file?

Good point actually. I assume this spec was written with only SpanContext propagation in mind? If so, this should be called out explicitly.

A better place may be around here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/library-guidelines.md#protocol-exporters

@Oberon00 We can move this (updated) section there, sure.

carlosalberto · 2020-08-05T14:07:12Z

@yurishkuro This PR intends to simply reflect the current state and the recommended location for propagators. There's another PR that will solve the issue on whether API should or not include (and do) out of the box propagation: #721

anuraaga · 2020-08-05T23:55:30Z

@bogdandrutu I'm concerned about the quick merge on this PR (yes I know I'm usually talking about the opposite :P). e7abd4a is a huge change in the definition of the spec made 10 hours ago. This is after @yurishkuro's suggestion to close this PR and split into two here #735 (review) - I was waiting to see whether this would be split or continue before providing more comments. But instead it seems the spec got changed and merged overnight :(

carlosalberto · 2020-08-06T00:26:19Z

If @bogdandrutu hadn't merged it, I would have done myself ;) The reason for this was a second discussion in the Specification meeting around this (with great feedback from JS maintainer @dyladan ), along @yurishkuro's approval given the fact I mentioned there's a related issue already.

More context: on last Monday there was A LOT of feedback regarding the lack of velocity regarding the Specification issues/PRs handling, and there was a call to a) Reduce scope in PRs, so we can move fast (follow ups are suggested for further discussion/tunning), and b) A call to the maintainers to make a 'hard' call when things go slow (as it had happened with this PR).

So please open a follow up issue if you want to propose changes around this ;) (btw, I still have a follow up to move the vendor propagators to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/library-guidelines.md#protocol-exporters, as suggested)

anuraaga · 2020-08-06T00:45:56Z

@carlosalberto OK, but definitely need to at least summarize the actual discussion that happened in the SIG meeting or a link to concrete notes about it. Many people can't join SIG meetings, due to time zone or other obligations, so they're effectively not an open forum for discussion, unlike offline conversation on GitHub. Making 'hard' calls is hard - not because of the actual decision making process but communicating it with empathy, an active desire to understand and support another party, something I find is sorely lacking among the committee members here...

carlosalberto · 2020-08-06T13:58:17Z

OK, but definitely need to at least summarize the actual discussion that happened in the SIG meeting or a link to concrete notes about it

Good call, will remember on that. Thanks for the feedback!

* Specify the initial propagators requirements. * Do not require the SDK to include the B3 protocol. * Update specification/trace/api.md Co-authored-by: Reiley Yang <[email protected]> * Update specification/correlationcontext/api.md Co-authored-by: Reiley Yang <[email protected]> * Remove the CorrelationContext propagator requirement. * Make TraceContext optional in the API. * Update the propagators location. * Fix typo. Co-authored-by: Reiley Yang <[email protected]>

Specify the initial propagators requirements.

f192f26

carlosalberto added area:api Cross language API specification issue area:sdk Related to the SDK spec:trace Related to the specification/trace directory release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Jul 24, 2020

carlosalberto requested review from a team July 24, 2020 12:33

anuraaga reviewed Jul 24, 2020

View reviewed changes

specification/trace/sdk.md Outdated Show resolved Hide resolved

Do not require the SDK to include the B3 protocol.

62836a6

open-telemetry deleted a comment from cijothomas Jul 27, 2020

reyang reviewed Jul 27, 2020

View reviewed changes

specification/trace/api.md Outdated Show resolved Hide resolved

reyang reviewed Jul 27, 2020

View reviewed changes

specification/correlationcontext/api.md Outdated Show resolved Hide resolved

reyang approved these changes Jul 27, 2020

View reviewed changes

carlosalberto and others added 2 commits July 27, 2020 19:07

Update specification/trace/api.md

3b1c0da

Co-authored-by: Reiley Yang <[email protected]>

Update specification/correlationcontext/api.md

0f7cc66

Co-authored-by: Reiley Yang <[email protected]>

yurishkuro reviewed Jul 27, 2020

View reviewed changes

specification/correlationcontext/api.md Outdated Show resolved Hide resolved

Remove the CorrelationContext propagator requirement.

fd44e8a

Oberon00 reviewed Jul 29, 2020

View reviewed changes

specification/trace/api.md Show resolved Hide resolved

Make TraceContext optional in the API.

837897c

Oberon00 approved these changes Jul 30, 2020

View reviewed changes

anuraaga approved these changes Jul 30, 2020

View reviewed changes

dyladan reviewed Jul 30, 2020

View reviewed changes

MrAlias mentioned this pull request Jul 31, 2020

Move the B3 propagator out of the API open-telemetry/opentelemetry-go#1012

Closed

bogdandrutu approved these changes Aug 4, 2020

View reviewed changes

yurishkuro requested changes Aug 5, 2020

View reviewed changes

carlosalberto added 2 commits August 5, 2020 15:49

Update the propagators location.

e7abd4a

Fix typo.

373a228

yurishkuro approved these changes Aug 5, 2020

View reviewed changes

bogdandrutu approved these changes Aug 5, 2020

View reviewed changes

bogdandrutu merged commit 3891524 into open-telemetry:master Aug 5, 2020

carlosalberto mentioned this pull request Oct 1, 2020

Revert #1043 (Third Party Propagators location) #1047

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify the initial propagators requirements. #735

Specify the initial propagators requirements. #735

carlosalberto commented Jul 24, 2020 •

edited

Loading

anuraaga left a comment

carlosalberto commented Jul 24, 2020

anuraaga commented Jul 24, 2020

carlosalberto commented Jul 24, 2020 •

edited

Loading

carlosalberto commented Jul 24, 2020

anuraaga commented Jul 24, 2020

carlosalberto commented Jul 27, 2020

reyang left a comment

carlosalberto commented Jul 29, 2020

dyladan Jul 30, 2020

carlosalberto Jul 30, 2020

trask Jul 30, 2020

carlosalberto Jul 31, 2020

dyladan Aug 4, 2020

bogdandrutu Aug 4, 2020

obecny Aug 4, 2020

bogdandrutu Aug 4, 2020

carlosalberto Aug 4, 2020

trask Aug 4, 2020

bogdandrutu Aug 4, 2020

yurishkuro commented Aug 5, 2020

yurishkuro left a comment

yurishkuro Aug 5, 2020

Oberon00 Aug 5, 2020

Oberon00 Aug 5, 2020

carlosalberto Aug 5, 2020

carlosalberto commented Aug 5, 2020

anuraaga commented Aug 5, 2020

carlosalberto commented Aug 6, 2020

anuraaga commented Aug 6, 2020

carlosalberto commented Aug 6, 2020

Specify the initial propagators requirements. #735

Specify the initial propagators requirements. #735

Conversation

carlosalberto commented Jul 24, 2020 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

carlosalberto commented Jul 24, 2020

anuraaga commented Jul 24, 2020

carlosalberto commented Jul 24, 2020 • edited Loading

carlosalberto commented Jul 24, 2020

anuraaga commented Jul 24, 2020

carlosalberto commented Jul 27, 2020

reyang left a comment

Choose a reason for hiding this comment

carlosalberto commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Aug 5, 2020

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Aug 5, 2020

anuraaga commented Aug 5, 2020

carlosalberto commented Aug 6, 2020

anuraaga commented Aug 6, 2020

carlosalberto commented Aug 6, 2020

carlosalberto commented Jul 24, 2020 •

edited

Loading

carlosalberto commented Jul 24, 2020 •

edited

Loading