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

Correct description of OTEL_EXPORTER_JAEGER_ENDPOINT and OTEL_TRACES_EXPORTER #2333

Merged
merged 11 commits into from
Feb 13, 2022

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Feb 10, 2022

Fixes #2331

Changes

The OTEL_EXPORTER_JAEGER_ENDPOINT env var description is updated:

  • clarify that it refers to Thrift over HTTP endpoint, provide a link to Jaeger API spec
  • change the default value to point to the correct HTTP port, not the gRPC port
  • clarify that the path (/api/traces) must be included in the value, to support installations running at remapped URLs

The OTEL_TRACES_EXPORTER env var description updated to reflect state of the world where all but Java SDKs implement http/thrift protocol, not gRPC as was stated in the description.

…to correct HTTP port

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link

@abe545 abe545 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much clearer, thanks!

@carlosalberto
Copy link
Contributor

@yurishkuro Should we get this as part of 1.9, or should we hold this? (Sounds like the former?)

@yurishkuro
Copy link
Member Author

Yes, would be good to include that (ref #2329)

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not find this PR as a "complete" solution, because:

  • It is a breaking change for the default value which is used e.g. in OTel Java
  • it does not play well with the OTEL_TRACES_EXPORTER=jaeger which is defined as Jaeger over gRPC (it is defined in the same markdown file)

I propose defining OTEL_EXPORTER_JAEGER_PROTOCOL. Then the default value of OTEL_EXPORTER_JAEGER_ENDPOINT could depend on the selected protocol. This would be similar to the OTLP Exporter configuration.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Feb 10, 2022

Let's not delay 1.9.0 release if there are still debates on this PR. We have delayed the release for a while now.

(Unless this a critical fix and we cannot release without it).

@yurishkuro
Copy link
Member Author

@pellared I do not believe that OTEL_TRACES_EXPORTER=jaeger == gRPC is actually respected by the SDKs, because Java is the only SDK that even has a + against Protobuf/gRPC in the compliance matrix, whereas almost all SDKs support Thrift/HTTP. So I would suggest changing the description for OTEL_TRACES_EXPORTER=jaeger to say something like "usually HTTP, sometimes gRPC, please check the SDK docs". Then introducing OTEL_EXPORTER_JAEGER_PROTOCOL should be a separate PR since (a) I don't know the level of consensus around it, and (b) this PR is mostly about correcting the wrong value for HTTP endpoint that all (except Java) SDKs implement currently.

@pellared
Copy link
Member

pellared commented Feb 10, 2022

@pellared I do not believe that OTEL_TRACES_EXPORTER=jaeger == gRPC is actually respected by the SDKs, because Java is the only SDK that even has a + against Protobuf/gRPC in the compliance matrix, whereas almost all SDKs support Thrift/HTTP. So I would suggest changing the description for OTEL_TRACES_EXPORTER=jaeger to say something like "usually HTTP, sometimes gRPC, please check the SDK docs". Then introducing OTEL_EXPORTER_JAEGER_PROTOCOL should be a separate PR since (a) I don't know the level of consensus around it, and (b) this PR is mostly about correcting the wrong value for HTTP endpoint that all (except Java) SDKs implement currently.

It could be done similarly to the OTLP Exporter. Something like:

[*] If no configuration is provided
the default protocol SHOULD be `http/thrift`
unless SDKs have good reasons to choose other as the default
(e.g. for backward compatibility reasons
when `grpc` was already the default in a stable SDK release).

@nrcventura
Copy link
Member

So I would suggest changing the description for OTEL_TRACES_EXPORTER=jaeger to say something like "usually HTTP, sometimes gRPC, please check the SDK docs".

I think that updating that description would be great, and better matches the behavior I saw described across a handful of SDK implementations.

I think that adding the protocol environment variable to the spec is something that's nice to have, but should probably be discussed separately.

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro changed the title Change description/default of OTEL_EXPORTER_JAEGER_ENDPOINT to point to correct HTTP port Correct description of OTEL_EXPORTER_JAEGER_ENDPOINT and OTEL_TRACES_EXPORTER Feb 10, 2022
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@carlosalberto
Copy link
Contributor

@pellared Does the latest commit fix your issue?

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

Successfully merging this pull request may close these issues.

Clarify how the default value of OTEL_EXPORTER_JAEGER_ENDPOINT should work
8 participants