opentracing-java icon indicating copy to clipboard operation
opentracing-java copied to clipboard

Make GlobalTracer easier to use with java 8 (depends on #115)

Open mabn opened this issue 8 years ago • 3 comments

In #115 GlobalTracer gets a new method:

* @return the {@link ActiveSpan active span}, or null if none could be found.
*/
ActiveSpan activeSpan();

One of the common uses of this method will be adding tags/logs to the active span if present, like this:

if(GlobalTracer.activeSpan() != null) {
   GlobalTracer.activeSpan().setTag("tagged", true);
} 

It's tempting to declare the return type as Optional and instead be able to do this:

GlobalTracer.activeSpan().ifPresent(span -> {
    span.setTag("tagged", true)
});

But I guess we can't use java 8 in this project. So instead I propose one of two options:

  1. Add a method taking a Runnable-like interface (something which allows throwing exceptions maybe?) and make this possible:
    GlobalTracer.withActiveSpan(span -> span.setTag("tagged", true))
    
    I'm not sure about the naming
  2. Implement simplified Optional<ActiveSpan> and return that. activeSpan() comes from ActiveSpanSource and I'm not sure if it should be touched, so it would need to be returned from a new method.

I'd go with option 1.

mabn avatar May 11 '17 09:05 mabn

Optional:

GlobalTracer.activeSpan().ifPresent(span -> {
    span.setTag("tagged", true)
});

and

GlobalTracer.withActiveSpan(span -> span.setTag("tagged", true))

are not the same, null check is still required. I don't see any benefits there.

pavolloffay avatar May 11 '17 16:05 pavolloffay

The intention was to make withActiveSpan have the same semantic - it would basically do the null check. Implementation would be something similar to:

public void withActiveSpan(Consumer<Span> consumer) {
    if(activeSpan() != null) {
        consumer.accept(span)
    }
}

mabn avatar May 11 '17 17:05 mabn

(my recent comment from #134 applies here as well: basically, that this API seems like the right way to do what's being proposed, but that I am hesitant to add API surface area for convenience purposes until we have more experience with ActiveSpan)

bhs avatar May 28 '17 23:05 bhs