-
Notifications
You must be signed in to change notification settings - Fork 17
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
Comments
Filed separately from comments on #76, so this can be discussed without affecting the merge of that PR. |
Couple of thoughts:
|
These two:
will no longer be used.
Nothing changed from today. When the protocol is
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. |
I don't think |
Discussed in the 8/19/24 SIG.
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. |
I was thinking about this more and came up with a slightly different direction we could go: What if we followed the the collector, and introduced separate exporters for [grpc](https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter/otlphttpexporter] and HTTP. We could then eliminate the The result would be something like:
One issue I see is that the collector uses So if we went in this direction, we'd need to decide whether to follow the now suboptimal naming of the collector, or introduce more explicit names like I think this solves the problems you outline while avoiding inventing anything new (i.e. leaning on prior art in the collector). |
Using |
The existing environment variables used in the spec to describe an endpoint:
are the result of incremental design, due to history.
The current design for config.yaml derives from this existing specification, and currently is:
Exposing the same fields to match the existing environment variables has some merits, but also carries over existing issues.
Existing configuration
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.
The message format, "protobuf" or "json", is mixed with the protocol.
I don't think it belongs there.
The "insecure" field is a last resort configuration parameter for grpc when not used with http/https.
This is too convoluted.
With the existing configuration, if someone changes the protocol between http and grpc, then the following fields must be adjusted as well:
insecure=false
may still be insecure.Proposed configuration
Examples of configuration:
Note how this simplifies things, as the endpoint URL specifies:
all this in the same field.
It then becomes much easier to change from:
to
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
andendpoint
fields is trivial.I think we should take this opportunity to simplify.
The text was updated successfully, but these errors were encountered: