Opentelemetry-specification: Is it acceptable for AWS x-ray propagators to be hosted by the OpenTelemetry project?

Created on 5 Jun 2020  路  14Comments  路  Source: open-telemetry/opentelemetry-specification

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.

p2 required-for-ga context trace

Most helpful comment

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?

All 14 comments

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

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.

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?

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.

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.

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

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.

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 (?).

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).

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

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?

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

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.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

maxgolov picture maxgolov  路  4Comments

cijothomas picture cijothomas  路  3Comments

naseemkullah picture naseemkullah  路  5Comments

carlosalberto picture carlosalberto  路  4Comments

pavolloffay picture pavolloffay  路  4Comments