Make Tracer extends Closable.

[carlosalberto]

Prior to 0.32, I decided to take a shot at this feature (as part of https://github.com/opentracing/specification/issues/130) - the change itself is actually minimal, and I'm guessing that this could be an easy-to-implement item for Tracer maintainers, but would definitely love some feedback.

Depending on how good or bad the changes are deemed, we might include this in the 0.32 Final Release ;)

PS - Was tempted to leave MockTracer.close() as no-op, but preferred to keep the invariant.

@yurishkuro @tylerbenson @austinlparker @opentracing/opentracing-java-maintainers

[coveralls]

Coverage increased (+0.2%) to 77.301% when pulling 5f1d881383f493e947565836c0da73785bf27546 on carlosalberto:tracer-closeable into 748a229c0c051111b47eba33a2e10307e2e601f2 on opentracing:v0.32.0.

[jpkrohling]

I'm +1 to this change, but haven't we had this discussion before?

[carlosalberto]

Hey @sjoerdtalsma

So, from the mentioned RFC:

In lieu of having to perform potentially unsafe casts of a Tracer to access it's implementation-specific Close method, the interface would ensure that it was available regardless.

Also, there was some previous feedback (https://github.com/opentracing/opentracing-java/issues/250#issuecomment-420210277 and https://github.com/opentracing/opentracing-java/issues/302#issuecomment-419909901)

Hope that helps ;)

[sjoerdtalsma]

@carlosalberto I have read those arguments and there seems to be a genuine need for this. However, I'm more curious than anything else about a use-case where one is in control of the Tracer's lifecycle (because we need close) but not knowing which implementation he/she created earlier in the lifecycle.

Don't let my curiosity get in the way of merging by the way 😉 ..

[tylerbenson]

@sjoerdtalsma perhaps a good example is around the usage of https://github.com/opentracing-contrib/java-tracerresolver, where the user only adds it to their classpath, and does not reference their desired tracer class directly.

[jpkrohling]

However, I'm more curious than anything else about a use-case where one is in control of the Tracer's lifecycle (because we need close) but not knowing which implementation he/she created earlier in the lifecycle

I think I might have mentioned before, but one of the concrete cases where I needed this was when implementing the tracing subsystem for WildFly. Basically, the application server will create a tracer based on the tracer implementation from the application's classpath (possibly packaged inside the application's WAR) and start/close it whenever the deployment has started/finished.

[yurishkuro]

My only concern with Close() API is that it requires synchronous, uninterruptible behavior. I don't know why, but Lightstep's tracer for Go implements cancellable Close():

https://github.com/lightstep/lightstep-tracer-go/blob/9a9bdeed74406bebe00f3875387e5ef1e2364c9e/tracer.go#L190

In Jaeger we never had requests to support that. Our Node.js implementation of Close() is asynchronous, but that's more of a side effect of Node.js style.

So I am interested in hearing from existing implementations:

• do you need Close() to be async (i.e. return a future)?
• do you need Close() to be cancellable?

If yes on either question, would be good to hear & document the use cases.

[jpkrohling]

• do you need Close() to be async (i.e. return a future)?
• do you need Close() to be cancellable?

For Java, catching an InterruptedException is typically sufficient to handle the case where the close has to be canceled.

IMO, having the Tracer extend Closeable has a higher value than providing a non-standard async close method, or a close method with a context (or similar).

[austinlparker]

@yurishkuro I don't think close is cancellable in go - the context object is only used in the flush so we can ensure there's nothing in the span buffer before closing the tracer. Not really sure about the rationale for that implementation other than making it easier to close + flush in one instruction rather than two.

edit - actually, now that I look at it again it does seem weird because of the return on line 180. edit x2 - I think there's actually a bug here, because if you did cancel it then we'd never actually call connection.Close and you could never call it again on that tracer instance...

[carlosalberto]

Hey @yurishkuro @jpkrohling

IMO, having the Tracer extend Closeable has a higher value than providing a non-standard async close method, or a close method with a context (or similar).

I strongly agree with this. But I'm still curious on this detail, and would love so hear other points of view.

[sjoerdtalsma]

@tylerbenson @jpkrohling Thanks for the use cases. With that I'm totally on board with this PR.

