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

Clarify how the default value of OTEL_EXPORTER_JAEGER_ENDPOINT should work #2331

Closed
abe545 opened this issue Feb 10, 2022 · 11 comments · Fixed by #2333
Closed

Clarify how the default value of OTEL_EXPORTER_JAEGER_ENDPOINT should work #2331

abe545 opened this issue Feb 10, 2022 · 11 comments · Fixed by #2333
Assignees
Labels
spec:trace Related to the specification/trace directory

Comments

@abe545
Copy link

abe545 commented Feb 10, 2022

What are you trying to achieve?
My organization is trying to switch from opentracing to opentelemetry, and currently the .net exporter will not work with our infrastructure, because our jaeger collector listens on a different path than /api/traces. With the opentracing jaeger client, we were able to specify the endpoint and have everything wired up correctly. With the current spec, the default value of the OTEL_EXPORTER_JAEGER_ENDPOINT env var makes it seem like libraries need to append /api/traces themselves to correctly send spans over http.

What did you expect to see?
I'd like it to be clear in the spec how this should behave - should libraries append /api/traces - or should the default value include the path in the uri, and expect developers to configure the full url to export to. If libraries should append the path, can we add another configuration option to override it (this is what my organization needs to adopt otel in .net).

Additional context.
The .net sdk is currently implemented to append /api/traces to the endpoint, but the golang, javascript, and ruby sdks all default the value of the endpoint to include the path, and require users to specify the path in the value.
I opened a PR to try to make the .net sdk configurable: open-telemetry/opentelemetry-dotnet#2847
This led to a discussion where one of the maintainers felt that the golang implementation is wrong: open-telemetry/opentelemetry-go#2599

@pellared
Copy link
Member

pellared commented Feb 10, 2022

Related issue in OTel Go SDK open-telemetry/opentelemetry-go#2599

@carlosalberto
Copy link
Contributor

Re-assigning this to @yurishkuro who may know better.

@yurishkuro
Copy link
Member

The environment variable should be assumed to contain the actual URL, including the path. The SDK SHOULD NOT append anything to the URL, except, in come cases, the parameters in the query string, such as ?service=*** for remote sampling URL.

If this is not clear in the spec, we should make it clear, and I think the .NET implementation is incorrect.

@abe545
Copy link
Author

abe545 commented Feb 10, 2022

The environment variable should be assumed to contain the actual URL, including the path. The SDK SHOULD NOT append anything to the URL, except, in come cases, the parameters in the query string, such as ?service=*** for remote sampling URL.

If this is not clear in the spec, we should make it clear, and I think the .NET implementation is incorrect.

Thanks for the clarification @yurishkuro. Would you like me to open a PR making it clear in the documentation?

@yurishkuro
Copy link
Member

I am creating one now, will tag you for review.

@yurishkuro
Copy link
Member

Actually, something is odd here. The default value in the spec is http://localhost:14250, which is a gRPC port number in Jaeger and thus does not need the path. The default Thrift over HTTP endpoint is http://jaeger:14268/api/traces, which does need the path. Which one are you (@abe545) and the spec are referring to?

@abe545
Copy link
Author

abe545 commented Feb 10, 2022

@yurishkuro
Copy link
Member

I know, what the spec says currently seems inconsistent, it says HTTP but lists the gRPC port number.

image

@abe545
Copy link
Author

abe545 commented Feb 10, 2022

Yeah - I'm trying to configure it to for HTTP.

@pellared
Copy link
Member

pellared commented Feb 10, 2022

Can we also document the OTEL_EXPORTER_JAEGER_PROTOCOL env var?

I think it is related.

Origin open-telemetry/opentelemetry-dotnet-instrumentation#355 (comment).

How about making the spec similar to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md ?

@yurishkuro
Copy link
Member

I think the open question to me is what data format other SDKs are using currently. I checked the Go SDL, it uses Thrift over HTTP, and they even made a change to use a different default URL in open-telemetry/opentelemetry-go#1898, with the correct port.

Here's the proposed fix #2333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:trace Related to the specification/trace directory
Projects
None yet
5 participants