Change default value for OTEL_EXPORTER_JAEGER_AGENT_PORT

[bastianeicher]

jaeger-agent accepts traces on two different ports:

• 6831 for the "normal" format used by most Jaeger clients
• 6832 for a Node.js-only format

(see docs)

I believe the default value for OTEL_EXPORTER_JAEGER_AGENT_PORT should therefore be 6831 and not 6832.

[carlosalberto]

Overall fine but I'm wondering as this is a breaking change, and may be hence breaking people's existing deployments. Any opinion about that?

[bastianeicher]

opentelemetry-java does not use OTEL_EXPORTER_JAEGER_AGENT_PORT and specifies no default Jaeger agent port.

opentelemetry-dotnet does not use OTEL_EXPORTER_JAEGER_AGENT_PORT and already defaults to 6831.

opentelemetry-go does use OTEL_EXPORTER_JAEGER_AGENT_PORT and currently defaults to 6832.
However, I'm pretty sure almost everyone using opentelemetry-go with Jaeger will have done so by setting OTEL_EXPORTER_JAEGER_AGENT_PORT=6831 rather than reconfiguring jaeger-agent to swap port numbers.

[dyladan]

So should JS continue to use 6832? This is what jaeger's client library does.

[carlosalberto]

So should JS continue to use 6832? This is what jaeger's client library does.

Great question. @yurishkuro any opinion on this?

[yurishkuro]

The reason Jaeger JS SDK used a different port compared to all other SDKs is because there was no support for compact encoding in the JS version of Thrift library that we used (only binary encoding was available).

What Thrift library does the JS Jaeger exporter use?

If it can produce compact encoding, then it can use the same default port. If it can only produce binary encoding, it must use a different default port 6832 (fwiw if anyone's interested to help out, we have a ticket and some investigation into auto-recognizing the payload format on the backend so that both ports could be used jaegertracing/jaeger#1596).

[dyladan]

We are using the jaeger library directly which uses thriftrw.

From the thriftrw README:

The scope of this project may eventually cover alternate Thrift binary encodings including the compact binary protocol.

So no, it does not currently support the compact encoding.

[yurishkuro]

Then JS needs to use a different default port from the rest of the SDKs. I am fairly confident we cannot expect new features in thriftrw.

We can change the spec to be explicit:

• if SDK supports compact thrift, then default to 6831
• if SDK only supports binary thrift, then default to 6832

[carlosalberto]

Do other Jaeger transports (e.g. grpc) allow specifying port?

[carlosalberto]

Hey @bastianeicher, would you mind updating this PR according to @yurishkuro's suggestion?

[bastianeicher]

Maybe it would be better to use a different environment variable name to specify the binary thrift port?

E.g., opentelemetry-js could use OTEL_EXPORTER_JAEGER_AGENT_BINARY_PORT while all other otel SDKs stick with the existing OTEL_EXPORTER_JAEGER_AGENT_PORT.

Having the same environment variable use different default values and have different meanings in different SDKs seems risky to me. Maybe someone wants to set OTEL_EXPORTER_JAEGER_AGENT_PORT for all processes in their environment, without caring about which language they are written in.

[bastianeicher]

I've updated the PR with my suggested two env var solution, plus links to the relevant Jaeger docs.

[yurishkuro]

I would rather say that JS has a different default than introduce an extra one-off var.

[bastianeicher]

I'm worried about a scenario like this:

Somebody has set up their Jaeger agents to listen on non-default ports like 1234 for compact and 1235 for binary.
(This may not be a very common thing, but without non-default ports there's no need for the env vars anyway.)

Now they read the otel docs and discovers OTEL_EXPORTER_JAEGER_AGENT_PORT. They decide to set this environment variable for their entire environment. E.g., for all processes on a VM or as a default in a shared Docker base image or Helm chart.

Now, there is no way for them to pick the "correct" value for the variable, because it needs to be either 1234 or 1235 based on which otel SDK the various processes are using. While it's of course possible to set process-specific values, imho this seems to run counter to the spirit of environment variables.

[pellared]

Go also uses the binary format: https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/jaeger . Those it mean that it should use compact format instead?

FYI @MrAlias @Aneurysm9

[bastianeicher]

opentelemetry-go already uses the compact format:
https://github.com/open-telemetry/opentelemetry-go/blob/3be9813d56ff53aede1f2c77c54fd5bd3e53dcc7/exporters/jaeger/agent.go#L79

I believe it's just a bug-in-docs that the readme says "binary thrift protocol".

[pellared]

@benhastings Thx. I created a bug: open-telemetry/opentelemetry-go#2130

[arminru]

@bastianeicher Would you be fine with @MrAlias suggestion to carve it out from this PR to discuss it (adding a separate env var or making the default conditional) separately? Then we should be good to merge the rest right away.

[bastianeicher]

@MrAlias, @arminru I've changed the PR back to its original scope of only changing the default value for OTEL_EXPORTER_JAEGER_AGENT_PORT, as you suggested.

[arminru]

Thanks, Bastian!
Will you file a follow-up PR or should we ask the @open-telemetry/javascript-maintainers to take care of that?

[dyladan]

Hello, js maintainer here. Is the suggested action item for JS to introduce OTEL_EXPORTER_JAEGER_AGENT_BINARY_PORT or for JS to simply have a different default value for OTEL_EXPORTER_JAEGER_AGENT_PORT?

[arminru]

@dyladan I'll merge the PR for now since this part is quite clear, let's discuss the way to go for JS in a follow-up issue or PR.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.