Define OTLP protocol driver registration mechanism #3722
Conversation
pellared
changed the title
|
@yurishkuro Do you think such changes would make the specification better? It is a draft because I would first would like you to review it. |
pellared
changed the title
|
Based on @yurishkuro initial review I think this it is ready for wider feedback. "Undrafting" |
|
Important: this is a "Stable" document. The proposed changes seem to be breaking from end user perspective. The now must do something they previously were not required (register a "driver"). Breaking changes to "Stable" documents are not allowed. Maybe see if you can rephrase it so that it is not a breaking change. Minor: I am not sure we use the term "driver" anywhere else. Do we need to introduce it? Can we use "implementation" instead? |
How is it breaking? See #3722 (comment). Maybe this would be better: #3722 (comment)
I can do it, but I think a "driver" is a good term (abstraction) for it. We have a driver for a protocol like we have database drivers or hardware drivers. |
| If no configuration is provided the default transport SHOULD be `http/protobuf` | ||
| unless SDKs have good reasons to choose `grpc` as the default (e.g. for backward | ||
| compatibility reasons when `grpc` was already the default in a stable SDK | ||
| release). | ||
|
|
||
| The exporter SHOULD NOT depend on any transport protocol driver. | ||
| The exporter SHOULD have a mechanism to register transport protocol driver. | ||
| The caller SHOULD explicitly register the driver for the default transport protocol if one wants to support it. |
There was a problem hiding this comment.
@tigrannajaryan @yurishkuro would this be better?
| The caller SHOULD explicitly register the driver for the default transport protocol if one wants to support it. | |
| By default, no driver SHOULD be registered. The exporter SHOULD require the caller to explicitly register the driver for the default transport protocol if the caller wants to support it. |
@tigrannajaryan, I updated the description and gave a reference to a comment where I explain why I do not see it as a breaking change. |
This assumes it is OK to make breaking changes to the "Stable" spec as long as we don't require stable SDKs to implement them. I don't think we discussed in the past and I am not sure we want to allow it. @open-telemetry/specs-approvers I think we need to have a position on this: are breaking changes to the spec's "Stable" document allowed provided that we don't require stable SDK implementations to adopt such changes? My first thought is that this should not be allowed and a slippery slope, but I think it is worth discussing. If we allow it then possibly we need to:
This may be doable but introduces complexity which I am not sure we want. There are probably also other approaches to handle such changes. Thoughts? |
Just for reference I think it already happened before (but it was not clearly described). See #3717.
Whatever the decision is. I think it would be a good practice anyway as I have found out that SDKs are not always keeping up with the specification. I think the specification should recommend some design decisions that are based on "lessons learned" from existing SDKs to make new implementations better if the differences between "old" and "new" are reasonable and worth adoption in new implementations. Also it is worth to notice that, in my opinion, most SDKs do not follow the OTLP exporter specification as I described under #3721
The specification says that it is the exporter that has to support EDIT: I would say that only OTLP exporters in Erlang and Rust have a potential to be compliant with the current specification as these are the only languages that support all |
|
Just want to voice that a few SIGs offer this as separate artifacts/packages, e.g. |
|
JS provides gRPC, http/protobuf, and http/JSON for all signals. We were the first and may still be the only, aside from the collector, to support all 3 "drivers". Each signal/driver pair has a separate package that must be installed if you want to use it in order to reduce dependencies. |
|
@pellared this was discussed at the TC meeting. The consensus was that we do not want that specific wording that allows stable SDKs to opt-out of the requirement, as this would lead to bifurcation of capabilities in different SDKs in the long term and set a negative precedent. Instead, it was noted that the current phrasing of the spec is intentionally vague such that it does not require supporting every flavor that can be specified in the config. As an example, Zipkin/Jaeger exporters were cited which are required to be supported but not so that they always have to be statically linked. The proposal is to find a phrasing that double-clicks into this gray area to clarify that SDKs are allowed to implement support for different capabilities via opt-in model (as some already do), but are not required to do so. |
|
I am changing this PR to a draft. |
|
Closing in favor of #3730 |
…er, SDK, or separate component (#3730) Fixes #3721 ## Why The Go SIG is working towards stabilizing the OTLP metrics exporter. The Go SIG would prefer to manage the exporter environment variables through a distinct package: https://pkg.go.dev/go.opentelemetry.io/contrib/exporters/autoexport. This approach is akin to [Java Autoconfigure](https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/autoconfigure). The rationale behind this decision is as follows: 1. If users aim to utilize the `OTEL_EXPORTER_OTLP_PROTOCOL` and `OTEL_TRACES_EXPORTER`, we aim to support all the values documented in the specification. We want to ensure that users are not prone to encountering runtime errors if a protocol driver hasn't been registered in the code. 2. Simultaneously, we wish to avoid applications to depend on all exporter implementations defined in the specification. Currently, it is not clear of such design is in compliance with the specification. ## What Define that **configuration options MAY be implemented by exporter, SDK, or separate component**. While this PR may be seen as a breaking change, because of the way how the languages adopted the specification I would say that this is a "clarification" or "adopting to the reality". Here is how different languages currently implement the OTLP configuration options. ### .NET Configuration options implemented by exporter. Side note: Per-signal endpoint configuration options are not implemented. Source code: - https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol ### C++ Most configuration options implemented by exporter. However, the `*_PROTOCOL` env vars are not implemented at all. See: open-telemetry/opentelemetry-cpp#971. Source code: - https://github.com/open-telemetry/opentelemetry-cpp/tree/main/exporters/otlp ### Erlang Configuration options implemented by exporter. Source code: - https://github.com/open-telemetry/opentelemetry-erlang/tree/main/apps/opentelemetry_exporter ### Go Most configuration options implemented by exporter. However, the `*_PROTOCOL` env vars are implemented by a separate component (autoexport) Source code (package docs): - https://pkg.go.dev/go.opentelemetry.io/contrib/exporters/autoexport - https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace ### Java Configuration options implemented by an autoconfigure component. Source code: - https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/autoconfigure - https://github.com/open-telemetry/opentelemetry-java/tree/main/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp ### JavaScript Most configuration options implemented by exporter. However, the `*_PROTOCOL` env vars are implemented by a separate component (opentelemetry-sdk-node) Source code: - https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-sdk-node - https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/exporter-trace-otlp-http ### PHP Configuration options implemented by exporter. Source code: - https://github.com/open-telemetry/opentelemetry-php/tree/main/src/Contrib/Otlp ### Python Most configuration options implemented by exporter. However, the `*_PROTOCOL` env vars are implemented by the SDK. Source code: - https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py - https://github.com/open-telemetry/opentelemetry-python/tree/main/exporter ### Ruby Most configuration options implemented by exporter. However, the `*_PROTOCOL` env vars are implemented by the SDK. Source code: - https://github.com/open-telemetry/opentelemetry-ruby/blob/main/sdk/lib/opentelemetry/sdk/configurator.rb - https://github.com/open-telemetry/opentelemetry-ruby/tree/main/exporter/otlp-http ### Rust Configuration options implemented by exporter. Source code: - https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-otlp ### Swift Env vars not supported. ### Previous work and discussions - #3721 - #3722
Fixes #3721
I do not see it as a breaking change. Reasoning here.