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

Adding support for setting OTLP exporter protocol by env vars #2893

Merged
merged 10 commits into from
Sep 6, 2022

Conversation

ronyis
Copy link
Contributor

@ronyis ronyis commented Aug 28, 2022

Description

Fixes #2586

Adding support for specifying OTLP export protocol using env vars, as defined in the specifications.

Since the SDK already supports setting the protocol using the values otlp_proto_grpc and otlp_proto_http of OTEL_TRACES_EXPORTER, these values are still supported and have priority over the new supported variables. In case of conflict, a warning will be printed.

Future support for configuring metrics and logs with HTTP exporter is already added, although those do not exist yet, therefore will result in a RuntimeError.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • opentelemetry-sdk/tests/test_configurator.py::TestExporterNames

Does This PR Require a Contrib Repo Change?

See open-telemetry/opentelemetry-python-contrib#1250
Contrib repo change is not necessarily required, but it is related to fixing the same issue.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ronyis ronyis requested a review from a team as a code owner August 28, 2022 13:40
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 28, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ronyis / name: Ron Yishai (0ef86ee)

@ronyis ronyis marked this pull request as draft August 28, 2022 13:41
@srikanthccv srikanthccv added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Aug 29, 2022
@ronyis ronyis marked this pull request as ready for review August 29, 2022 18:27
@ronyis ronyis changed the title [WIP] Adding support for setting OTLP exporter protocol by env vars Adding support for setting OTLP exporter protocol by env vars Aug 29, 2022
Copy link
Contributor

@galbash galbash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the PR you mentioned:

Since the SDK already supports setting the protocol using the values otlp_proto_grpc and otlp_proto_http of OTEL_TRACES_EXPORTER, these values are still supported and have priority over the new supported variables. In case of conflict, a warning will be printed.

Is our goal to deprecate otlp_proto_grpc/http over time or discourage the use of them? if so maybe we should issue a deprecation warning / log

@ronyis
Copy link
Contributor Author

ronyis commented Sep 4, 2022

in the PR you mentioned:

Since the SDK already supports setting the protocol using the values otlp_proto_grpc and otlp_proto_http of OTEL_TRACES_EXPORTER, these values are still supported and have priority over the new supported variables. In case of conflict, a warning will be printed.

Is our goal to deprecate otlp_proto_grpc/http over time or discourage the use of them? if so maybe we should issue a deprecation warning / log

@srikanthccv what do you think about that?

@srikanthccv
Copy link
Member

Ideally we should deprecate them. It may be our shortsightedness because the way this going - for someone who is Installing just the *-proto-http package why do they need need to configure *_PROTOCOL again to get it to work correctly.

@ronyis
Copy link
Contributor Author

ronyis commented Sep 6, 2022

I think it's OK that the user will have to explicitly specify the OTLP protocol in case he doesn't use the default value.
We can also add the functionality of trying to import the HTTP exporter, in case the gRPC one isn't found. Personally, I think that it should better be defined in the specifications first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting any value to OTEL_EXPORTER_OTLP_PROTOCOL has no effect.
5 participants