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.

Sign up for GitHub

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

Jump to bottom

introduce flag for insecure communication #14

Merged
merged 1 commit into from
Sep 23, 2022
Merged

introduce flag for insecure communication #14

merged 1 commit into from
Sep 23, 2022

Conversation

vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Sep 16, 2022 •
edited

closes #13

this is particularly useful in development environments so that folks don't have to setup TLS in their collector

jzelinskie
jzelinskie reviewed Sep 16, 2022
View reviewed changes
otel.go Outdated
@@ -38,6 +38,7 @@ func RegisterOpenTelemetryFlags(flags *pflag.FlagSet, flagPrefix, serviceName st
flags.String(prefixed("endpoint"), "", "OpenTelemetry collector endpoint - the endpoint can also be set by using enviroment variables")
flags.String(prefixed("service-name"), serviceName, "service name for trace data")
flags.String(prefixed("trace-propagator"), "w3c", `OpenTelemetry trace propagation format ("b3", "w3c", "ottrace"). Add multiple propagators separated by comma.`)
flags.String(prefixed("insecure"), "false", `OpenTelemetry communicate with the collector with HTTP instead of HTTPS.`)
Copy link
Owner

Choose a reason for hiding this comment

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

Trying to think of a better flag description

"connect to the OpenTelemetry collector in plaintext"

@jzelinskie
Copy link
Owner

Let's double check there isn't a way to do this with environment variables first.

@vroldanbet
Copy link
Contributor Author

Closing - you can achieve this by using OTELs OTEL_EXPORTER_OTLP_ENDPOINT env var like http://localhost:4318. Otel library calls WithInsecure if http is set.

https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go#L97-L104

@vroldanbet vroldanbet closed this Sep 16, 2022
@vroldanbet vroldanbet deleted the support-insecure-collectors branch September 16, 2022 16:03
@vroldanbet vroldanbet mentioned this pull request Sep 21, 2022
Closed
@vroldanbet vroldanbet restored the support-insecure-collectors branch September 21, 2022 10:54
@vroldanbet vroldanbet reopened this Sep 21, 2022
@vroldanbet
Copy link
Contributor Author

@jzelinskie I've reopened this. Even though it's possible to enable insecure communication via OTEL environment variables, it's not immediately obvious to users of applications using cobrautil. For example SpiceDB offers --help flag with all options, including those related to OTEL provided by cobrautil. It would be a better UX - we cannot expect folks to go and check the opentelemetry go library source code to figure out they can achieve it with OTEL_EXPORTER_OTLP_ENDPOINT.

Until it's decided to deprecate cobrautil/otel flags in favour of OTEL's native envs, we should provide a cohesive interface in cobrautil.

@vroldanbet vroldanbet force-pushed the support-insecure-collectors branch 2 times, most recently from 8e4f573 to e2da000 Compare September 21, 2022 11:28
@jzelinskie
Copy link
Owner

I think that's reasonable, is there an equivalent for Jaeger though? Because the cobrautil flag is not tied to a particular provider, ideally it can also trigger the behavior for Jaeger.

@vroldanbet
Copy link
Contributor Author

vroldanbet commented Sep 22, 2022 •
edited

@jzelinskie

I think that's reasonable, is there an equivalent for Jaeger though? Because the cobrautil flag is not tied to a particular provider, ideally it can also trigger the behavior for Jaeger.

in Jaeger the endpoint must have the protocol as prefix, so it's defined through that. Nothing in the API showed the equivalent, but I can dig a bit more

EDIT: I couldn't wind any equivalent in the otel Jaeger exporter. It would seem to be delegated to the http.Client, which figures out based on the endpoint URL

@vroldanbet vroldanbet force-pushed the support-insecure-collectors branch 4 times, most recently from 3d14db9 to e26feef Compare September 22, 2022 19:11
jzelinskie
jzelinskie reviewed Sep 23, 2022
View reviewed changes
otel.go Outdated Show resolved Hide resolved
@vroldanbet
introduce flag for insecure communication
972f47d 
this is particularly useful in development environments
so that folks don't have to setup TLS in their collector
@jzelinskie jzelinskie merged commit 8f74475 into jzelinskie:main Sep 23, 2022
@vroldanbet vroldanbet deleted the support-insecure-collectors branch September 23, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzelinskie jzelinskie jzelinskie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

It doesn't seem to be possible to configure WithInsecure for an opentelemetry collector endpoint
2 participants
@vroldanbet @jzelinskie