search

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 `io.opentracing.Span.isFinished()` method #342

Open ravirajj opened 5 years ago

ravirajj commented 5 years ago

To implement the GRPC tracing client interceptor we need to be able to discern if the span has finished or not. Currently, a volatile variable external to the span is used to track this state. See https://github.com/opentracing-contrib/java-grpc/pull/36.

The client span needs to be finished in either the ClientCall.cancel() or ClientCall.Listener.onClose() method. These methods are the last to run in the outbound and inbound threads, respectively. Depending on whether the client call succeeds or fails, either or both methods may be executed in any order. See https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/ClientCall.java#L47-L50.

By using io.opentracing.Span.isFinished(), each method can check if the span has not already been closed by the other, thus ensuring that span.finish() is only run once.

yurishkuro commented 5 years ago

Currently, a volatile variable external to the span is used to track this state.

What's the problem with that?

Overall, I don't think is the best approach to keep changing OpenTracing API to compensate for the deficiencies of instrumented framework. What about asking gRPC to provide a callback method that is reliably called at the end of the operation such that you don't have to guess where to finish the span?

ravirajj commented 5 years ago

There is no problem using AtomicBoolean to save span finished state (See https://github.com/opentracing-contrib/java-grpc/pull/36). The Span.isFinished() API would be a convenience, but then again access to the finished state would need to be synchronized for this use case and that is OpenTracing API implementation dependent.

I have also filed a bug with grpc-java to ensure the onClose() is always called as it is expected: https://github.com/grpc/grpc-java/issues/5576, so we only call span.finish() in the onClose() method.

As per GRPC expected behaviour, ClientCall.Listener.onClose() is the last method to be called in the ingress client thread and, if there is a client error, the ClientCall.cancel() is the last method to be called in the egress client thread. Since they run in different threads and cancel() may run after onClose(), we would still need to check that the span is not already finished, before adding logs/tags to the span in the cancel() method.