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

fix: set withinsecure() by default in otlp_http_example #4682

Closed

Conversation

rfyiamcool
Copy link

summary

I think the default configuration Insecure would be better.

When I tested it yesterday, I found that it always prompted https issues. I found out through the code that https is enabled by default. Usually the collector does not enable https security.

When people learn how to use otel through examples, it is easy to misunderstand.

Of course, this is just personal advice. 😁

otel  http: server gave HTTP response to HTTPS client
otel  http: server gave HTTP response to HTTPS client
otel  http: server gave HTTP response to HTTPS client

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 31, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rfyiamcool / name: fengyun.rui (06ac004)

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. However, we cannot accept this change as it would be breaking. See more: #4147

@arukiidou
Copy link
Contributor

arukiidou commented Nov 1, 2023

Thank you for your contribution. However, we cannot accept this change as it would be breaking. See more: #4147

@pellared

  • I have a different opinion. it will not broke something.
    • example_test will never be the default behavior
    • example_test will not a production environment

@dmathieu
Copy link
Member

dmathieu commented Nov 1, 2023

Even outside the breaking change, I don't know if setting WithInsecure here is so good.
It's something someone should only be using for dev/testing environments, never in production. And this example here is a runnable example, but not a "getting started" one where we show off how things work using a dummy environment (the example [here])(https://github.com/open-telemetry/opentelemetry-go/tree/main/example/otel-collector) is that).

So I agree with @pellared that this change needn't be merged, not necessarily because of the breaking change but rather because the example doesn't need it per-se.

@pellared
Copy link
Member

pellared commented Nov 2, 2023

Sorry, I have not noticed that this is just an example 🤦

I will look at making better documentation as part of #4688.

@pellared pellared closed this Nov 6, 2023
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.

4 participants