Description

This updates the OTLP gRPC exporter behavior to better conform to the spec.

• Enable HTTPS by default, i.e. nothing is set for insecure. (Not in the linked issue but in the spec: insecure:default=False.)
• Enable HTTPS if the endpoint scheme is HTTPS even if insecure=True.
• Allow setting the insecure flag by environment variable.

Fixes #1981

Type of change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Added unit tests to test_otlp_trace_exporter.py that ensure the correct endpoint and correct secure/insecure channel is being used. New cases ensure the default is set and the https scheme supersedes the insecure option and test the new environment variable.

Does This PR Require a Contrib Repo Change?

• No.

Checklist:

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

• ✅ Arianna Y (b6f07af, 274115d)

I am having a little trouble with running some tests with tox:

You seem to have unrelated test failures and unit tests do not connect with actual collector instance. I will take a look at it.

Choose a reason for hiding this comment

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

It appears we'll also need to add OTEL_EXPORTER_OTLP_SPAN_INSECURE and OTEL_EXPORTER_OTLP_METRIC_INSECURE to be compliant with the spec https://github.com/open-telemetry/opentelemetry-specification/pull/2240/files

Do we need to support both even if didn't have support for env prior? My understanding is SIGs that already implemented OTEL_EXPORTER_OTLP_SPAN_INSECURE should continue to support both.

@codeboten @lonewolf3739

Do we need to support both even if didn't have support for env prior? My understanding is SIGs that already implemented OTEL_EXPORTER_OTLP_SPAN_INSECURE should continue to support both.

I believe since we did not support OTEL_EXPORTER_OTLP_SPAN_INSECURE before, we do not need to support it now. Simply adding OTEL_EXPORTER_OTLP_TRACES_INSECURE would be sufficient imo assuming the spec PR gets merged.

Thanks for the reviews folks. I believe all the feedback so far has been incorporated.

Modified HTTPS cases for tests for the recently merged metrics exporter #2323.

@areveny
Can you update your contrib repo SHA to point to 23394ccd80878a91534f8421b82a7410eb775e65

@areveny
Any reason to not add OTEL_EXPORTER_OTLP_METRICS_INSECURE functionality as well for the recently merged otlp metrics exporter?

@areveny You pushed changes just before I edited the Leighton's comment. It should OTEL_EXPORTER_OTLP_METRICS_INSECURE not OTEL_EXPORTER_OTLP_METRIC_INSECURE

Choose a reason for hiding this comment

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

@lonewolf3739
Could you create an issue to add support for this for logging otlp exporter?

Thanks folks, really appreciate all the guidance.

I'll keep an eye out for this on the logging otlp exporter and take that up when it's ready.

We missed something here. Since we introduced a change in SDK that the exporter depends on, the exporter should have bumped it's minimum required SDK version to the in development version. See #2438 for details.