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

Split OTEL_EXPORTER to OTEL_TRACE_EXPORTER and OTEL_METRICS_EXPORTER #1318

Merged
merged 8 commits into from
Jan 13, 2021

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Jan 5, 2021

Fixes #1309
Fixes #1313

Changes

Splits OTEL_EXPORTER into two variables that only accept a single value. This makes the syntax of the variable consistent with others (singular name, singular value) and removes some corner cases in configuration caused by the previous list (multiple jaeger exporters accept different format for endpoint, push / pull metrics exporters can't be enabled at the same time, inconsistency in not having a way of enabling multiple exporters of a same type).

@@ -77,9 +77,9 @@ See [OpenTelemetry Protocol Exporter Configuration Options](./protocol/exporter.

| Name | Description | Default |
| ------------- | ---------------------------------------------------------------------------- | ------- |
| OTEL_EXPORTER | Exporter to be used, can be a comma-separated list to use multiple exporters | "otlp" |
| OTEL_EXPORTERS | Exporters to be used as a comma separated list | "otlp" |
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit annoying that we support exporting to two different exporters at the same time, but not two differently configured exporters of the same kind (e.g. zipkin + jaeger works, but not two different jaeger URLs). But this is a edge case anyway, because contrary to propators, you should usually only have a single exporter. Maybe that was the reason for the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to already be common to specify two exporters, one for trace and one for metrics (e.g., otlp_span and prometheus). Should we have two variables for each and drop the ability to set a list for either of them to simplify things? We also get the win of not having the otlp_span and otlp_metrics variables.

I agree it's extremely rare to want to set two exporters for the same signal, and it would still be possible programmatically if really needed.

Copy link
Member

@Oberon00 Oberon00 Jan 5, 2021

Choose a reason for hiding this comment

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

I like the idea of having two variables OTEL_SPAN/TRACE_EXPORTER and OTEL_METRIC_EXPORTER 👍
I think this could also make implementation simpler. For example right now, probably you might have one place that searches the metric exporter and another place that searches the span exporter, and if there is an unknown exporter you might not even be able to warn the user because it might just be an exporter for another signal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyone else that likes signal-specific exporters, please thumbs up and after a couple more votes I'll rework the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and split them up. Wondering if it means others should be e.g., OTEL_TRACE_EXPORTER_JAEGER_AGENT_HOST

@anuraaga anuraaga changed the title Rename OTEL_EXPORTER to OTEL_EXPORTERS Split OTEL_EXPORTER to OTEL_TRACE_EXPORTER and OTEL_METRICS_EXPORTER Jan 6, 2021
@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review. This is a small yet important change, that I'd like to have a lot of eyes on it ;)

Comment on lines 78 to 79
We define environment variables for setting a single exporter per signal. SDKs SHOULD provide a means to
set multiple exporters for a signal programmatically.
Copy link
Member

Choose a reason for hiding this comment

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

I am confused. First it says setting "a single exporter". Then it says set "multiple exporters". Is it "single" or "multiple"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is whether it's an environment variable or programmatically. Not sure how to reword it to be less confusing but let me know if you have an idea :) Or if this point isn't helpful (doc is about env variables) I can delete it too.

Copy link
Member

Choose a reason for hiding this comment

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

not helpful here - SDK API requirements should be in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - removed

spec-compliance-matrix.md Outdated Show resolved Hide resolved
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
@bogdandrutu bogdandrutu merged commit 9f49515 into open-telemetry:master Jan 13, 2021
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…pen-telemetry#1318)

* Rename OTEL_EXPORTER to OTEL_EXPORTERS

* Split exporter variables

* Update compliance matrix

* Clean

* Changelog

* Update spec-compliance-matrix.md

Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>

Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants