Move static access of current span to Span.

[anuraaga]

I was thinking of talking about this after changes like #1753 and #1737 (after which I'm thinking of moving context interaction to OpenTelemetry) but figured it doesn't hurt to split this out as an idea.

I think getting the current span is a very common operation, especially for javaagent users. They will rarely create spans, or deal with tracers. But adding attributes to a span is very common for anyone.

Response myRequestHandler(Request request) {
  Span.current().setAttribute("userid", request.getUserId());
}

We are also toying with the idea of having static methods on Tracer

Response myRequestHandler(Request request) {
  Tracer.currentSpan().setAttribute("userid", request.getUserId());
}

For simple usage, which is common for accessing the current span since when you have a tracer, you usually also will have created a span from it to track yourself, the former feels more intuitive and looks like better code from a user perspective to me. Tracer is a very important concept for instrumentation authors, but not so important for end users.

[codecov]

Codecov Report

Merging #1795 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #1795   +/-   ##
=========================================
  Coverage     85.53%   85.53%           
- Complexity     1374     1375    +1     
=========================================
  Files           166      166           
  Lines          5308     5309    +1     
  Branches        549      549           
=========================================
+ Hits           4540     4541    +1     
  Misses          569      569           
  Partials        199      199           

Impacted Files	Coverage Δ	Complexity Δ
...va/io/opentelemetry/trace/TracingContextUtils.java	100.00% <ø> (ø)	7.00 <0.00> (ø)
...ain/java/io/opentelemetry/trace/DefaultTracer.java	97.14% <100.00%> (ø)	5.00 <1.00> (ø)
api/src/main/java/io/opentelemetry/trace/Span.java	100.00% <100.00%> (ø)	2.00 <1.00> (+1.00)
...ain/java/io/opentelemetry/sdk/trace/TracerSdk.java	100.00% <100.00%> (ø)	6.00 <1.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 e539890...c837e22. Read the comment docs.

[iNikem]

It seems to contradict this spec issue

Remove all current public helpers from Tracer and Trace Context Utilities that interact with "current Context".
Things like Get the currently active span and Set the currently active span should be removed.

[Oberon00]

I think that is a spec wording issue and should be fixed there. IMHO the spec just wants these methods to not require a Span/Tracer instance as parameters (e.g. implicit this parameter)

[jkwatson]

odd fully-qualified class here. might cleaning it up while you're here?

[carlosalberto]

I think that is a spec wording issue and should be fixed there

No, we also want to reduce the places where we expose this functionality.

That being said, whether we keep this functionality (Tracer or TracingContextUtils), I'd personally be fine with Span.current() as a single shortcut (that is, no more shortcuts, at least for now). Else, maybe a reason to have Tracer.getCurrentSpan() as a static member.

[jkwatson]

I'm definitely not a fan of TCU, so this seems like a good change to me. Would love @carlosalberto to comment, as well.

[Oberon00]

@carlosalberto

No, we also want to reduce the places where we expose this functionality.

But this is not adding more places?

[carlosalberto]

But this is not adding more places?

Yes! And that's why I'd be fine with a single shortcut (or, well, none). The idea is that Spec change is to not have all duplicated (at this moment we have many operations all over the place).

[jkwatson]

We had some discussion about this during the SIG meeting this morning. The request from @bogdandrutu , which I think everyone agreed with, is possibly an end-state-describing issue that can track all the work on this cleanup, so we can have a good handle on where you/we are headed. For instance, we all like having Span.current(), but many of us also think that if we add it, we should remove at least one of the other public options for accessing the current span.

So, the ask is for a little bit of design work to describe where you see the end-state of the API (this might tie into the instance/global work, additionally, if you feel they are related).

[anuraaga]

Thanks, summarized in #1807

[carlosalberto]

Given that TracingContextUtils still is the root, I'd be happier with using that one instead (users and instrumentation are free to use whatever they want of course ;) )

[jkwatson]

this is gone from the Tracer now, so probably not relevant. :)

[carlosalberto]

Disagree - we want to use whatever the root is. Otherwise, let's simply move the logic to Span ;) (although the Spec may not allow that).

[jkwatson]

I'm just saying that this method is no longer on the Tracer on the main branch, so this comment can't apply without adding it back to the Tracer.

[carlosalberto]

Likewise.

[jkwatson]

same

[bogdandrutu]

Proposing for the moment to close this PR and update or create a new PR with the latest discussion from #1807

[carlosalberto]

@bogdandrutu Fine by me to close this PR while we discuss our options.