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

Deprecate otlp_proto_grpc and otlp_proto_http in auto-instrumentation #1250

Merged
merged 16 commits into from
Oct 27, 2022

Conversation

ronyis
Copy link
Contributor

@ronyis ronyis commented Aug 29, 2022

Description

Fixes open-telemetry/opentelemetry-python#2586

This PR depends on SDK-2893 , to support setting OTLP transport protocol with env vars, instead of using otlp_proto_grpc / otlp_proto_http.

This method should be preferred, as it is defined in the specifications (it is also how it works in other OTel libs).

This PR fixes open-telemetry/opentelemetry-python#2586, by allowing the user to configure the protocol using OTEL_EXPORTER_OTLP_PROTOCOL without making a conflict with the current default otlp_proto_grpc setting. It also updates documentation on how to set the protocol.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • opentelemetry-distro/tests/test_distro.py

Does This PR Require a Core Repo Change?

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated (in core package only)
  • Unit tests have been added
  • Documentation has been updated

@@ -22,7 +22,7 @@ This package provides a couple of commands that help automatically instruments a
For more info about ``opentelemetry-distro`` check `here <https://opentelemetry-python.readthedocs.io/en/latest/examples/distro/README.html>`__
::

pip install opentelemetry-distro[otlp]
pip install opentelemetry-distro
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's different. The auto instrumentation requires it this way otherwise without any exporter users won't see any telemetry.

Comment on lines +78 to +79
- otlp_proto_grpc (`deprecated`)
- otlp_proto_http (`deprecated`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should not recommend using this, as the defined way in OTel to configure the protocol is with the other env vars, and to avoid conflicts.

In future versions, we may also remove these options (though it's a breaking change).


``otlp`` is an alias for ``otlp_proto_grpc``.
Note: The default transport protocol for ``otlp`` is gRPC.
HTTP is currently supported for traces only, and should be set using ``OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=http/protobuf``
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest not using the more general way of OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf, since it's still not supported for metrics & logs.

@ronyis ronyis marked this pull request as ready for review August 29, 2022 18:27
@ronyis ronyis requested a review from a team August 29, 2022 18:27
@ronyis
Copy link
Contributor Author

ronyis commented Sep 7, 2022

open-telemetry/opentelemetry-python#2893 is already merged, this PR awaits review

@@ -22,7 +22,7 @@ This package provides a couple of commands that help automatically instruments a
For more info about ``opentelemetry-distro`` check `here <https://opentelemetry-python.readthedocs.io/en/latest/examples/distro/README.html>`__
::

pip install opentelemetry-distro[otlp]
pip install opentelemetry-distro
Copy link
Member

Choose a reason for hiding this comment

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

That's different. The auto instrumentation requires it this way otherwise without any exporter users won't see any telemetry.

Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
@ronyis
Copy link
Contributor Author

ronyis commented Sep 12, 2022

@srikanthccv can you please add a skip-changelog label?
(I think the changelog added in the SDK is sufficient since the logic is there)

@srikanthccv
Copy link
Member

Please add the CHANGELOG entry. It's needed here also for visibility for the end users who use the instrumentation packages. The SDK PR makes it complaint with spec but this one says they are deprecated.

@ronyis
Copy link
Contributor Author

ronyis commented Sep 20, 2022

Is there anything missing to merge this PR? (@srikanthccv)

@srikanthccv
Copy link
Member

Your branch is out of date with the main branch. You didn't enable maintainers to push commits to your branch so we can't update it. This won't be merged until it is updated with the main changes at the time of merge.

@srikanthccv
Copy link
Member

@ronyis update/rebase the branch if you would like to get this merged.

CHANGELOG.md Outdated
@@ -40,6 +40,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `opentelemetry-distro` Deprecate `otlp_proto_grpc` and `otlp_proto_http` in favor of using
`OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` as according to specifications
([#1250](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1250))
Copy link
Member

Choose a reason for hiding this comment

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

Move this to Unreleased section

Copy link
Member

Choose a reason for hiding this comment

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

Fix this

@ocelotl
Copy link
Contributor

ocelotl commented Oct 12, 2022

@srikanthccv please remove your approval if there are still pending changes from the submitter to prevent accidental merging.

CHANGELOG.md Outdated
@@ -40,6 +40,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `opentelemetry-distro` Deprecate `otlp_proto_grpc` and `otlp_proto_http` in favor of using
`OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` as according to specifications
([#1250](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1250))
Copy link
Member

Choose a reason for hiding this comment

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

Fix this

@ronyis
Copy link
Contributor Author

ronyis commented Oct 18, 2022

@srikanthccv Should be fixed now (sorry for the delay)

@srikanthccv
Copy link
Member

Please update with the main

@srikanthccv srikanthccv merged commit 83f37ed into open-telemetry:main Oct 27, 2022
@ronyis ronyis deleted the otlp-protocol-env-vars branch October 27, 2022 12:42
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
@MasaNahti
Copy link

This deprecation has not been documented in Python automatic instumentation configuration docs. It took me awhile to figure it out, since I assumed the docs as the best source of truth. There are also plenty other mistakes there, but that's another topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting any value to OTEL_EXPORTER_OTLP_PROTOCOL has no effect.
6 participants