-
Notifications
You must be signed in to change notification settings - Fork 888
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
Update endpoint for OTLP/HTTP to allow scheme #1234
Update endpoint for OTLP/HTTP to allow scheme #1234
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an entry in the changelog.
specification/protocol/exporter.md
Outdated
@@ -8,9 +8,9 @@ The following configuration options MUST be available to configure the OTLP expo | |||
|
|||
| Configuration Option | Description | Default | Env variable | | |||
| -------------------- | ------------------------------------------------------------ | ----------------- | ------------------------------------------------------------ | | |||
| Endpoint | Target to which the exporter is going to send spans or metrics. This MAY be configured to include a path (e.g. `example.com/v1/traces`). | `localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_SPAN_ENDPOINT` `OTEL_EXPORTER_OTLP_METRIC_ENDPOINT` | | |||
| Endpoint | Target to which the exporter is going to send spans or metrics. For OTLP/gRPC the endpoint MUST contain a host and MAY contain a port. For OTLP/HTTP the endpoint MUST a valid URL with scheme and host and MAY contain a port and path. | `localhost:4317`, `http://example.com/v1/traces` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_SPAN_ENDPOINT` `OTEL_EXPORTER_OTLP_METRIC_ENDPOINT` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this backwards-compatible and assume https
if not provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
CC: @open-telemetry/collector-approvers |
@tigrannajaryan Maybe you are the right person to poke here? |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
I am not quite sure I like the proposed change.
Do we ignore I am not strongly opposed to this change, but at the minimum would like clarifications on point 2. |
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).
|
@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. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Ping @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:
rather than:
|
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 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). |
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 My rationale for these two statements:
In that sense, I don't think this does enough. Let's get rid of |
+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. |
+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. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@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. |
@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 |
I support the single variable approach. Much easier to use. If we want fancy, GRPC implementation can ignore the protocol so both |
3453d3e
to
010ee8a
Compare
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 I can also remove the |
Consider opening an issue for this (specially if it drags off the current PR). |
c639489
to
8fc4a70
Compare
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
eadee2c
to
ae5f21d
Compare
Ping @tigrannajaryan |
variables weren't adopted yet so it's ok to change. Will merge tomorrow morning if nobody will object. |
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 theinsecure
bit only applies to gRPC.