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

Adds project to wrap OTel SpanProcessor/SpanExporter as Money SpanHandler #147

Merged
merged 18 commits into from Oct 21, 2020

Conversation

HaloFour
Copy link
Collaborator

Enables wrapping an OTel SpanProcessor/SpanExporter as a Money SpanHandler so that Money can report spans directly to OpenTelemetry-compatible exporters, like ZipKin and Jaeger.

@HaloFour
Copy link
Collaborator Author

Working out the general shape of the API and then I plan on adding modules that wrap the exporters for Zipkin and Jaeger that can pull their configuration from TypeSafe.

@codecov-io
Copy link

codecov-io commented Oct 17, 2020

Codecov Report

Merging #147 into master will increase coverage by 4.54%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   89.28%   93.82%   +4.54%     
==========================================
  Files          55       59       +4     
  Lines        1008     1053      +45     
  Branches       62       75      +13     
==========================================
+ Hits          900      988      +88     
+ Misses        108       65      -43     
Impacted Files Coverage Δ
...a/com/comcast/money/otel/handlers/MoneyEvent.scala 100.00% <100.00%> (ø)
...st/money/otel/handlers/MoneyReadableSpanData.scala 100.00% <100.00%> (ø)
.../comcast/money/otel/handlers/OtelSpanHandler.scala 100.00% <100.00%> (ø)
...y/otel/handlers/jaeger/JaegerOtelSpanHandler.scala 100.00% <100.00%> (ø)
...y/otel/handlers/zipkin/ZipkinOtelSpanHandler.scala 100.00% <100.00%> (ø)

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 016806e...498db0c. Read the comment docs.

@HaloFour
Copy link
Collaborator Author

woot, codecov report!

I'm going to take the WIP off of this PR. I still have docs to add but I think functionally it's finished.

@HaloFour HaloFour changed the title WIP: Adds project to wrap OTel SpanProcessor/SpanExporter as Money SpanHandler Adds project to wrap OTel SpanProcessor/SpanExporter as Money SpanHandler Oct 17, 2020
@HaloFour HaloFour removed the WIP Work-in-Progress label Oct 17, 2020
@HaloFour
Copy link
Collaborator Author

Added Scaladocs with sample config. Also added an integration test for Zipkin. Writing one for Jaeger would require using test containers which would be possible but a bit more involved.

PowerMockito.when(BatchSpanProcessor.newBuilder(spanExporter)).thenReturn(batchSpanProcessorBuilder);
PowerMockito.when(SimpleSpanProcessor.newBuilder(spanExporter)).thenReturn(simpleSpanProcessorBuilder);
PowerMockito.when(batchSpanProcessorBuilder.build()).thenReturn(batchSpanProcessor);
PowerMockito.when(simpleSpanProcessorBuilder.build()).thenReturn(simpleSpanProcessor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can power mock give false positives in your experience?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't had any serious problems with it. I hate reaching for it when it comes to Unit testing but a lot of the OTel APIs aren't mock-friendly, these builders are all final classes.

@ptravers
Copy link
Collaborator

ptravers commented Oct 21, 2020

youve got conflicts

…cessor

# Conflicts:
#	money-api/src/main/java/com/comcast/money/api/SpanId.java
@HaloFour HaloFour merged commit 14ff7a9 into Comcast:master Oct 21, 2020
@HaloFour HaloFour deleted the money-otel-processor branch October 21, 2020 17:19
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.

None yet

3 participants