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

Always include Zipkin's localEndpoint serviceName #1481

Merged

Conversation

pmm-sumo
Copy link
Contributor

@pmm-sumo pmm-sumo commented Aug 3, 2020

Description:

Fixes a bug with service name being skipped when span contains no attributes

Link to tracking Issue: #1480

Testing: Added unit test and tested manually

Documentation: No extra documentation added

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 3, 2020

CLA Check
The committers are authorized under a signed CLA.

@bogdandrutu
Copy link
Member

/cc @kbrockhoff

@pmm-sumo
Copy link
Contributor Author

pmm-sumo commented Aug 3, 2020

Seems that the previous CLA didn't get migrated, I will work on getting this fixed

@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #1481 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1481      +/-   ##
==========================================
+ Coverage   90.92%   90.95%   +0.02%     
==========================================
  Files         239      239              
  Lines       16706    16709       +3     
==========================================
+ Hits        15190    15197       +7     
+ Misses       1085     1083       -2     
+ Partials      431      429       -2     
Impacted Files Coverage Δ
translator/trace/zipkin/protospan_to_zipkinv1.go 89.59% <100.00%> (+1.36%) ⬆️
translator/internaldata/resource_to_oc.go 87.50% <0.00%> (ø)
internal/internal.go 89.47% <0.00%> (+10.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0106dc...d766e3b. Read the comment docs.

@pmm-sumo
Copy link
Contributor Author

pmm-sumo commented Aug 6, 2020

Thanks for the review and sorry for the delay with EasyCLA migration. Should be good now @bogdandrutu @pjanotti

@bogdandrutu bogdandrutu merged commit c347178 into open-telemetry:master Aug 11, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
Based on its module, it should be go.opencensus.io/otel/example/opencensus.
However, it's simpler to get rid of it since main packages shouldn't be imported at all.

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants