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

Update Jaeger environment variables #1752

Merged

Conversation

s-nadendla
Copy link
Contributor

Updated Jaeger environment variables to match Open Telemetry specification:

Removed envServiceName and envTags since the Jaeger exporter already gets attributes from a Resource that has its own system for determining environment values; overriding these values here is unnecessary. Removed this logic and the corresponding tests

Addresses: #1639

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 29, 2021

CLA Signed

The committers are authorized under a signed CLA.

@s-nadendla s-nadendla closed this Mar 29, 2021
@s-nadendla s-nadendla reopened this Mar 30, 2021
@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #1752 (0489674) into main (5843280) will decrease coverage by 0.1%.
The diff coverage is 64.2%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1752     +/-   ##
=======================================
- Coverage   78.7%   78.6%   -0.2%     
=======================================
  Files        134     134             
  Lines       7180    7156     -24     
=======================================
- Hits        5656    5628     -28     
- Misses      1274    1278      +4     
  Partials     250     250             
Impacted Files Coverage Δ
exporters/trace/jaeger/jaeger.go 93.4% <ø> (-0.1%) ⬇️
exporters/trace/jaeger/uploader.go 49.2% <33.3%> (-7.3%) ⬇️
exporters/trace/jaeger/agent.go 60.0% <75.0%> (+1.0%) ⬆️
exporters/trace/jaeger/env.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 83.9% <0.0%> (+1.5%) ⬆️

@s-nadendla s-nadendla changed the title Update Jaeger environment variables #1639 Update Jaeger environment variables Mar 30, 2021
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

Should we support OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT, which listed in #1639 ?

exporters/trace/jaeger/env.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@s-nadendla s-nadendla changed the title Update Jaeger environment variables [WIP] Update Jaeger environment variables Apr 1, 2021
@s-nadendla s-nadendla changed the title [WIP] Update Jaeger environment variables Update Jaeger environment variables Apr 1, 2021
@s-nadendla s-nadendla requested a review from MrAlias April 1, 2021 20:58
exporters/trace/jaeger/env.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/uploader.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/env_test.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/uploader.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/uploader.go Outdated Show resolved Hide resolved
@s-nadendla s-nadendla changed the title Update Jaeger environment variables [WIP] Update Jaeger environment variables Apr 5, 2021
@s-nadendla s-nadendla changed the title [WIP] Update Jaeger environment variables Update Jaeger environment variables Apr 6, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
exporters/trace/jaeger/agent.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/uploader.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/uploader.go Show resolved Hide resolved
exporters/trace/jaeger/uploader.go Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

🥇

@MrAlias
Copy link
Contributor

MrAlias commented Apr 9, 2021

@s-nadendla this looks ready to merge, can you resolve the conflicts and merge the main branch in?

@MrAlias MrAlias merged commit c5d006c into open-telemetry:main Apr 9, 2021
@s-nadendla s-nadendla deleted the 1639-UpdateJaegerEnvironmentVariables branch April 14, 2021 19:11
@MrAlias MrAlias mentioned this pull request Apr 23, 2021
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

5 participants