Remove finish span on close

carlosalberto commented 6 years ago

Addresses #291

SpanBuilder.startActive(Span, boolean) and ScopeManager.activate(Span, bool) have been kept, but as deprecated methods.

Updated the opentracing-testbed code to use this change, and added a new, small, error-reporting case.

cc @felixbarny @adriancole @yurishkuro @tedsuo @sjoerdtalsma

coveralls commented 6 years ago

Coverage decreased (-0.6%) to 73.464% when pulling fdb669950da30f0693e7310a39de675fff34bd12 on carlosalberto:remove_finish_span_on_close into d6c6509d5524f99039e34cbf121df79c46a25147 on opentracing:v0.32.0.

codefromthecrypt commented 6 years ago

on the note about scope.. it is odd because scope in opentracing still has an accessor to the span. it makes it awkward and still encourages passing the scope around. I think this should be removed along with this change.

When scope.span() is removed, it leaves you with at least two paths for doing "startActive" if chosen as a method to retain.

1) encourage use of "current span" logic like census https://github.com/census-instrumentation/opencensus-java/blob/c943c6c63637cc866e92b90dae0e48dccd488130/api/src/main/java/io/opencensus/trace/SpanBuilder.java#L186 2) use a special span that closes an implicit scope on finish like brave https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/ScopedSpan.java#L7

Either way, please don't forget to remove your scope.span as it encourages people to leak scopes.

carlosalberto commented 6 years ago

Hey @adriancole

Thanks for the feedback. So my personal take for your points is:

That's not a bad thing, but then people would need to remember to do something like:

Span span = tracer.buildSpan("one").startActive();
try {
} catch (Exception e) {
  ...
  span.finish();
} finally {
   tracer.scopeManager.active().close();
}

This being implicit kinda makes me wonder (but definitely thing to consider).

A special Span is something that would add complexity to the current (relatively simple) API. For example: what happens if this very special Span is passed to another thread? I'd leave it as a potential addition in the future.

So the thing about removing Scope.span() is probably fine, as long as we have ScopeManager.activeSpan() (so we always expose the current active Span and the active Scope without explicitly linking them together). However, I'd like to hear the opinion of other people here.

Opinions? @felixbarny @yurishkuro @tedsuo

yurishkuro commented 6 years ago

are we shooting for this?

Span span = tracer.buildSpan("nasty").start();
try (Scope scope = tracer.activate(span)) {
  // do nasty stuff
} catch (Exception e) {
  span.SetTag(...);
  ...
} finally {
  span.finish();
}

It makes the usage slightly more verbose than the current api, especially without try-with-resource (only if <=1.6, who cares), but avoids the misuse. scope.span() is removed.

felixbarny commented 6 years ago

I agree with @yurishkuro

Remove* SpanBuilder#startActive
Remove* Scope.span() (this is only useful in combination with SpanBuilder#startActive)
Remove* ScopeManager.activate(Span, boolean)

*deprecate and eventually remove

carlosalberto commented 6 years ago

Hey,

Was wondering, after the feedback, about this (small variation):

SpanBuilder.startActive() and Scope.span() get removed
ScopeManager.activeSpan() and ScopeManager.active() (so we have the two of them available, but not linked to each other, thus less chances to have users pass Scope objects arounds).
Trace.activeSpan() and Tracer.activate() as aliases to ScopeManager's respective methods.

carlosalberto commented 6 years ago

For additional information: some frameworks do not let us 'wrap' their entire operation, but provide hooks for start and end events (guaranteed to run in the same thread), so we either need a) to let Scope be passed around (even if there is a risk that some users may pass it to another thread), or b) expose it in ScopeManager.active() (which is also risky).

In some cases, as mentioned in the main discussion, the user does not even have a place to store context information as part of the aforementioned start/end hooks, so the latter is definitely helpful.

Either way, we decide to go a) or b) (or even both), and improving greatly the documentation (mentioning what should never be done with Scopes and related) should be the way to go.

felixbarny commented 6 years ago

Sounds fine to me