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

Add OTEL_EXPORTER_JAEGER_PROTOCOL and improve description of other OTEL_EXPORTER_JAEGER_* env vars #2341

Merged
merged 10 commits into from
Feb 17, 2022

Conversation

pellared
Copy link
Member

@pellared pellared commented Feb 14, 2022

Follow-up of #2333

Changes

Preview here

  • Add the OTEL_EXPORTER_JAEGER_PROTOCOL environment variable to select the protocol used
    by the Jaeger exporter.
  • More explicit description of the rest of the OTEL_EXPORTER_JAEGER_* environment variables (e.g. when they are applicable - it also represents the current state of SDKs)

Here are the following reasons for this change

  1. Enable the possibility of selecting the protocol when the Jaeger exporter is set via env var OTEL_TRACES_EXPORTER=jaeger (instead of supporting values like OTEL_TRACES_EXPORTER=jaeger-udp).
  2. This resembles the configuration of the OTLP exporter which has OTEL_EXPORTER_OTLP_PROTOCOL
  3. The newest OTel .NET SDK RC version supports this env var. See here. I feel that it is safer to standardize it (especially the supported values) before it is released
  4. We want to take advantage of it in OTel .NET AutoInstrumentation. However, I believe this could be useful in other places as well.
  5. Node.js SDK uses 6832 as the default Agent port because it uses a different protocol

Related issues:

FYI (all SIG maintainers where I see that OTEL_EXPORTER_JAEGER_ENDPOINT is used) @open-telemetry/dotnet-maintainers @open-telemetry/dotnet-instrumentation-maintainers @open-telemetry/java-instrumentation-maintainers @open-telemetry/python-maintainers @open-telemetry/go-maintainers @open-telemetry/php-maintainers @open-telemetry/java-maintainers @open-telemetry/ruby-maintainers @open-telemetry/rust-maintainers

Alternative

Instead of adding OTEL_EXPORTER_JAEGER_PROTOCOL we can specify more specific values for OTEL_TRACES_EXPORTER like jaeger/http/thrift.binary but this would be inconsistent with the OTLP exporter configuration.

TODO in next PR

@pellared pellared changed the title [WIP] Standardize OTEL_EXPORTER_JAEGER_PROTOCOL Add OTEL_EXPORTER_JAEGER_PROTOCOL to select the protocol used by the Jaeger exporter Feb 14, 2022
@pellared pellared changed the title Add OTEL_EXPORTER_JAEGER_PROTOCOL to select the protocol used by the Jaeger exporter Add OTEL_EXPORTER_JAEGER_PROTOCOL Feb 14, 2022
@pellared pellared marked this pull request as ready for review February 14, 2022 10:04
@pellared pellared requested review from a team February 14, 2022 10:04
@pellared pellared marked this pull request as draft February 14, 2022 18:18
@pellared pellared changed the title Add OTEL_EXPORTER_JAEGER_PROTOCOL [WIP] Add OTEL_EXPORTER_JAEGER_PROTOCOL Feb 14, 2022
@pellared
Copy link
Member Author

Going to address @yurishkuro comments tomorrow

@pellared pellared changed the title [WIP] Add OTEL_EXPORTER_JAEGER_PROTOCOL Add OTEL_EXPORTER_JAEGER_PROTOCOL Feb 15, 2022
@pellared pellared marked this pull request as ready for review February 15, 2022 09:36
@pellared
Copy link
Member Author

Ready for another review round.

@pellared pellared changed the title Add OTEL_EXPORTER_JAEGER_PROTOCOL Add OTEL_EXPORTER_JAEGER_PROTOCOL and improve description of other OTEL_EXPORTER_JAEGER_* env vars Feb 15, 2022
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Side note: the formatting of this page makes Github diffs difficult to read. Perhaps we need to reformat it similar to #2345 (can be done in a separate, future PR).

specification/sdk-environment-variables.md Show resolved Hide resolved
| OTEL_EXPORTER_JAEGER_TIMEOUT | Maximum time the Jaeger exporter will wait for each batch export | 10s |
| OTEL_EXPORTER_JAEGER_USER | Username to be used for HTTP basic authentication | |
| OTEL_EXPORTER_JAEGER_PASSWORD | Password to be used for HTTP basic authentication | |
| OTEL_EXPORTER_JAEGER_PROTOCOL | The transport protocol. Options MAY include [`http/thrift.binary`][jaeger_http], [`grpc`][jaeger_grpc], [`udp/thrift.compact`][jaeger_udp], [`udp/thrift.binary`][jaeger_udp] | `http/thrift.binary` [1] |
Copy link
Member

Choose a reason for hiding this comment

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

Why does this say MAY? This is not a recommendation, it is a strict list. Should it say: MUST be one of ... instead?.

Copy link
Member Author

@pellared pellared Feb 15, 2022

Choose a reason for hiding this comment

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

Taken from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md. The same wording is used in #2345

I would prefer to address it in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

I still think MAY is not the right word here and other similar places that you found, but I am fine if it is addressed in a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will create issues for it once this PR is merged and assign it to myself.

Copy link
Contributor

@MrAlias MrAlias Feb 16, 2022

Choose a reason for hiding this comment

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

Addressing this addition in a future PR could potentially lead to compatibility issues. Reviewers of the future PR may not know if this change was released and "upgrading" from an option to a requirement would be a backwards incompatible change. Or, even worse, this would be released prior to the follow on PR being merged. Given this section of the doc is marked stable, this seems like a valid concern.

Can we address the change from MAY to MUST for these changes in this PR instead? That way it will be a cohesive and complete change that is not copying prior errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pellared pellared requested a review from owais February 16, 2022 09:00
@carlosalberto
Copy link
Contributor

Ping @owais ;)

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.

8 participants