I should've guessed the tracer resolver example though.. :wink:

[austinlparker]

Apparently the rationale behind the Golang Close method being cancellable is to prevent process shutdown being blocked by the tracer, so that a shutdown circuit breaker implemented on the context can kill the Close method. The more you know!

[carlosalberto]

Hey hey @yurishkuro wondering if this makes sense to you? If you think we need more feedback, we could poke people a little bit more ;)

cc @codingfabian @felixbarny

[yurishkuro]

I am being a bit pedantic, but the fact that Lightstep had run into this situation indicates that the use case of tracer.close() blocking is not completely theoretical, that's why I want the semantics of that method to be defined more carefully.

[carlosalberto]

I am being a bit pedantic, but the fact that Lightstep had run into this situation indicates that the use case of tracer.close() blocking is not completely theoretical, that's why I want the semantics of that method to be defined more carefully.

I think this is fine. We can iterate a little bit more to have better semantics ;) (worst case: if it takes too long, we can include it in a further release).

I'd love to get feedback on this

cc @austinlparker

[yurishkuro]

every Tracer should try to flush for some given time -implementation specific- and then simply give up

That's reasonable, but if the API was cognizant of that use case it wouldn't have to be implementation specific. I assume the option here is to return CancellableFuture, which would make the tracer not io.Closable.

[carlosalberto]

That's reasonable, but if the API was cognizant of that use case it wouldn't have to be implementation specific.

Agreed.

I assume the option here is to return CancellableFuture, which would make the tracer not io.Closable.

Also correct. But we could offer this also in the future, for 'advanced' scenarios ;)

What about saying something along "this is a synchronous call, will try to flush pending Spans and shutdown related resources. Be aware of the fact it may ultimately block for long". This is perhaps too general but would help give better expectations to users.

[yurishkuro]

Is the weak guarantee, especially of freeing resources and returning, actually going to be sufficient for the people who need this functionality?

[austinlparker]

The people that have asked for the feature, iirc, haven't mentioned anything about wanting this to be cancellable, but have mentioned try-with-resources, so I think the current evidence tips towards implementing closeable rather than cancellable.

[jpkrohling]

+1, making the Tracer implement Closeable has real benefits, such as being able to use with closeable streams and try-with-resources. Tracers aren't the only components that might depend on IO resources upon close, and the typical behavior is to properly handle signals like thread interruption when it has to exit right away.

[yurishkuro]

@jpkrohling I don't follow your point about interruption. Typically this is used when you have work done in thread A and another thread B is waiting for that work to complete (e.g. B waits on future.get()). If someone interrupts A, then B gets the interrupted exception. In case of close() method, there's only single thread (most likely main), so who would/could interrupt the close() operation?

[jpkrohling]

I expect blocking operations to not run in the main thread, so, Tracer#close should always run in a worker thread. Otherwise, you are right, but that's an application-level concern that applies to all other #close operations they might have (HTTP connections, file system operations, ...)

[yurishkuro]

I expect blocking operations to not run in the main thread, so, Tracer#close should always run in a worker thread

Well, then to interrupt it you need another API on the tracer, and there's still nobody to actually do the interrupt because main thread is blocked.

Anyway, we're getting into the weeds. My main concern is that we're saying that tracer does not need to provide any guarantee how long it will take to return from close(), and to avoid blocking indefinitely we can only suggest that implementations support some sort of internal configurable timeout. Returning a future that can be cancelled or waited on with a user-controlled timeout avoids this problem completely in a very clean way. It does not make the tracer Closable.

[jpkrohling]

I understand the appeal, but I'm not aware of any standard APIs in Java for that. I'm afraid we'd be building something that deviates from what pretty much everything out there uses.

If we really have a need for the feature you described, we could consider adding an overloaded method, #close(Context) (or #close(int)) with a deadline/timeout. Until then, it's more prudent to keep it simple and consistent with what people are used to.

[austinlparker]

🎉

[carlosalberto]

Hey all - I've removed a pair of unused imports, and added a pair of minor notes on the close() behavior.

The plan is to merge it late Monday (start preparing the 0.32 release), btw ;)

[carlosalberto]

Merged it, thank you all for the feedback!