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 default for OTLP endpoint should, not must, be https #1997

Merged
merged 3 commits into from
Oct 8, 2021

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Oct 6, 2021

Fixes #1984

Changes

Adds a subnote clarifying that while the OTLP endpoint variables should default to https, it is not a must

I mostly copied the wording from #1969

@anuraaga anuraaga requested review from a team October 6, 2021 08:56
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, to avoid breaking a stable SDK release.

@Oberon00 Oberon00 added the spec:protocol Related to the specification/protocol directory label Oct 6, 2021
@tigrannajaryan
Copy link
Member

@open-telemetry/specs-approvers please review.

@jack-berg
Copy link
Member

Is it actually a good thing to have the default scheme be https? IMO the default configuration should work with a local collector running with minimal configuration. An app configured to export over https to a collector configured with a minimalist otlp receiver doesn't work.

To get this working, you'd have to configure TLS for the otlp receiver, issuing and specifying the certificate, and passing the same certificate to app via OTEL_EXPORTER_OTLP_CERTIFICATE. That's a lot to get right. The ergonomics are better if SDKs can talk to the collector without any configuration.

FYI, here's the original PR where the default scheme was set to https. The convo is primarily about inferring TLS from the scheme. Don't see any convo discussing whether https is actually a reasonable default, but maybe that's buried somewhere else.

@jack-berg
Copy link
Member

After more digging, I found this PR that seems to have been the seed for the original https default. It added the original OTEL_EXPORTER_OTLP_*_INSECURE flags, which defaulted to false, and seem to have inspired the https default.

I can't find any context in that PR related to why to default to secure.

@carlosalberto
Copy link
Contributor

@jack-berg Also we don't want to break this invariant for users, as mentioned @anuraaga :

I think fixing this now would be too disruptive to existing users without enough benefit.

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Responding to @jack-berg's comments, I think it's a valid concern. However, I think instead we should try to define some kind of "local-lookup" mechanism in a future version of the specification where an SDK can find a local collector and communicate over a locally-optimal protocol. If you're interested in defining/refining that mechanism, would be happy to brainstorm ideas with you.

@jsuereth
Copy link
Contributor

jsuereth commented Oct 7, 2021

This has enough approvals. Going to leave open for 1 more business day for feedback before enabling auto-merge.

@jsuereth jsuereth enabled auto-merge (squash) October 8, 2021 15:35
@jsuereth jsuereth merged commit 442e088 into open-telemetry:main Oct 8, 2021
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
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
Development

Successfully merging this pull request may close these issues.

Document discrepancy with spec or loosen spec for default value of exporter endpoint
6 participants