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

Populate Jaeger's Span.Process from Resource #1673

Merged
merged 13 commits into from
Mar 16, 2021
Merged

Populate Jaeger's Span.Process from Resource #1673

merged 13 commits into from
Mar 16, 2021

Conversation

XSAM
Copy link
Member

@XSAM XSAM commented Mar 7, 2021

Resolves #1550

@codecov
Copy link

codecov bot commented Mar 7, 2021 •
edited
Loading

Codecov Report

Merging #1673 (057880e) into main (28eaaa9) will increase coverage by 0.0%.
The diff coverage is 96.4%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1673   +/-   ##
=====================================
  Coverage   77.7%   77.8%           
=====================================
  Files        130     130           
  Lines       6955    6986   +31     
=====================================
+ Hits        5407    5436   +29     
- Misses      1298    1299    +1     
- Partials     250     251    +1
Impacted Files Coverage Δ
exporters/trace/jaeger/jaeger.go 93.7% <96.4%> (-0.1%) ⬇️

@XSAM XSAM marked this pull request as ready for review March 8, 2021 07:58
evantorrie
evantorrie reviewed Mar 9, 2021
View reviewed changes
Copy link
Contributor

@evantorrie evantorrie left a comment

Choose a reason for hiding this comment

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

Looks good! Some comments you may want to look at.

exporters/trace/jaeger/jaeger.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/jaeger.go Show resolved Hide resolved
exporters/trace/jaeger/jaeger.go Show resolved Hide resolved
exporters/trace/jaeger/jaeger.go Show resolved Hide resolved
exporters/trace/jaeger/jaeger.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/jaeger_test.go Show resolved Hide resolved
exporters/trace/jaeger/jaeger_test.go Show resolved Hide resolved
exporters/trace/jaeger/jaeger_test.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/jaeger_test.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/jaeger_test.go Show resolved Hide resolved
@XSAM XSAM mentioned this pull request Mar 13, 2021
Closed
3 tasks
@Aneurysm9 Aneurysm9 merged commit 62cbf0f into open-telemetry:main Mar 16, 2021
@XSAM XSAM deleted the jaeger-process branch March 17, 2021 11:53
@MrAlias MrAlias mentioned this pull request Mar 18, 2021
Merged
@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
Reviewers

@evantorrie evantorrie evantorrie approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@jmacd jmacd Awaiting requested review from jmacd

@lizthegrey lizthegrey Awaiting requested review from lizthegrey

@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
untracked
Development

Successfully merging this pull request may close these issues.

Jaeger exporter must populate Jaeger's Span.Process from Resource service.name attribute
4 participants
@XSAM @Aneurysm9 @evantorrie @pellared