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

Wire instrumentation library info through SpanFactory to SpanInfo #154

Merged
merged 5 commits into from
Oct 23, 2020

Conversation

HaloFour
Copy link
Collaborator

To support SPI scenarios where an instrumentation library will request a tracer with the instrumentation library information which may be used by the span handler/exporter wire the instrumentation library information (name and version) through the CoreSpan and SpanInfo.

@HaloFour HaloFour added the WIP Work-in-Progress label Oct 23, 2020
@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #154 into master will decrease coverage by 0.20%.
The diff coverage is 82.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   94.06%   93.85%   -0.21%     
==========================================
  Files          77       77              
  Lines        1079     1091      +12     
  Branches       63       77      +14     
==========================================
+ Hits         1015     1024       +9     
- Misses         64       67       +3     
Impacted Files Coverage Δ
.../comcast/money/otel/handlers/OtelSpanHandler.scala 100.00% <ø> (ø)
.../scala/com/comcast/money/wire/SpanConverters.scala 91.66% <0.00%> (-1.11%) ⬇️
...st/money/otel/handlers/MoneyReadableSpanData.scala 96.42% <66.66%> (-3.58%) ⬇️
...a/com/comcast/money/core/MoneyTracerProvider.scala 87.50% <87.50%> (-12.50%) ⬇️
...c/main/scala/com/comcast/money/core/CoreSpan.scala 94.28% <100.00%> (+0.16%) ⬆️
...scala/com/comcast/money/core/CoreSpanFactory.scala 94.11% <100.00%> (+0.36%) ⬆️
.../src/main/scala/com/comcast/money/core/Money.scala 94.44% <100.00%> (+0.32%) ⬆️

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 6cf2056...c73d17e. Read the comment docs.

@HaloFour HaloFour changed the title WIP: Wire instrumentation library info through SpanFactory to SpanInfo Wire instrumentation library info through SpanFactory to SpanInfo Oct 23, 2020
@HaloFour HaloFour removed the WIP Work-in-Progress label Oct 23, 2020
@HaloFour
Copy link
Collaborator Author

Taking WIP off of this PR, I think this approach works and has relatively limited impact on the APIs.

TL;DR: The instrumentation library is exposed from SpanInfo but there is nothing in the API that specifies how it gets there. The CoreSpanFactory defaults it to the name/version of Money, but MoneyTraceProvider can create new instances of Tracer and copy the CoreSpanFactory from the existing singleton and apply different instrumentation library.

Copy link
Collaborator

@ptravers ptravers left a comment

Choose a reason for hiding this comment

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

not really related but we need to bump the year in the headers

@HaloFour HaloFour merged commit c0fef00 into Comcast:master Oct 23, 2020
@HaloFour HaloFour deleted the otel-library-info branch October 23, 2020 17:08
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