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

Setting any value to OTEL_EXPORTER_OTLP_PROTOCOL has no effect. #2586

Closed
moriyoshi opened this issue Apr 4, 2022 · 7 comments · Fixed by #2893 or open-telemetry/opentelemetry-python-contrib#1250
Assignees
Labels
bug Something isn't working good first issue Good first issue help wanted

Comments

@moriyoshi
Copy link

Describe your environment

$ uname -srvmpio
Linux 4.19.128-microsoft-standard #1 SMP Tue Jun 23 12:58:10 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
$ python -VV
Python 3.9.9 (main, Dec 19 2021, 23:35:26)
[GCC 9.3.0]
$ grep ^opentelemetry pyproject.toml
opentelemetry-sdk = "^1.10.0"
opentelemetry-exporter-otlp-proto-http = "^1.10.0"
opentelemetry-exporter-otlp-proto-grpc = "^1.10.0"
opentelemetry-sdk-extension-aws = "^2.0.1"
opentelemetry-distro = "^0.29b0"
opentelemetry-instrumentation-django = "^0.29b0"

Steps to reproduce
I am using opentelemetry-instrument to wire up the components.

What is the expected behavior?
According to the documentation, one may choose the transport protocol for the OTLP exporter through OTEL_EXPORTER_OTLP_PROTOCOL, but it doesn't seem to apply to OTel Python, as the variable is referenced from nowhere.

What is the actual behavior?
Instead I had to set OTEL_*_EXPORTER to either otlp_proto_grpc or otlp_proto_http to change the protocol.

Additional context
The current SDK's architecture for the implementation lookup is not great IMO. This needs to be addressed.

@ronyis
Copy link
Contributor

ronyis commented Aug 18, 2022

Hey, I would like to take this PR.

Some follow-up questions:

  1. Do the options otlp_proto_grpc and otlp_proto_http should be removed for OTEL_*_EXPORTER? As far as I understand, this is behavior in Java SDK autoconfigure. It also makes more sense, otherwise users could define conflicting config by also using OTEL_EXPORTER_OTLP_PROTOCOL.

  2. Does building the logs exporter should be based on OTEL_EXPORTER_OTLP_PROTOCOL and OTEL_EXPORTER_OTLP_LOGS_PROTOCOL? As it's not defined in the specs.

Thanks!

@srikanthccv
Copy link
Member

The env would have more sense when we had one package. We decided to split the single package to multiple packages based on protocol + encoding so we have separate packages for gRPC, HTTP. I do not think this can easily be addressed without any breaking change.

@ronyis
Copy link
Contributor

ronyis commented Aug 19, 2022

The SDK already supports initializing the configured exporter from the relevant package (in this module).
So I think this issue can be solved without changing the exporters packages structure.

But I do think that the breaking change required here is to remove the support for otlp_proto_grpc and otlp_proto_http for OTEL_*_EXPORTER.

@ronyis
Copy link
Contributor

ronyis commented Aug 24, 2022

After some discussions, adding an overview of my proposed changes (all are related to traces, metrics & logs):

  1. Updating SDK configurator to support the env vars OTEL_EXPORTER_OTLP_*_PROTOCOL defined by the specs. These envs will take effect only when OTEL_*_EXPORTER is set to otlp.

  2. In case OTEL_*_EXPORTER will be set to otlp_proto_grpc/otlp_proto_http and a conflicting value will be set to OTEL_EXPORTER_OTLP_*_PROTOCOL, the protocol will be set according to OTEL_*_EXPORTER and a warning will be printed.

  3. Updating the default value for OTEL_*_EXPORTER to otlp in opentelemetry-instrumentation package and in the SDK (also according to specs), as an actual value and not just an alias. The exporter used in this case will still be gRPC, to avoid a breaking change. This way, OTEL_EXPORTER_OTLP_*_PROTOCOL could be configured without specifically setting OTEL_*_EXPORTER=otlp.


Future breaking changes I propose is to remove the options otlp_proto_grpc/otlp_proto_http, since they are not defined in the specs, and they introduce this redundant complexity. Also, I didn't find that these are used in other runtimes.

Note - it does not require changing package entry points (such that), to avoid this unnecessary breaking change.

@srikanthccv
Copy link
Member

Sounds good to me. It looks like the env are already defined here https://github.com/open-telemetry/opentelemetry-python/blob/edb1391a3d29d844141b15e486cd1107e163ee0b/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py. If this requires only changes in contrib repo, I will transfer this issue there.

@ronyis
Copy link
Contributor

ronyis commented Aug 24, 2022

It requires changes in both repos, so I think it can remain here.

@srikanthccv
Copy link
Member

Ah correct, the configuration part was also originally part of opentelemetry-distro package but later it was moved to SDK. Since it requires changes in both, You will have to update SHA in test.yml when you open PRs for tests to pass; more details here https://github.com/open-telemetry/opentelemetry-python/blob/main/CONTRIBUTING.md#pull-requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment