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

Clarify if the OTLP exporter has to support all protocol related env vars #3721

Closed
pellared opened this issue Oct 13, 2023 · 10 comments · Fixed by #3730
Closed

Clarify if the OTLP exporter has to support all protocol related env vars #3721

pellared opened this issue Oct 13, 2023 · 10 comments · Fixed by #3730
Assignees
Labels
spec:protocol Related to the specification/protocol directory

Comments

@pellared
Copy link
Member

pellared commented Oct 13, 2023

https://github.com/open-telemetry/opentelemetry-specification/blob/541b2ffe54ca416732aac5015c6b57ac3a678ae0/specification/protocol/exporter.md

Problem statement

Is it necessary for the OTLP exporter to intentionally support OTEL_EXPORTER_OTLP_PROTOCOL, OTEL_EXPORTER_OTLP_TRACES_PROTOCOL, OTEL_EXPORTER_OTLP_METRICS_PROTOCOL, OTEL_EXPORTER_OTLP_LOGS_PROTOCOL?

Analysis

This requirement would create a dependency for the OTLP exporter on all the libraries utilized for implementing the transport protocols (such as the HTTP client, JSON parser, protobuf parser, and gRPC client). This means that users who only require the HTTP protocol might not want to introduce any gRPC dependencies to their application. In certain programming languages, like Go (as mentioned in this paper), it is customary to minimize the number of dependencies. This approach also helps reduce the overall "security surface" of the components. It's important to note that if a gRPC library used by the exporter had a CVE, the entire exporter would be considered vulnerable as well (since most security static analysis tools solely check dependencies and not how the code is structured).

Currently, most languages (such as C++, Go, Java, Python, JavaScript, and Ruby) follow the principle of minimizing the number of dependencies, with a separate OTLP exporter implementation for each protocol. Currently, the specification does not align with these OTLP exporters, making them non-compliant. Most of these languages have additional (separate) "configurator" components that enable the creation of exporters selected via environmental variables (for instance, OTEL_TRACES_EXPORTER and OTEL_EXPORTER_OTLP_PROTOCOL).

However, in some languages, an OTLP exporter supports multiple protocols (.NET, Erlang, PHP, Rust). It's worth mentioning that for Rust, optional dependencies are supported, and it makes complete sense to structure it that way.

Proposal

I believe that the decision of how OTEL_EXPORTER_OTLP_PROTOCOL, OTEL_EXPORTER_OTLP_TRACES_PROTOCOL, OTEL_EXPORTER_OTLP_METRICS_PROTOCOL, OTEL_EXPORTER_OTLP_LOGS_PROTOCOL should be supported should be made by the language Special Interest Group (SIG).

I suggest updating the specification to indicate that OTEL_EXPORTER_OTLP_PROTOCOL, OTEL_EXPORTER_OTLP_TRACES_PROTOCOL, OTEL_EXPORTER_OTLP_METRICS_PROTOCOL, OTEL_EXPORTER_OTLP_LOGS_PROTOCOL SHOULD be supported, with a footnote explaining that the support for these environment variables could be provided through separate components.

@pellared pellared added the spec:protocol Related to the specification/protocol directory label Oct 13, 2023
@pellared pellared changed the title Clarify if the OTLP exporter has to support all protocols and protocol related env vars Clarify if the OTLP exporter has to support all protocol related env vars Oct 13, 2023
@yurishkuro
Copy link
Member

Sorry, I am missing the connection between "all env vars" (for different OTEL signals) vs. "all protocols".

On the latter, I do not believe this requires unnecessary coupling of dependencies. The SDK can be organized in such a way that when an exporter for specific protocol is imported into user app, it registers itself with the SDK. So if I only import HTTP exporter, then the only valid value for the env variable would be "http", using "grpc" would raise an error.

@pellared
Copy link
Member Author

pellared commented Oct 13, 2023

@yurishkuro, first of all thanks a lot from your comment, I find it very insightful 👍 I have a few questions for clarification.

The SDK can be organized in such a way that when an exporter for specific protocol is imported into user app, it registers itself with the SDK.

Do you mean the SDK or the OTLP exporter? The specification expects the env var support from the OTLP exporter implementation itself.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md?plain=1#L9

Still, I think get your idea and I have an idea how it could be implemented. I changed your recommendation a little - do you agree with it (changes are bold)?

The exporter can be organized in such a way that when a specific protocol driver is imported into user app, it registers itself with the the exporter. So if I only import HTTP protocol driver, then the only valid value for the env variable would be "http", using "grpc" would raise an error.

I do not think that the SDK should know anything about the OTLP exporter env vars.

PS. Currently, in Go we have such registration design in a separate package. In my opinion it is not compliant with the specification. Such mechanism registration would have to live in otlptrace and otlpmetric packages.

@yurishkuro
Copy link
Member

+1 to the quoted framing.

It's probably worth implementing the registration mechanism at the SDK level such that it's only done once, not in different way by every exporter. For example, at the SDK level we can have the same mechanism for selecting which exporter to use (OTLP or Debug, for instance). This would also work well with the upcoming configuration mechanism - without it the Configurator class (or whatever) would need to import everything, which will defeat the min-deps purpose you're trying to achieve here.

@pellared
Copy link
Member Author

@yurishkuro, just to double-check.

SDK should have registration mechanism for OTEL_TRACES_EXPORTER, OTEL_METRICS_EXPORTER, OTEL_LOGS_EXPORTER.

OTLP exporters should have registration mechanism for OTEL_EXPORTER_OTLP_PROTOCOL OTEL_EXPORTER_OTLP_TRACES_PROTOCOL OTEL_EXPORTER_OTLP_METRICS_PROTOCOL OTEL_EXPORTER_OTLP_LOGS_PROTOCOL.

@chameleon82
Copy link

Should OTEL_EXPORTER_OTLP_PROTOCOL be validated if none is specified for OTEL_{SIGNAL}_EXPORTER ?

Should otlp (grpc or http) be explicitly added as a dependency as a default exporter?

@pellared
Copy link
Member Author

pellared commented Oct 17, 2023

Should OTEL_EXPORTER_OTLP_PROTOCOL be validated if none is specified for OTEL_{SIGNAL}_EXPORTER?

IMO it is not necessary. In such scenario the OTEL_EXPORTER_OTLP_PROTOCOL will not be used.

Should otlp (grpc or http) be explicitly added as a dependency as a default exporter?

No. See #3721 (comment). For example if given language's OTLP implementation uses http/protobuf as a default protocol and the user would NOT register the HTTP protocol driver then the OTLP exporter factory may return an error saying that given protocol is unknown. I think it makes sense as the user may not want to support/use/depend on the default one and e.g. use only a single protocol.

@pellared
Copy link
Member Author

pellared commented Oct 17, 2023

I created a PR #3722.

@pellared
Copy link
Member Author

pellared commented Oct 19, 2023

We discussed this matter at the Go SIG meeting.

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

I intend to participate in the next Specification SIG meeting to deliberate on this matter with a broader audience.

@yurishkuro
Copy link
Member

manage the exporter environment variables through a distinct package

Yeah, that's the appropriate solution. My concern is whether this approach was also taken by other SDKs or did they follow the spec verbatim and bundled this into the core exporter - it's now hard to unbundle without breaking behavior.

@pellared
Copy link
Member Author

Yeah, that's the appropriate solution.

I created #3730

My concern is whether this approach was also taken by other SDKs or did they follow the spec verbatim and bundled this into the core exporter - it's now hard to unbundle without breaking behavior.

I did my best to describe how all languages implemented the OTLP configuration options.

carlosalberto pushed a commit that referenced this issue Oct 30, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:protocol Related to the specification/protocol directory
Projects
None yet
4 participants