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 to OpenTelemetry 0.10.0 #156

Merged
merged 22 commits into from
Nov 7, 2020
Merged

Conversation

HaloFour
Copy link
Collaborator

@HaloFour HaloFour commented Oct 24, 2020

Updates to using OpenTelemetry 0.10.0 libraries.

Summary of major changes:

  1. Packages renamed from io.opentelemetry.trace._ to io.opentelemetry.api.trace._
  2. StatusCanonicalCode renamed to StatusCode
  3. Many methods on Span to add attributes, events, etc., changed to return the instance of the Span instead of void/Unit
    • Because of this I updated the Money Span interface to override all of these members and use covariant returns so that they return a Money Span instead of the OpenTelemetry Span.
  4. TextMapPropagator.Getter[T] now has two required members and is no longer valid as a functional interface in Java or as a SAM in Scala.
    • The second member is keys() which returns an Iterable[String] of header names. This forces the Formatter#fromHttpHeaders method to also be changed to accept an Iterable[String] of the header names.
  5. io.grpc.Context has been replaced with io.opentelemetry.context.Context
  6. Context APIs have been removed, e.g. Tracer#withSpan, TracingContextUtils
  7. Context APIs have been moved to Context, e.g. Context.with(span).makeCurrent()
    • Because of this there is no longer an API path to intercept when the current span is changed, so an SPI provider for ContextStorage is used to replace the underlying storage mechanism which can intercept when contexts are changed which detects the span in those contexts and updates MDC accordingly.

@HaloFour HaloFour added the WIP Work-in-Progress label Oct 24, 2020
@HaloFour
Copy link
Collaborator Author

The biggest change is to how span thread context is managed. The GRPC Context has been replaced with a built-in implementation and the static helper class has been replaced with a combination of static and instance methods. The trickiest aspect is that putting a span into the context involves calling an instance method on Span, which doesn't really work when Span is mocked.

The rest is mostly superficial.

# Conflicts:
#	money-core/src/test/scala/com/comcast/money/core/TracerSpec.scala
#	money-core/src/test/scala/com/comcast/money/core/formatters/FormatterPropagatorSpec.scala
@codecov-io
Copy link

codecov-io commented Oct 26, 2020

Codecov Report

Merging #156 into master will increase coverage by 0.53%.
The diff coverage is 90.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   92.88%   93.42%   +0.53%     
==========================================
  Files          87       89       +2     
  Lines        1167     1171       +4     
  Branches       71       79       +8     
==========================================
+ Hits         1084     1094      +10     
+ Misses         83       77       -6     
Impacted Files Coverage Δ
.../com/comcast/money/akka/SpanContextWithStack.scala 64.70% <ø> (ø)
...main/scala/com/comcast/money/core/Formatters.scala 40.00% <0.00%> (ø)
...a/com/comcast/money/core/MoneyTracerProvider.scala 87.50% <ø> (ø)
...rc/main/scala/com/comcast/money/core/Tracers.scala 30.00% <0.00%> (ø)
.../scala/com/comcast/money/core/UnrecordedSpan.scala 14.28% <0.00%> (ø)
...omcast/money/core/formatters/EgressFormatter.scala 0.00% <0.00%> (ø)
.../com/comcast/money/core/formatters/Formatter.scala 100.00% <ø> (ø)
...mcast/money/core/formatters/IngressFormatter.scala 0.00% <0.00%> (ø)
...st/money/core/formatters/MoneyTraceFormatter.scala 100.00% <ø> (ø)
.../money/core/formatters/TraceContextFormatter.scala 100.00% <ø> (ø)
... and 33 more

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 ea661f3...f0072c3. Read the comment docs.

@ptravers
Copy link
Collaborator

Ooof thanks for doing this!

@HaloFour
Copy link
Collaborator Author

Of course they decided to change the packages for everything, so the PR looks huge now. 😄

# Conflicts:
#	money-api/src/main/java/com/comcast/money/api/Span.java
#	money-core/src/main/scala/com/comcast/money/core/CoreSpan.scala
#	money-core/src/main/scala/com/comcast/money/core/CoreSpanBuilder.scala
#	money-core/src/main/scala/com/comcast/money/core/CoreSpanFactory.scala
#	money-core/src/main/scala/com/comcast/money/core/Disabled.scala
#	money-core/src/main/scala/com/comcast/money/core/Tracer.scala
#	money-core/src/main/scala/com/comcast/money/core/UnrecordedSpan.scala
#	money-core/src/test/scala/com/comcast/money/core/CoreSpanBuilderSpec.scala
#	money-core/src/test/scala/com/comcast/money/core/TracerSpec.scala
# Conflicts:
#	money-api/src/main/java/com/comcast/money/api/SpanInfo.java
#	money-core/src/main/scala/com/comcast/money/core/CoreEvent.scala
#	money-core/src/main/scala/com/comcast/money/core/CoreSpan.scala
#	money-core/src/main/scala/com/comcast/money/core/CoreSpanBuilder.scala
#	money-core/src/main/scala/com/comcast/money/core/CoreSpanInfo.scala
#	money-core/src/test/scala/com/comcast/money/core/CoreSpanBuilderSpec.scala
#	money-otel-handler/src/main/scala/com/comcast/money/otel/handlers/MoneyEvent.scala
#	money-otel-handler/src/main/scala/com/comcast/money/otel/handlers/MoneyLink.scala
#	money-otel-handler/src/main/scala/com/comcast/money/otel/handlers/MoneyReadableSpanData.scala
#	money-otel-handler/src/test/scala/com/comcast/money/otel/handlers/MoneyEventSpec.scala
#	money-otel-handler/src/test/scala/com/comcast/money/otel/handlers/MoneyReadableSpanDataSpec.scala
@HaloFour HaloFour changed the title WIP: Update to OpenTelemetry 0.10.0-SNAPSHOT Update to OpenTelemetry 0.10.0 Nov 5, 2020
@HaloFour HaloFour removed the WIP Work-in-Progress label Nov 5, 2020
@HaloFour
Copy link
Collaborator Author

HaloFour commented Nov 5, 2020

I'm taking the WIP label off. I've adopted the late-breaking changes and switched the dependencies to the release build (and removing the snapshot repository).

@HaloFour HaloFour merged commit 1ffedb4 into Comcast:master Nov 7, 2020
@HaloFour HaloFour deleted the update-otel-0.10 branch November 7, 2020 01: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.

3 participants