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

Clarify OTEL_EXPORTER_OTLP_ENDPOINT envvar for OTLP/HTTP. #1975

Conversation

Oberon00
Copy link
Member

The current wording of the OTLP endpoint config was confusing, especially the "if not present already" wording. Instead, clarify that we must always append when using the envvar for all signals (it was already clearly specified that the per-signal vars do not get the path appended).

This came up in open-telemetry/opentelemetry-java#3650 and again at open-telemetry/opentelemetry-java#3666 (comment).

Also make it a MUST (not SHOULD) since this kind of thing would be extremely annoying to have differences per-language in. Also, without appending, the variable cannot be used to configure more than one signal which would defeat its sole purpose.

@Oberon00 Oberon00 added the area:sdk Related to the SDK label Sep 28, 2021
@Oberon00 Oberon00 requested review from a team September 28, 2021 13:48
@Oberon00 Oberon00 changed the title Clarify OTEL_EXPORTER_OTLP_ENDPOINT endpoint envvar for OTLP/HTTP. Clarify OTEL_EXPORTER_OTLP_ENDPOINT envvar for OTLP/HTTP. Sep 28, 2021
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.

LGTM, once duplicate mycollector/ that @fbogsany noticed is fixed.

@arminru arminru added the area:configuration Related to configuring the SDK label Sep 30, 2021
Oberon00 and others added 3 commits September 30, 2021 11:34
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
…-envvar

 Conflicts:
	specification/protocol/exporter.md
@carlosalberto
Copy link
Contributor

@iNikem Any further comment? Or are you ok with the current PR?

specification/protocol/exporter.md Show resolved Hide resolved
@tigrannajaryan tigrannajaryan enabled auto-merge (squash) October 4, 2021 08:36
@tigrannajaryan tigrannajaryan merged commit 2d72f95 into open-telemetry:main Oct 4, 2021
@Oberon00 Oberon00 deleted the clarify-otlp-config-envvar branch October 4, 2021 09:16
tigrannajaryan pushed a commit that referenced this pull request Oct 12, 2021
Clarifies that default ports for the URL schemes, not default OTLP/HTTP port should be used if no port is given in URL.

See #1975 (comment)
tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-proto that referenced this pull request Apr 20, 2023
Clarifies that default ports for the URL schemes, not default OTLP/HTTP port should be used if no port is given in URL.

See open-telemetry/opentelemetry-specification#1975 (comment)
joaopgrassi pushed a commit to dynatrace-oss-contrib/semantic-conventions that referenced this pull request Mar 21, 2024
Clarifies that default ports for the URL schemes, not default OTLP/HTTP port should be used if no port is given in URL.

See open-telemetry/opentelemetry-specification#1975 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK area:sdk Related to the SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants