Update endpoint for OTLP/HTTP to allow scheme

[mwear]

Changes

For OTLP/gPRC it's convenient to represent a secure connection using a secure/insecure bit, but for OTLP/HTTP it's more natural to provide this information as the scheme portion of a URL. This PR updates the OTLP exporter spec to allow for that possibility. It changes the Endpoint so that it can include a scheme for OTLP/HTTP and indicates that the insecure bit only applies to gRPC.

[arminru]

Please add an entry in the changelog.

[arminru]

Should we make this backwards-compatible and assume https if not provided?

[mwear]

I have a slight preference to require the scheme for OTLP/HTTP as it both simplifies the spec and resulting implementation, but I am open to the possibility if everyone thinks it's worth the tradeoff.

[carlosalberto]

I'd be fine with the suggestion @arminru proposed, but my feeling is that this part hasn't been truly adopted, so we can still change it without any problem.

[SergeyKanzhelev]

CC: @open-telemetry/collector-approvers

[carlosalberto]

@tigrannajaryan Maybe you are the right person to poke here?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[tigrannajaryan]

I am not quite sure I like the proposed change.

1. This creates non-uniformity in how we specify grpc or http transports endpoints and how we choose secure vs insecure connections. This is probably OK, but ideally we would have a consistent way regardless of the transport (e.g. ideally grpc would have a standard way to specify the the scheme in the URI, such as grpc://<server name>... and we would use that). I don't know if there is such an ideal possible though.

2. This creates the possibility of specifying conflicting config options, e.g. what happens if we do this:

export OTEL_EXPORTER_OTLP_METRICS_PROTOCOL=grpc
export OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=https://collector.example.com/v1/metrics

Do we ignore grpc? Do we give an error and fail?

I am not strongly opposed to this change, but at the minimum would like clarifications on point 2.

[mwear]

I think there are existing problems with the configuration from your example (pasted below), and I don't think this change makes them any worse (or better).

export OTEL_EXPORTER_OTLP_METRICS_PROTOCOL=grpc
export OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=https://collector.example.com/v1/metrics

• Some languages do not have a single collector exporter, and instead, have a separate package per protocol. This is the case for Javascript where we have three packages, one that supports GRPC, one that supports HTTP/proto and another that supports to HTTP/JSON. The reasoning for the separation is to minimize the number of dependencies that need to be pulled in. So, not all exporters will support the protocol environment variable, as the protocol is determined by the package that is used.
• In packages where it is supported, if the scheme was omitted and the secure bit was used, the configuration would still be invalid for grpc, as it only allows a host and port for the endpoint. See below:

export OTEL_EXPORTER_OTLP_METRICS_PROTOCOL=grpc
export OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=collector.example.com/v1/metrics
export OTEL_EXPORTER_OTLP_METRICS_INSECURE=false

[tigrannajaryan]

@mwear OK, I understand what you say. Just to be clear, I am not sure this PR is an improvement over the current state, but I don't have an alternate suggestion and I don't mind this change if other approvers can be convinced.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[carlosalberto]

Ping @mwear

[mwear]

It's natural to think of an endpoint as being an complete URL when configuring an HTTP based exporter. This change would allow a user to specify an endpoint as:

export OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=https://collector.example.com:443/v1/metrics

rather than:

export OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=collector.example.com/v1/metrics
export OTEL_EXPORTER_OTLP_METRICS_INSECURE=false
export OTEL_EXPORTER_OTLP_METRICS_PORT=443

[SergeyKanzhelev]

This was not adopted by any language: https://github.com/search?q=org%3Aopen-telemetry+OTEL_EXPORTER_OTLP_METRICS_ENDPOINT&type=code

Is the main idea for this change is to allow http (over https)? Or just make sure that human error of typing the full URL doesn't break the configuration? If second - I'd suggest to call it out and allow both cases.

Styling-wise, after this change it's not easy to read examples column when both protocols are in the same table. Perhaps this can be split into two separate tables for readability? (original text didn't assume differences in how address may and must be represented).

[jmacd]

I strongly support the outcome where the user has to only write one URL as the endpoint, regardless of which protocol is in use, and that the URL should include the host, port, and path. I believe http:// should signify insecure, and https:// should signify secure connections, and I do not believe it matters to have a choice of which protocol actually gets used (e.g., OTLP/gRPC, OTLP/HTTP in json or protobuf).

My rationale for these two statements:

1. Services should protect themselves. They should not allow plaintext traffic when clients use an insecure network. This is easy to do in the OTel collector or in a proxy.
2. Clients are unlikely to be built with multiple protocol support. At the moment when we need to build a concrete exporter and open a real connection, there is unlikely to be any question which protocol should be used. It's a compile-time choice, not a runtime choice.

In that sense, I don't think this does enough. Let's get rid of OTLP_EXPORTER_INSECURE, OTLP_EXPORTER_SPAN_PROTOCOL, and so on.

[carlosalberto]

I strongly support the outcome where the user has to only write one URL as the endpoint

+1. Even though part of us want to 'decouple' the components, in my experience this has been the very best solution, user-wise. Adding more components easily increases checks/complexity and chances of errors, sadly.

[anuraaga]

+1 to require a schemed URL for gRPC endpoints. I've used the pattern for internal RPC too before and have never found it too weird or anything. Simplifications of the flags would be a great win.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[mwear]

@anuraaga I would like to propose schemed URLs for gRPC endpoints. Do you have examples for how they should look? I found this document: https://github.com/grpc/grpc/blob/master/doc/naming.md, but it wasn't entirely clear to me what the right approach should be.

[anuraaga]

@mwear I was thinking just normal HTTP schemes. At least in Java, it's the exporter that models whether something is gRPC or not, for example we have jaeger (gRPC) and jaeger_thrift (Thrift). So it could just be http://localhost:1234 for plaintext and https://localhost:1234 for TLS, even for gRPC, I don't think we need anything like grpc://. I believe this is what @jmacd is proposing in #1234 (comment) too.

[SergeyKanzhelev]

I support the single variable approach. Much easier to use. If we want fancy, GRPC implementation can ignore the protocol so both http:\\ and grpc:\\ may be supported.

[mwear]

Based on the feedback, I've updated this to specify schemed endpoints for OTLP/gRPC and OTLP/HTTP. Since the scheme indicates whether or not a connection is secure, I was able to remove the OTEL_EXPORTER_OTLP*_INSECURE options.

I can also remove the OTEL_EXPORTER_OTLP_PROTOCOL options as suggested by @jmacd, but I wanted to get others'
opinions on this. For the languages that I am familiar with, the exporters for each protocol are implemented in their own packages. So, the package determines the protocol, and a result, this environment variable doesn't make sense. If this is true across the board, we should remove it. If it is being used somewhere, I'd be fine keeping it as documentation.

[carlosalberto]

I can also remove the OTEL_EXPORTER_OTLP_PROTOCOL options as suggested by @jmacd, but I wanted to get others'
opinions on this.

Consider opening an issue for this (specially if it drags off the current PR).

[carlosalberto]

Ping @tigrannajaryan

[SergeyKanzhelev]

variables weren't adopted yet so it's ok to change. Will merge tomorrow morning if nobody will object.