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

Inconsistency between X_ENDPOINT and WithEndpoint #3730

Closed
dmathieu opened this issue Feb 14, 2023 · 9 comments · Fixed by #4808
Closed

Inconsistency between X_ENDPOINT and WithEndpoint #3730

dmathieu opened this issue Feb 14, 2023 · 9 comments · Fixed by #4808
Labels
bug Something isn't working pkg:exporter:otlp Related to the OTLP exporter package
Milestone

Comments

@dmathieu
Copy link
Member

dmathieu commented Feb 14, 2023

Description

When using the OTLP exporter, the WithEndpoint option requires an hostname, without the protocol.
However, the X_ENDPOINT environment variables requires an endpoint, including the protocol.

This can be misleading, as both config have very similar names, but expect different values.

Per the specification, the environment variable currently has the proper behavior.
I'm not seeing any spec for the config though.

I think we could solve this two ways:

  • Change WithEndpoint to require a protocol, and be the same as the environment variable.
  • Deprecate WithEndpoint, and replace it with WithHostname.
  • Keep WithEndpoint, and introduce a new WithEndpointURL method.

See #3726

@dmathieu dmathieu added the bug Something isn't working label Feb 14, 2023
@dubonzi
Copy link
Contributor

dubonzi commented Feb 14, 2023

Deprecation in favor of WithHostname might be the best option here as to not break current users of WithEndpoint.
Maybe including a warning during exporter initialization.

@dmathieu
Copy link
Member Author

Looking at the specification, I think introducing WithHostname doesn't match the specifications.
The specification link given above (https://github.com/open-telemetry/opentelemetry-specification/blob/203a8b4eb7806673f74d34b404f7761fd0b71a3a/specification/protocol/exporter.md) mentions "configuration options", not environment variables. So that includes all options.

That prevents us from doing option 2.

@pellared
Copy link
Member

According the the spec, for gRPC protocol, neither WithEndpoint nor OTEL_EXPORTER_OTLP_ENDPOINT env var requires providing the scheme.

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

Insecure: Whether to enable client transport security for the exporter's gRPC connection. This option only applies to OTLP/gRPC when an endpoint is provided without the http or https scheme

The gRPC flavor should work without providing http or https.

@MrAlias MrAlias added the pkg:exporter:otlp Related to the OTLP exporter package label Sep 14, 2023
@pellared
Copy link
Member

I am favor of not doing any breaking change. I find the existing documentation is good enough:

Note that the endpoint must not contain any URL path.

We could rather consider adding an additional option like WithEndpointURL for otlptracehttp and otlpmetrichttp which would provide a full URL where the exporter should send telemetry (without any manipulation like appending the path).

@pellared
Copy link
Member

pellared commented Oct 19, 2023

SIG meeting:

We can add WithEndpointURL and deprecate WithEndpoint in otlptracehttp.

Before otlpmetrichttp is stable we can add WithEndpointURL and remove WithEndopoint from otlpmetrichttp.
However, we may also release stable otlpmetrichttp and add WithEndpointURL and deprecate WithEndopoint in otlpmetrichttp later.

@pellared
Copy link
Member

pellared commented Nov 2, 2023

I think that we can simply improve the documentation to make it clear that value set via _ENDPOINT can be altered via WithInsecure, WithEndpoint and WithURLPath options.

While I am perfectly aware that it is inconvenient I have not heard a lot of complains about the current API so I would prefer to not increase the number of options.

@pellared pellared self-assigned this Nov 2, 2023
@JakeCooper
Copy link

JakeCooper commented Nov 10, 2023

I'll add a complaint. Lost an hour of my day until I picked up this thread.

I'll leave this comment as a little "frustration counter"

@pellared
Copy link
Member

pellared commented Nov 10, 2023

@JakeCooper I tried my best to improve the docs in #4695. I also plan to improve the https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp#WithEndpoint documentation of to include something like

This endpoint is specified as a host and optional port, no path or scheme should be included (see WithInsecure and WithURLPath).

(based on https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp#WithEndpoint)

Do you have any other proposals on how to avoid the frustration for future adopters?

@pellared
Copy link
Member

@dmathieu What do you think about updating the issue description to propose adding e.g. WithEndpointURL to otlptracehttp and otlpmetrichttp?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:exporter:otlp Related to the OTLP exporter package
Projects
None yet
5 participants