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

The default endpoint should use HTTP, not HTTPS #4834

Closed
edigaryev opened this issue Jan 19, 2024 · 10 comments
Closed

The default endpoint should use HTTP, not HTTPS #4834

edigaryev opened this issue Jan 19, 2024 · 10 comments
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@edigaryev
Copy link

edigaryev commented Jan 19, 2024

Here's an excerpt from the OpenTelemetry's mission statement:

OpenTelemetry strives to be standards-compliant, vendor-neutral, and consistent across languages and components.

Now take a look at the following table:

Language Uses http://localhost:PORT endpoint by default
C++ Yes
.NET Yes
Erlang/Elixir Yes
Go No
Java Yes
JavaScript Yes
PHP Yes
Python Yes
Ruby Yes
Rust Yes
Swift Yes

I've searched for the issues a bit and found #4147 (comment), which argues against the change, saying that:

One significant concern is that changing the default would break most code that is using secure HTTPS endpoints. Presently, the otlptracehttp lacks a WithSecure method, offering only an option for insecure connections.

However, I believe it is based on a false premise that we need treat all OTEL_EXPORTER_OTLP_ENDPOINT's as HTTP by default.

What we need instead is to simply change the default OTEL_EXPORTER_OTLP_ENDPOINT value from https://localhost:PORT to http://localhost:PORT.

Moreover, I'd strongly argue that most people won't notice this change at all, because almost everyone seems to run the OpenTelemetry Collector with the TLS disabled.

Cc: @pellared

@edigaryev edigaryev added the bug Something isn't working label Jan 19, 2024
@pellared
Copy link
Member

pellared commented Jan 19, 2024

Most people is not everyone. For some it would be a breaking change. This issue was already discussed at least 2 times during SIG meetings and nobody was supporting such change. There is a more detailed explanation in the issue that you mentioned.

@pellared pellared added the invalid This doesn't seem right label Jan 19, 2024
@edigaryev
Copy link
Author

edigaryev commented Jan 19, 2024

Most people is not everyone. For some it would be a breaking change for them.

Is there a policy for making breaking changes?

It seems that when a single component in a whole ecosystem deviates from the rest of the ecosystem, by making it conform we will benefit not just this single component, but the whole ecosystem and its users in the long run, even if it involves making a breaking change.

There is a more detailed explanation in the issue that you mentioned.

I've read it all, but I believe it is based on a false premise, which I've explained in #4834 (comment).

Could you please clarify this?

@edigaryev
Copy link
Author

Thanks!

So according to VERSIONING.md, it seems that the change proposed here falls under the following statement:

In addition to public APIs, telemetry produced by stable instrumentation will remain stable and backwards compatible. This is to avoid breaking alerts and dashboard.

Could this change be scheduled for v2 (perhaps added to a milestone or a roadmap)?

@pellared
Copy link
Member

SDK stability, as defined above, will be maintained for a minimum of one year after the release of the next major SDK version.

Right now, I do not see any v2 label/milestone and we have more TODO than we are able to handle 😢 I am not sure if we want (or be able) to ever release a v2. I will discuss it during SIG meeting.

@pellared
Copy link
Member

pellared commented Feb 9, 2024

Closing as, according to my knowledge, the SIG is against creating a v2 until necessary. This proposal is not strong enough to create a v2. At last, I am not sure if we would even agree that this change is good as we preferred to be "secure by default" (like e.g. google.golang.org/grpc) .

@pellared pellared closed this as not planned Won't fix, can't repro, duplicate, stale Feb 9, 2024
@tmc
Copy link

tmc commented Sep 1, 2024

This seems to put the go sdk out of compliance with the spec.

If this isn't reverted I encourage documentation at https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/otlptrace/otlptracehttp/doc.go#L12 to be updated to point out that the default value for the go SDK is non-standard.

@dmathieu
Copy link
Member

dmathieu commented Sep 2, 2024

The default value is already documented on your link.

@tmc
Copy link

tmc commented Oct 25, 2024

The default value is already documented on your link.

Do you acknowledge that this diverges from the spec and that is harms developer experience?

@dmathieu
Copy link
Member

dmathieu commented Oct 25, 2024

This does diverge from the spec.
However, changing it on our end would be a breaking change, and I don't think the developer experience inconvenience here is worth doing a major release.

It would also, again be argued that configuration should be secure by default (and therefore default to HTTPS). So if we start looking at a major release, I'd favor changing the spec rather than this implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

4 participants