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

Add reset to GlobalTracer

Open pavolloffay opened this issue 6 years ago • 7 comments

Related to https://github.com/opentracing/opentracing-java/issues/170.

At the moment there is no reset method on GlobaLTracer which makes testing hard. The solution might be to fork jvm or use test-jar. However most people don't know that there is a test-jar. The test jar is also not a preferred way to provide these type of functionality.

If the reset API is well documented I don't think it will be misused.

cc @bobmcwhirter

pavolloffay avatar Jul 13 '18 09:07 pavolloffay

The original proposal had a reset functionality and the consensus then was that having at-most-once semantics for the GlobalTracer were desirable.

Adding a reset can cause issues in an application by addition of a new library whose author misunderstood or didn't read the documentation. These kind of bugs are not my favorite to solve!

My opinion: Adding functionality to production code to facilitate testing is bad practice and should be avoided.

sjoerdtalsma avatar Jul 21 '18 08:07 sjoerdtalsma

Can this one be closed?

jpkrohling avatar Aug 14 '18 14:08 jpkrohling

The request originate in a use-case for running multiple tests within a single JVM, where each test attempts to crank up the whole system, including setting the global tracer, and then tearing it down.

bobmcwhirter avatar Aug 14 '18 14:08 bobmcwhirter

Do you need to have each test to use a different tracer instance? If you need to replace the tracer instance, wouldn't be more suitable to create a wrapper like this and register that with the GlobalTracer?

public class MutableTracer implements Tracer {
    private Tracer delegate;

    public MutableTracer(Tracer delegate) {
        this.delegate = delegate;
    }

    @Override
    public ScopeManager scopeManager() {
        return delegate.scopeManager();
    }

   ... // other delegated methods

    public void replace(Tracer tracer) {
        this.delegate = tracer;
    }
}

jpkrohling avatar Aug 14 '18 14:08 jpkrohling

I think a better suggestion would be to not design your under test code to depend on global tracer. Globals and unit tests don't go together well. For example, if you're creating some middleware, always make it accept the tracer rather than depending on global tracer directly.

yurishkuro avatar Aug 14 '18 15:08 yurishkuro

I agree with @yurishkuro. The suggestion of @jpkrohling sounds simple enough as a workaround. And you could further always use the test-jar dependency of opentracing-util as a last resort.

sjoerdtalsma avatar Aug 14 '18 18:08 sjoerdtalsma

Hey @pavolloffay

Wondering about your opinion on this - it feels like there's agreement to not add this method. Let us know ;)

carlosalberto avatar Dec 07 '18 06:12 carlosalberto