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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1e64a2b
Standardize OTEL_EXPORTER_JAEGER_PROTOCOL
pellared Feb 14, 2022
0be7bf2
Update sdk-environment-variables.md
pellared Feb 14, 2022
f0bcb48
Update CHANGELOG.md
pellared Feb 14, 2022
4254238
Update CHANGELOG.md
pellared Feb 14, 2022
63716b4
Update CHANGELOG.md
pellared Feb 14, 2022
1b746ac
Add udp/thrift.binary and reformat table
pellared Feb 15, 2022
b523b36
Merge branch 'main' into patch-2
pellared Feb 15, 2022
37885d2
Merge branch 'main' into patch-2
pellared Feb 16, 2022
e2c4178
Update sdk-environment-variables.md
pellared Feb 16, 2022
f701df3
Merge branch 'main' into patch-2
carlosalberto Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ release.
### Traces

- Clarify `StartSpan` returning the parent as a non-recording Span when no SDK
is in use
is in use.
([#2121](https://github.com/open-telemetry/opentelemetry-specification/pull/2121))
- Align Jaeger remote sampler endpoint with OTLP endpoint.
([#2246](https://github.com/open-telemetry/opentelemetry-specification/pull/2246))
Expand All @@ -45,9 +45,13 @@ release.
- Add support for probability sampling in the OpenTelemetry `tracestate` entry and
add optional specification for consistent probability sampling.
([#2047](https://github.com/open-telemetry/opentelemetry-specification/pull/2047))
- Change description and default value of OTEL_EXPORTER_JAEGER_ENDPOINT env var
to point to the correct HTTP port and correct description of OTEL_TRACES_EXPORTER
([#2333](https://github.com/open-telemetry/opentelemetry-specification/pull/2333)).
- Change description and default value of `OTEL_EXPORTER_JAEGER_ENDPOINT` environment
variable to point to the correct HTTP port and correct description of
`OTEL_TRACES_EXPORTER`.
([#2333](https://github.com/open-telemetry/opentelemetry-specification/pull/2333))
- Add `OTEL_EXPORTER_JAEGER_PROTOCOL` environment variable to select the protocol
used by the Jaeger exporter.
([#2341](https://github.com/open-telemetry/opentelemetry-specification/pull/2341))

### Metrics

Expand Down
36 changes: 19 additions & 17 deletions specification/sdk-environment-variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,25 @@ See [OpenTelemetry Protocol Exporter Configuration Options](./protocol/exporter.

| Name | Description | Default |
|---------------------------------|------------------------------------------------------------------|--------------------------------------------------------------------------------------------------|
| OTEL_EXPORTER_JAEGER_AGENT_HOST | Hostname for the Jaeger agent [1] | "localhost" |
| OTEL_EXPORTER_JAEGER_AGENT_PORT | Port for the Jaeger agent `compact` Thrift protocol | 6831 |
| OTEL_EXPORTER_JAEGER_ENDPOINT | Full URL of [Thrift over HTTP][jaeger_http] endpoint for Jaeger traces [2] | <!-- markdown-link-check-disable --> `"http://localhost:14268/api/traces"` <!-- markdown-link-check-enable --> |
| 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 •
edited
Loading

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 •
edited
Loading

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.

@tigrannajaryan @MrAlias PTAL e2c4178

| OTEL_EXPORTER_JAEGER_ENDPOINT (`http/thrift.binary` protocol) | Full URL of the [Jaeger HTTP endpoint][jaeger_collector] | `http://localhost:14268/api/traces` |
| OTEL_EXPORTER_JAEGER_ENDPOINT (`grpc` protocol) | URL of the [Jaeger gRPC endpoint][jaeger_collector] | `http://localhost:14250` |
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
pellared marked this conversation as resolved.
Show resolved Hide resolved
| OTEL_EXPORTER_JAEGER_TIMEOUT (`http/thrift.binary`, `grpc` protocols) | Maximum time the Jaeger exporter will wait for each batch export | 10s |
| OTEL_EXPORTER_JAEGER_USER (`http/thrift.binary`, `grpc` protocols) | Username to be used for HTTP basic authentication | |
| OTEL_EXPORTER_JAEGER_PASSWORD (`http/thrift.binary`, `grpc` protocols) | Password to be used for HTTP basic authentication | |
| OTEL_EXPORTER_JAEGER_AGENT_HOST (`udp/thrift.*` protocols) | Hostname of the [Jaeger agent][jaeger_agent] | `localhost` |
| OTEL_EXPORTER_JAEGER_AGENT_PORT (`udp/thrift.compact` protocol) | `udp/thrift.compact` port of the [Jaeger agent][jaeger_agent] | `6831` |
| OTEL_EXPORTER_JAEGER_AGENT_PORT (`udp/thrift.binary` protocol) | `udp/thrift.binary` port of the [Jaeger agent][jaeger_agent] | `6832` |
pellared marked this conversation as resolved.
Show resolved Hide resolved

[1] The default transport SHOULD be `http/thrift.binary` unless
SDKs have good reasons to choose other as the default
(e.g. for backward compatibility reasons).

[1] See [Jaeger Agent](https://www.jaegertracing.io/docs/latest/deployment/#agent) documentation.

[2] When the exporter uses the gRPC protocol, the environment variable refers to the gRPC endpoint and the default value should be `http://localhost:14250`.
[jaeger_http]: https://www.jaegertracing.io/docs/latest/apis/#thrift-over-http-stable
[jaeger_grpc]: https://www.jaegertracing.io/docs/latest/apis/#protobuf-via-grpc-stable
[jaeger_udp]: https://www.jaegertracing.io/docs/latest/apis/#thrift-over-udp-stable
[jaeger_collector]: https://www.jaegertracing.io/docs/latest/deployment/#collector
[jaeger_agent]: https://www.jaegertracing.io/docs/latest/deployment/#agent

## Zipkin Exporter

Expand Down Expand Up @@ -181,11 +190,7 @@ The SDK MAY accept a comma-separated list to enable setting multiple exporters.
Known values for `OTEL_TRACES_EXPORTER` are:

- `"otlp"`: [OTLP](./protocol/otlp.md)
- `"jaeger"`: export in Jaeger data model. If no additional configuration is
provided the default protocol SHOULD be [Thrift over HTTP][jaeger_http] unless
SDKs have good reasons to choose [gRPC][jaeger_grpc] as the default
(e.g. for backward compatibility reasons when gRPC was already the default
in a stable SDK release).
- `"jaeger"`: export in Jaeger data model
- `"zipkin"`: [Zipkin](https://zipkin.io/zipkin-api/) (Defaults to [protobuf](https://github.com/openzipkin/zipkin-api/blob/master/zipkin.proto) format)
- `"none"`: No automatically configured exporter for traces.

Expand Down Expand Up @@ -228,6 +233,3 @@ To ensure consistent naming across projects, this specification recommends that
```
OTEL_{LANGUAGE}_{FEATURE}
```

[jaeger_http]: https://www.jaegertracing.io/docs/latest/apis/#thrift-over-http-stable
[jaeger_grpc]: https://www.jaegertracing.io/docs/latest/apis/#protobuf-via-grpc-stable