Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Add close() method to io.opentracing.Tracer #302

Closed
safris opened this issue Sep 10, 2018 · 9 comments
Closed

Add close() method to io.opentracing.Tracer #302

safris opened this issue Sep 10, 2018 · 9 comments

Comments

@safris
Copy link

safris commented Sep 10, 2018

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.

The close() method can 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.

@jpkrohling
Copy link
Contributor

The close() method can be declared with a noop default

IIRC, this library is still on Java 6, so, no default implementation for interfaces are available :/

@safris
Copy link
Author

safris commented Sep 10, 2018

I just checked the io.opentracing:parent:pom, and it has compiler source and target set to 1.7.

@jpkrohling
Copy link
Contributor

@safris
Copy link
Author

safris commented Sep 10, 2018

That override had eluded me. The override is itself a bad practice. So how should this move forward?

  1. Declare the close() method without a default: This would force some vendors to have to implement the close() method if their Tracer(s) don't already have it implemented.
  2. Remove the override: The class bytecode versions across the submodules are not consistent, which has the potential to lead to frustrating situations for vendors and app developers (this being one of them). I cannot comment on the reason for the override, as there is no comment in the pom where the override is set. Is this a path worth following? It's out of the scope of this issue, so a new issue would have to be created.
  3. Do nothing: Keep the jdk1.6 override, and do not declare a Tracer.close(). This would force application code to be coupled to vendor implementations of the Tracer interface.

@yurishkuro
Copy link
Member

This issue has come up a number of times before. Maybe worth searching for history for the arguments.

Personally I am supportive of adding close(). While the reasons against raised previously are still valid, it is clear that community needs this mechanism to release the tracer and its resources. I believe we have close() in one of the languages (Python?) and there have been no complaints from implementors.

However, backwards compatibility is an issue.

@yurishkuro
Copy link
Member

Btw, option 4 to the list above - declare optional interface for closing and cast the tracer.

@safris
Copy link
Author

safris commented Sep 10, 2018

Yes, this is an Option 4. However, this option is a hack that would further bury the primary issues (not having a close() on Tracer, and the jdk1.6 override). I believe Option 4 would be to incur a mindful technical debt.

@carlosalberto
Copy link
Collaborator

This was being addressed in #250 - maybe worth checking the arguments there (at the time we had decided to not add this method and put if in a FAQ, which... never happened).

Personally I'd recommend we 1) do not override the Java version (to avoid confusion) and stick to Java 1.6 (using 1.7 or newer would require another discussion IHMO), and 2) if we add this close() method, add it directly to the Tracer interface.

@yurishkuro
Copy link
Member

I am going to close this as a dup of #250, please continue discussion on close() there. Regarding dropping 1.6 - a separate issue altogether. We had a similar 1.6 discussion jaegertracing/jaeger-client-java#511 and did not arrive at an agreement, and that's just a single implementation of the API.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants