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

handle noop correctly #4968

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Conversation

mattspataro
Copy link
Contributor

@mattspataro mattspataro commented Nov 18, 2022

We recently migrated our codebase from OpenTracing to OpenTelemetry and were delighted to take advantage of the OpenTracingShim. Unfortunately we ran into issues with noop spans.

There are a few instances in which we register new spans as children of a io.opentracing.noop.NoopSpan. Unfortunately, when this happens, the OpenTelemetryShim attempts to turn the parent NoopSpan into a SpanShim, which throws an exception. This issue can be mitigated by performing a noop if we attempt to register a NoopSpan as a parent. It appears that this was meant to be handled anyways, judging from the following TODO comment that already existed in the codebase:
// TODO - Verify we handle a no-op Span

This PR fixes this issue and provides a unit test to verify that it works correctly.

@mattspataro mattspataro requested a review from a team November 18, 2022 02:18
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 18, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: mattspataro / name: Matt Spataro (9529a74)

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Base: 91.26% // Head: 91.26% // No change to project coverage 👍

Coverage data is based on head (ff4338c) compared to base (ac2ba4a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #4968   +/-   ##
=========================================
  Coverage     91.26%   91.26%           
- Complexity     4885     4886    +1     
=========================================
  Files           552      552           
  Lines         14431    14431           
  Branches       1373     1373           
=========================================
  Hits          13170    13170           
  Misses          874      874           
  Partials        387      387           
Impacted Files Coverage Δ
...opentelemetry/opentracingshim/SpanBuilderShim.java 93.79% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jkwatson
Copy link
Contributor

@carlosalberto can you take a look at this one?

opentracing-shim/build.gradle.kts Outdated Show resolved Hide resolved
opentracing-shim/build.gradle.kts Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

So this is an undocumented/corner-case of the OpenTracing API - for safety reasons, though, I'm happy to accept it - (I will add a note in the Spec's section for this, so other languages support this scenario as well).

(OpenTracing's No-op Span is not quite the same as our OpenTelemetry's Span.getInvalid())

@mattspataro
Copy link
Contributor Author

mattspataro commented Nov 23, 2022

@carlosalberto

So this is an undocumented/corner-case of the OpenTracing API - for safety reasons, though, I'm happy to accept it - (I will add a note in the Spec's section for this, so other languages support this scenario as well).

(OpenTracing's No-op Span is not quite the same as our OpenTelemetry's Span.getInvalid())

I see you what you mean. I realized later that the java doc for io.opentracing.Tracer.SpanBuilder.asChildOf says: "If parent==null, this is a noop." So our code-base should've been passing in null for noop spans anyways. But still, I think this could be helpful for those that make the same mistake as us and accidentally pass in a noop span.

@jack-berg jack-berg merged commit 568bdb4 into open-telemetry:main Nov 29, 2022
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants