Make Tracer extends Closable. #329

carlosalberto · 2019-02-08T14:55:16Z

Prior to 0.32, I decided to take a shot at this feature (as part of opentracing/specification#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 · 2019-02-08T15:00:24Z

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

jpkrohling · 2019-02-08T15:01:06Z

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

MikeGoldsmith

This makes a lot of sense to me, and is equivalent to using the IDisposable pattern in C#.

sjoerdtalsma

Overall I have no problem with making Tracer Closeable, but what problem do we solve to have Tracer closeable?

Normally the one configuring the tracer in the application already knows exactly which implementation is instantiated and what its livecycle requirements are.

sjoerdtalsma · 2019-02-08T16:35:34Z

opentracing-util/src/main/java/io/opentracing/util/GlobalTracer.java

@@ -22,6 +22,7 @@
 import io.opentracing.noop.NoopTracerFactory;
 import io.opentracing.propagation.Format;

+import java.io.IOException;


Why the import of IOException?
I don't see it being thrown?

Oh, good catch. I forgot to remove it during my cleanup.

Will you remove the import before merging?

carlosalberto · 2019-02-08T16:52:33Z

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 (#250 (comment) and #302 (comment))

Hope that helps ;)

sjoerdtalsma · 2019-02-10T23:43:46Z

@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 · 2019-02-11T16:01:32Z

@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 · 2019-02-11T16:17:24Z

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 · 2019-02-11T16:44:39Z

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 · 2019-02-11T16:53:32Z

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 · 2019-02-11T16:56:33Z

@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 · 2019-02-11T17:31:24Z

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 · 2019-02-11T19:07:16Z

@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.. 😉

austinlparker · 2019-02-11T21:33:47Z

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 · 2019-02-14T01:46:25Z

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 · 2019-02-14T03:31:30Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+     *
+     * <p>
+     * The close method should be considered idempotent; closing an already closed Tracer should not raise an error.
+     * Spans that are created or finished after a Tracer has been closed may or may not be flushed.


this text implies that while spans may not be flushed, the tracer must still create them like business as usual, right?

Yes, I'd say so (as mentioned in the spec RFC) - an alternative is to NOT allow Span objects to be created (from where? from the builder? from start?)

yurishkuro · 2019-02-14T03:33:12Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+     * <p>
+     * The close method should be considered idempotent; closing an already closed Tracer should not raise an error.
+     * Spans that are created or finished after a Tracer has been closed may or may not be flushed.
+     * Calling the close method should be considered a synchronous operation.


What should happen if tracer uses sync span export (e.g. via HTTP), is able to retry failures, and the tracing backend is down? This method theoretically can block for a long time?

Oh, that's a good point. Retries are fine, I think - but that brings us to the timeout point, and whether we should tell the user it can block for long or it should return relatively soon.

Opinions on this?

(I think every Tracer should try to flush for some given time -implementation specific- and then simply give up).

yurishkuro · 2019-02-14T03:35:48Z

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 · 2019-02-14T18:07:50Z

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 · 2019-02-14T18:32:13Z

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 · 2019-02-15T14:27:46Z

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 · 2019-02-15T14:36:51Z

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

austinlparker · 2019-02-19T19:57:49Z

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 · 2019-02-20T09:36:28Z

+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 · 2019-02-20T16:43:37Z

@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 · 2019-02-20T16:51:12Z

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 · 2019-02-20T17:07:14Z

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 · 2019-02-20T17:30:03Z

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.

yurishkuro

ok, let's do it

austinlparker · 2019-02-20T17:44:10Z

🎉

carlosalberto · 2019-02-22T17:26:50Z

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

t-8ch · 2019-02-22T23:46:17Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+     * <p>
+     * For stateless tracers, this can be a no-op.
+     */
+    void close();


Should this @Override?

carlosalberto · 2019-03-01T03:40:52Z

Merged it, thank you all for the feedback!

* Deprecate the StringTag.set() overload taking a StringTag. (#262) * Implement Trace Identifiers. (#280) * Bump JaCoCo to use a Java 10 friendly version (#306) * Remove finish span on close (#301) * Deprecate finishSpanOnClose on activation. * Add ScopeManager.activeSpan() and Tracer.activateSpan(). * Clarify the API changes and deprecations. * Add an error reporting sample to opentracing-testbed. * Simple layer on top of ByteBuffer for BINARY format. (#276) * Add generic typed setTag/withTag (#311) * Allow injecting into maps of type Map<String,Object> (#310) * Add simple registerIfAbsent to global tracer (#289) * Split Inject and Extract Builtin interfaces (#316) * Deprecate ScopeManager.active() (#326) * Make Tracer extends Closable. (#329) * Do not make isRegistered() synchronized. (#333) * Deprecate AutoFinishScopeManager (#335)

Make Tracer extends Closeable.

bc09a25

MikeGoldsmith approved these changes Feb 8, 2019

View reviewed changes

jpkrohling approved these changes Feb 8, 2019

View reviewed changes

tylerbenson approved these changes Feb 8, 2019

View reviewed changes

sjoerdtalsma reviewed Feb 8, 2019

View reviewed changes

austinlparker mentioned this pull request Feb 11, 2019

RFC: Closeable Tracer opentracing/specification#130

Open

yurishkuro reviewed Feb 14, 2019

View reviewed changes

yurishkuro approved these changes Feb 20, 2019

View reviewed changes

carlosalberto added 2 commits February 22, 2019 15:53

Remove unused imports.

d0b880f

Add a small clarification on close() expectations.

175b6b8

t-8ch reviewed Feb 22, 2019

View reviewed changes

Add a @OverRide to Tracer.close()

5f1d881

carlosalberto changed the title ~~Make Tracer extends Closeable.~~ Make Tracer extends Closable. Mar 1, 2019

carlosalberto merged commit 8176208 into opentracing:v0.32.0 Mar 1, 2019

carlosalberto deleted the tracer-closeable branch March 7, 2019 01:33

Make Tracer extends Closable. #329

Make Tracer extends Closable. #329

Conversation

carlosalberto commented Feb 8, 2019

coveralls commented Feb 8, 2019 • edited Loading

jpkrohling commented Feb 8, 2019

MikeGoldsmith left a comment

Choose a reason for hiding this comment

sjoerdtalsma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Feb 8, 2019

sjoerdtalsma commented Feb 10, 2019

tylerbenson commented Feb 11, 2019

jpkrohling commented Feb 11, 2019

yurishkuro commented Feb 11, 2019

jpkrohling commented Feb 11, 2019

austinlparker commented Feb 11, 2019 • edited Loading

carlosalberto commented Feb 11, 2019

sjoerdtalsma commented Feb 11, 2019

austinlparker commented Feb 11, 2019

carlosalberto commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 14, 2019

carlosalberto commented Feb 14, 2019

yurishkuro commented Feb 14, 2019

carlosalberto commented Feb 15, 2019

yurishkuro commented Feb 15, 2019

austinlparker commented Feb 19, 2019

jpkrohling commented Feb 20, 2019

yurishkuro commented Feb 20, 2019

jpkrohling commented Feb 20, 2019

yurishkuro commented Feb 20, 2019

jpkrohling commented Feb 20, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

austinlparker commented Feb 20, 2019

carlosalberto commented Feb 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Mar 1, 2019

coveralls commented Feb 8, 2019 •

edited

Loading

austinlparker commented Feb 11, 2019 •

edited

Loading