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

Simplify endpoint #103

Open
marcalff opened this issue Jul 14, 2024 · 5 comments
Open

Simplify endpoint #103

marcalff opened this issue Jul 14, 2024 · 5 comments

Comments

@marcalff
Copy link
Member

The existing environment variables used in the spec to describe an endpoint:

  • OTEL_EXPORTER_OTLP_PROTOCOL
  • OTEL_EXPORTER_OTLP_ENDPOINT
  • OTEL_EXPORTER_OTLP_INSECURE

are the result of incremental design, due to history.

The current design for config.yaml derives from this existing specification, and currently is:

otlp:
  # http/protobuf, http/json, or grpc
  protocol:

  # http endpoint, https endpoint, or no scheme
  endpoint:

  # true or false, only used for grpc protocol with no scheme endpoint
  insecure: 

Exposing the same fields to match the existing environment variables has some merits, but also carries over existing issues.

Existing configuration

  1. Protocol

The protocol to use should be in the endpoint URL.
Instead, when using grpc, the "protocol" is set to "grpc", while the endpoint may contain "http" or "https".
This makes no sense.

  1. Message format

The message format, "protobuf" or "json", is mixed with the protocol.
I don't think it belongs there.

  1. Security

The "insecure" field is a last resort configuration parameter for grpc when not used with http/https.
This is too convoluted.

  1. Lack or orthogonality

With the existing configuration, if someone changes the protocol between http and grpc, then the following fields must be adjusted as well:

  • endpoint needs to be fixed for the port number
  • endpoint needs to be fixed to add or remove the /v1/{signal} path
  • the endpoint http/https scheme might be missing when coming from grpc, and needs to be fixed
  • the insecure field may be used or not, adding general confusion. insecure=false may still be insecure.

Proposed configuration

otlp:
  # Valid values are "protobuf" or "json".
  content_type: protobuf

  # Full URL with:
  # - a scheme ("http", "https", "grpc", "grpcs")
  # - a host name
  # - a port number
  # - an optional path, for http
  endpoint:

Examples of configuration:

otlp:
  content_type: protobuf
  endpoint: "http://localhost:4318/v1/traces"
otlp:
  content_type: json
  endpoint: "https://localhost:4318/v1/traces"
otlp:
  content_type: protobuf
  endpoint: "grpc://localhost:4317"
otlp:
  content_type: protobuf
  endpoint: "grpcs://localhost:4317"

Note how this simplifies things, as the endpoint URL specifies:

  • the protocol to use, HTTP versus gRPC
  • if TLS is used, with http / grpc versus https / grpcs

all this in the same field.

It then becomes much easier to change from:

  endpoint: "grpc://localhost:4317"

to

  endpoint: "https://localhost:4318/v1/traces"

when editing a config file, or when providing substitution variables.

General considerations

Note that I am NOT asking to change existing environment variables in the specs, as this will have some global impact on all SIG and all existing deployments.

This ship has sailed.

What I am asking to consider, is to have a different design for config.yaml only, which I think makes more sense and will be more practical to work with in the long term.

Given that there are no existing deployments using config.yaml, there is no impact if done at this stage.

The migration to config.yaml will not be any more complex, mapping to the new content_type and endpoint fields is trivial.

I think we should take this opportunity to simplify.

@marcalff
Copy link
Member Author

Filed separately from comments on #76, so this can be discussed without affecting the merge of that PR.

@jack-berg
Copy link
Member

Couple of thoughts:

  • This would appear to add to the list of env vars which are unmapped defined in Add sdk-config.yaml starter template w/ references to env vars #76, as OTEL_EXPORTER_OTLP_{SIGNAL}_PROTOCOL would join the others. This might not be a deal breaker, but should be considered.
  • This adds interpretation requirements (something discussed in this convo), since code has to detect the grpc / grpcs protocols and strip the protocol and configure the corresponding gRPC exporter according to the host and TLS from the endpoint.
  • Not sure, but are grpc / grpcs official values for scheme?

@marcalff
Copy link
Member Author

marcalff commented Jul 18, 2024

Couple of thoughts:

These two:

  • OTEL_EXPORTER_OTLP_{SIGNAL}_PROTOCOL
  • OTEL_EXPORTER_OTLP_{SIGNAL}_INSECURE

will no longer be used.

  • This adds interpretation requirements (something discussed in this convo), since code has to detect the grpc / grpcs protocols and strip the protocol and configure the corresponding gRPC exporter according to the host and TLS from the endpoint.

Nothing changed from today. When the protocol is grpc, the code has to parse a hostname and port number from the endpoint http://host:port, while taking the http or https scheme, if present, as an indication for INSECURE.

  • Not sure, but are grpc / grpcs official values for scheme?

I have seen some references to both in the wild, for example:

postmanlabs/postman-app-support#11489

However, grpc is not even in the iana registry:

https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

The point is, whatever bit dialect an exporter sends on a socket on the wire, there should be a scheme to name that dialect, and the scheme with / without TLS should have a different name, since the bits are really different.

@tsloughter
Copy link
Member

I don't think grpc:// should be used.

@jack-berg
Copy link
Member

Discussed in the 8/19/24 SIG.

I think we should take this opportunity to simplify.

This takes further away from compatibility with the environment variable scheme. We could solve this by having two sets of properties: one for interop with existing environment variable scheme, a second for improved use experience. But this adds confusion and the question is whether the current scheme had bad enough ergonomics to justify the additional cognitive load.

Didn't reach an answer, but we need to address this with a yes or no before stability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants