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

Is it acceptable for AWS x-ray propagators to be hosted by the OpenTelemetry project? #637

Closed
jkwatson opened this issue Jun 5, 2020 · 14 comments · Fixed by #735
Closed
Assignees
Labels
priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory

Comments

@jkwatson
Copy link
Contributor

jkwatson commented Jun 5, 2020

The Java project has gotten a PR to include an AWS x-ray propagator. I just want to confirm that it is ok to have this vendor-specific logic hosted by OpenTelemetry.

@dyladan
Copy link
Member

dyladan commented Jun 5, 2020

Just as a data point, the JS sig asked gcp to host their exporter themselves (and they have done so) https://github.com/GoogleCloudPlatform/opentelemetry-operations-js/tree/master/packages

@jkwatson
Copy link
Contributor Author

jkwatson commented Jun 5, 2020

Right, exporters are right-out from commercial vendors. I think that part is clear. And, open-source "vendors" are also just fine. But this is a new case that I hadn't seen before: a commercial vendor propagator.

@anuraaga
Copy link
Contributor

anuraaga commented Jun 6, 2020

For reference, xray propagator is probably not so different in concept from the x-ray ids generators and AWS resources.

https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk_contrib/aws_v1_support/src/main/java/io/opentelemetry/sdk/contrib/trace/aws/AwsXRayIdsGenerator.java

So while I'd understand a decisions to move to a separate repo, I'm assuming this applies to all of these.

One point that doesn't seem consistent is that the stackdriver JS SDK exporter was not allowed into contrib but the stackdriver otcollector exporter is. Is there a reason to be inconsistent there?

@carlosalberto
Copy link
Contributor

One point that doesn't seem consistent is that the stackdriver JS SDK exporter was not allowed into contrib but the stackdriver otcollector exporter is. Is there a reason to be inconsistent there?

If you are talking about all the Collector exporters, I think that's a historical reason (maybe with eventual intention to separate them later on). But @tigrannajaryan or @bogdandrutu might know better.

@dyladan
Copy link
Member

dyladan commented Jun 9, 2020

One point that doesn't seem consistent is that the stackdriver JS SDK exporter was not allowed into contrib but the stackdriver otcollector exporter is. Is there a reason to be inconsistent there?

I can't speak for the collector, but at the time that decision was made there was no JS contrib repo. There isn't any real guidance on this from the TC so each SIG has essentially been making these decisions on their own. To me, this comes down to maintenance burden. Who will be updating it if it is broken or needs to be updated for some new format? If it is expected that the vendor maintain it, then they should host it. As a SIG maintainer I feel I am responsible for the quality and timeliness of releases for code hosted in the OTel repositories, including contrib.

@shengxil
Copy link

For reference, the X-Ray propagator is now placed under contrib next to B3 and Jaeger propagator https://github.com/open-telemetry/opentelemetry-java/tree/master/contrib/trace_propagators/src/main/java/io/opentelemetry/contrib/trace/propagation

@toumorokoshi
Copy link
Member

toumorokoshi commented Jun 11, 2020

As another data point, Python has been allowing 3rd party exporters / propagators / etc in the core repo, under the "ext" directory:

https://github.com/open-telemetry/opentelemetry-python/tree/master/ext

We do want to move 3rd parties out eventually (vendors to the respective company, instrumentations to a contrib repo as well), but lack of tooling as well as a changing API made it more practical to include them for now.

we're targetting moving them out as one of the tasks around GA.

@bogdandrutu bogdandrutu added the spec:trace Related to the specification/trace directory label Jun 12, 2020
@carlosalberto carlosalberto added spec:context Related to the specification/context directory and removed spec:trace Related to the specification/trace directory labels Jun 12, 2020
@mtwo
Copy link
Member

mtwo commented Jun 16, 2020

Decision from today's specification meeting: propagators, even ones that are for specific cloud-providers or APM vendors, should be stored in the main API / SDK repository. Rationale:

  • Propagators are small pieces of code and their functionality is publicly documented (unlike exporters).
  • People will often need to use propagators that are not specific to their tracing or metrics vendor. For example, customers of New Relic or Datadog may still want to use an AWS propagator for requests to AWS APIs.
  • Only a small number of propagators will need to exist, and this number will shrink as vendors and users shift to W3C TraceContext.

The specs SIG also suggested that propagators be downloaded as separate packages, though I'm guessing that we'll always want to include the B3 and W3C propagators in the base packages (?).

@carlosalberto
Copy link
Contributor

though I'm guessing that we'll always want to include the B3 and W3C propagators in the base packages

Yet another point to discuss, i.e. the B3 propagator case - I see it's located in different places among different languages (e.g. in Python it's part of the SDK, and in Java it's in an extra tracer-propagators package).

@dyladan
Copy link
Member

dyladan commented Jun 18, 2020

One more data point: In JS we also have B3 as part of the SDK

@carlosalberto carlosalberto added 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 2, 2020
@carlosalberto
Copy link
Contributor

I went to take a look and this is the current state, to augment what @dyladan mentioned:

Java:

  • TraceContext in the API
  • B3, Jaeger, AWS in an official extension package.

Python:

  • TraceContext in the API
  • B3 in the SDK

Go:

  • TraceContext and B3 in the API.

So before I finish cooking a PR to update the specification, I'd like to get an agreement on the official location. Two obvious choices on the above are:

  1. TraceContext in the API, B3 in the SDK, other ones in official extension packages.
  2. TraceContext and B3 in the API, other ones in official extension packages.

To me, 2) seems like the best option. Opinions?

@andrewhsu andrewhsu added priority:p3 Lowest priority level priority:p2 Medium priority level and removed priority:p3 Lowest priority level labels Jul 17, 2020
@dyladan
Copy link
Member

dyladan commented Jul 21, 2020

What is meant by "official extension packages"? Are these hosted in our github org or by the companies?

@tsloughter
Copy link
Member

The Vendors doc should be expanded to include what is acceptable here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/vendors.md

In addition to whether or not to include vendor specific span processors or exporters in an Otel project I think it must be declared in the Vendors doc that a service saying it has "Support for OpenTelemetry" must not include the caveat that their span processor be used, whether it is for id generation or whatever. Vendor specific processors must be optional for improving the experience or else they simply "Implement OpenTelemetry".

Point being that "Support for OpenTelemtry" should mean that any of the Otel implementations works with said service.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants