Skip to content

Conversation

tonistiigi
Copy link
Contributor

These fixes are needed to make otel-cli work with BuildKit after moby/buildkit#2572

The first commit adds support for OTEL_EXPORTER_OTLP_TRACES_ variables for setting the trace endpoint. These are defined in https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/protocol/exporter.md . These variables are defined more specifically so take priority over the generic vars if set.

The second commit fixes unix URLs.

Unix URLs should not need any special handling but the current implementation handles values without the
schema that is not allowed by the spec.

From https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/protocol/exporter.md

The endpoint MUST be a valid URL with scheme (http or https) and host
A scheme of https indicates a secure connection.

If this can be removed in the future then all these exceptions can be removed as well and WithInsecure
is automatically detected by the endpoint value in otlptracegrpc package. https://github.com/open-telemetry/opentelemetry-go/blob/v1.3.0/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go#L166-L167

Currently, it fails in your custom endpoint parsing regexps that don't expect such values. Although otlptracegrpc correctly marks it insecure by default it is overwritten by otel-cli because it always sets TLS credentials if it can't match endpoint to a loopback name. The correct fix in here would be just to only set TLS credentials if the schema is https like the spec defines. Everything else is handled automatically by otlptracegrpc. But I didn't want to break any backward compatibility here so leave that for future development/discussion.

Defined in https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/protocol/exporter.md

These variables are defined more specifically
so take priority over the generic vars if set.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Unix URLs should not need any special handling but
current implementation handles values without the
schema that is not allowed by the spec.

If this can be removed in the future then all these
exceptions can be removed as well and WithInsecure
is automatically detected by the endpoint value
in otlptracegrpc package.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tobert
Copy link
Collaborator

tobert commented Jan 24, 2022

Looks good, I should have some time to review / merge / release in the next couple days.

@tonistiigi
Copy link
Contributor Author

Ping

@tobert
Copy link
Collaborator

tobert commented Feb 23, 2022

Thanks for the ping! I'm sorry I forgot. I looked this over and will merge presently, and am overdue for a release so I'll do that too.

Copy link
Collaborator

@tobert tobert left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you :)

@tobert tobert merged commit 8488238 into equinix-labs:main Feb 23, 2022
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.

2 participants