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

Fix per-signal endpoint parsing in OTLP exporters #2433

Merged
merged 7 commits into from Dec 9, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 7, 2021

Resolve #2338

@MrAlias MrAlias added the pkg:exporter:otlp Related to the OTLP exporter package label Dec 7, 2021
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #2433 (6977091) into main (b177541) will increase coverage by 0.0%.
The diff coverage is 94.4%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2433   +/-   ##
=====================================
  Coverage   76.0%   76.0%           
=====================================
  Files        173     173           
  Lines      11938   12008   +70     
=====================================
+ Hits        9074    9134   +60     
- Misses      2621    2629    +8     
- Partials     243     245    +2     
Impacted Files Coverage Δ
...s/otlp/otlpmetric/internal/otlpconfig/envconfig.go 94.8% <94.2%> (+1.7%) ⬆️
...rs/otlp/otlptrace/internal/otlpconfig/envconfig.go 94.8% <94.2%> (-1.2%) ⬇️
exporters/otlp/otlpmetric/otlpmetrichttp/client.go 78.7% <100.0%> (ø)
exporters/otlp/otlptrace/otlptracehttp/client.go 83.3% <100.0%> (ø)
sdk/metric/refcount_mapped.go 80.0% <0.0%> (-20.0%) ⬇️
sdk/metric/sdk.go 80.0% <0.0%> (-1.5%) ⬇️

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I think it could be good to update the docs e.g. here: https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace

Can be done in a separate PR.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 8, 2021

I think it could be good to update the docs e.g. here: https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace

The linked documentation does not contain any mention of the behavior that is being fixed here as far as I can see. I'm not sure what needs to be updated there.

CHANGELOG.md Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Dec 9, 2021

I think it could be good to update the docs e.g. here: https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace

The linked documentation does not contain any mention of the behavior that is being fixed here as far as I can see. I'm not sure what needs to be updated there.

I find it very inconvenient that there is no information that OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_ENDPOINT work differently. It suggests that all the configurations are equivalent. I suggest at least giving a reference to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp or whole https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 9, 2021

I find it very inconvenient that there is no information that OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_ENDPOINT work differently. It suggests that all the configurations are equivalent. I suggest at least giving a reference to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp or whole https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md

Ah, I see. Let me look into adding something.

@MrAlias MrAlias merged commit 9312664 into open-telemetry:main Dec 9, 2021
@MrAlias MrAlias deleted the otlp-endpoint branch December 9, 2021 18:40
@Aneurysm9 Aneurysm9 mentioned this pull request Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:exporter:otlp Related to the OTLP exporter package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exporter signal-specific environment variables interpretation
4 participants