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

Return Span from DefaultSpan factories. #1519

Merged

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Aug 7, 2020

#1135 talks about hiding DefaultSpan completely. I couldn't quite find a good home for the factory methods (maybe DefaultTracer?) but it seems like a good idea.

In the meantime, it's more important to make the return type of the factories generic because users should definitely not have to care whether a span is a DefaultSpan or not. In practice, the return value of DefaultSpan makes it difficult to bridge them in auto instrumentation.

open-telemetry/opentelemetry-java-instrumentation#764

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1519 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1519   +/-   ##
=========================================
  Coverage     87.01%   87.01%           
  Complexity     1367     1367           
=========================================
  Files           162      162           
  Lines          5183     5183           
  Branches        490      490           
=========================================
  Hits           4510     4510           
  Misses          492      492           
  Partials        181      181           
Impacted Files Coverage Δ Complexity Δ
.../main/java/io/opentelemetry/trace/DefaultSpan.java 80.00% <ø> (ø) 18.00 <0.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 bcb1d4a...5529657. Read the comment docs.

@carlosalberto
Copy link
Contributor

LGTM. I've long wondered whether there's any reason to expose DefaultSpan publicly. @bogdandrutu may know better (although I can't think of one at the moment).

@bogdandrutu bogdandrutu merged commit c930e03 into open-telemetry:master Aug 10, 2020
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.

5 participants