Tracer should be Closeable

[fzakaria]

I would like to guarantee that any Reporter's close at the end of my program (in the case they are batching). Tracer should implement Closeable similar to Jaeger's implementation.

[wu-sheng]

Why a tracer need to close? Tracer should have a context about tracing. Batch module tracing is not related to Tracer, I think.

[fzakaria]

I wrote it in the description. The use case is to shutdown reporters that may start threads or have file handlers.

[wu-sheng]

You are talking about the implementation. How does OT-java API library user know when to close and start a new one? And what happens when users close a tracer and still have Trace-Context unclosed?

Even more, in Java, many tracing (like Apache SkyWalking, and more) use ThreadLocal to manage Tracing Context. You can't clear the context just close a tracer.

In my perspective, If you are facing a batch process, and implementation supports file handler, you should start a Tracer for batch/file at the beginning. Or keep the switch mechanism in implementation level.

[yurishkuro]

The question of close() method has been raised in other languages as well, but we ended up with the same decisions:

• for some tracer implementations close() method doesn't make sense, e.g. a Noop tracer or a wrapper tracer
• in most cases the main program is aware which concrete tracer, rather than an abstract io.opentracing.Tracer, it is instantiating , so the close() method would be available on the concrete instance. However, this doesn't help if the app is using dynamic loading that Java supports.
• the application can always do a conditional cast to the Closeable interface, so it's not necessary to pull it into the Tracer interface

[carlosalberto]

@yurishkuro nice summary :) Sounds like we should put that in our FAQ (once we create one, maybe in the specification repository ;) )?

[carlosalberto]

Trying to retake this item @yurishkuro - think we should create a FAQ file or section in the README for cases like this?

(I personally think this is not something that could be included in the Specification, as it's more a Language thing ;) )

[yurishkuro]

+1 to add to the readme/faq

[carlosalberto]

Hey @wu-sheng

So it looks the way to proceed here is to create a FAQ file - starting with an explanation on this item (and why we don't implement the Closeable interface). Sounds fine?

[pavolloffay]

I have some contra arguments on @yurishkuro comments.

for some tracer implementations close() method doesn't make sense, e.g. a Noop tracer or a wrapper tracer

I would not be concerned about this type as it is just noop. Any real tracer usually needs to free up resources at the end.

However, this doesn't help if the app is using dynamic loading that Java supports.

It's indeed helpful for this use cases and casting to https://docs.oracle.com/javase/7/docs/api/java/io/Closeable.html is not java idiomatic (like it might be in go).

[jpkrohling]

Why a tracer need to close?

Tracers can benefit from knowing when the application is shutting down or being undeployed. I can give you two concrete examples where we had to deal with a #close() method in Jaeger in the past couple of weeks :-)

There are stateless tracers out there that don't need to be closed. For those, a noop #close() would be sufficient. Most JVMs will just inline this method call, so that no performance impact exist.

Noop tracer or a wrapper tracer

NoopTracer could just have a noop #close() method. Wrapper tracer needs a close method if the wrapped tracer needs to be closed.

All in all, it looks like all real tracers need to be closed, but we are saying "no" to this feature because of some of the exceptions. And those exception can easily just implement a noop method...

the application can always do a conditional cast to the Closeable interface, so it's not necessary to pull it into the Tracer interface

That's what we have to end up recommending to application developers. But seriously, this is more like a hack than a good advice.

[yurishkuro]

As I mentioned on #302, I'm +1 to add close().

[safris]

Hi all, I mistakenly created a duplicate issue for this here: #302. I'll repeat my reasons for having Tracer extend Closeable:

The Tracer interface does not define a close() method. Implementors of the Tracer interface, however, most often implement their own close() method to finalize traces, and to release resources. Since io.opentracing.Tracer#close() is not defined, application code that utilizes Tracer(s) must import the vendor-specific implementation in order for the close() method to be available. This effectively makes the application code coupled to the vendor, since the vendor-specific import is required.

By declaring a close() method in the io.opentracing.Tracer interface, vendor coupling can be avoided.

Originally, I thought opentracing-api was on jdk1.7, but it is on jdk1.6. If it was on 1.7, the close() method could be declared with a noop default, thus allowing for backward compatibility with all implementors of io.opentracing.Tracer that do not implement a close() method. However, since this is not possible in jdk1.6, a pure interface method would have to be declared.

If Tracer remains as-is, without close(), a workaround that avoids the vendor-specific import is to explicitly cast the Tracer to Closeable. This, however, breaks cohesion principles since it is not guaranteed that the vendor implementation of Tracer actually implements Closeable.

[austinlparker]

+1 to this as well. It seems to me that any practical usage of a tracer in a language that has Close/Dispose patterns is going to effectively require a close() method to avoid tight vendor coupling.

[pavolloffay]

I am +1, It would be great to get it done in the next release.

[austinlparker]

I do think this should probably be an RFC on the specification. C# at least has a similar use-with-resource pattern, and since this can be a noop in languages where it doesn't make sense, it's enough of a cross-language concern to apply across the board.

[austinlparker]

RFC is up, would love if people could take a look and make suggestions/catch things I've missed. https://github.com/opentracing/specification/pull/128