opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Add reset to GlobalTracer #288

Open pavolloffay opened 6 years ago

pavolloffay commented 6 years ago

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

sjoerdtalsma commented 6 years ago

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.

jpkrohling commented 6 years ago

Can this one be closed?

bobmcwhirter commented 6 years ago

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.

jpkrohling commented 6 years ago

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;
    }
}
yurishkuro commented 6 years ago

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.

sjoerdtalsma commented 6 years ago

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.

carlosalberto commented 5 years ago

Hey @pavolloffay

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