Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

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

Open
mabn opened this issue May 11, 2017 · 3 comments
Open

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

mabn opened this issue May 11, 2017 · 3 comments

Comments

@mabn
Copy link
Contributor

mabn commented May 11, 2017

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.

@pavolloffay
Copy link
Member

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.

@mabn
Copy link
Contributor Author

mabn commented May 11, 2017

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)
    }
}

@bhs
Copy link
Contributor

bhs commented May 28, 2017

(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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants