Finishing Span on close

[carlosalberto]

I'm opening this issue as I'm curious on whether we should adopt the (now merged) OT Java PR for deprecating finishing Spans upon Scope.close().

Problem

For OT Java, finishing Span upon Scope.close() makes it impossible (or hard at least) to report/log errors, as the Span tends to be finished when an Exception is caught. For Python, this is not exactly the same, as Python offers hooks for with statements (the equivalent to try-with Java block) for reporting errors - and as part of #101 we already do that.

So the question is left in case the user wants to report custom errors or additional data upon Exception catching (and how often this truly happens). In OT Java, a close-resources block (try-with statement) can be used to both close the Scope and report the error - for Java, the equivalent block (with statement) does not allow catching Exceptions, and a try-except-finally block is needed instead:

span = tracer.start_span('foo')
scope = tracer.scope_manager.activate(span, False)
try:
  # do work
except Exception as e:
  span.set_tag('...', '...')
  span.log_kv({...})
finally:
  scope.close()
  span.finish()
  # A single call to scope.close() would have been enough, if we had used finish-on-close behavior

Asynchronous frameworks

Most of the time, when working with asynchronous frameworks (gevent, asyncio, tornado) results in calls where we automatically can close the Span and Scope:

with tracer.start_active_span('foo', True):
  # Can close the Span after yielding/awaiting on any coroutine here.
  # Errors would be caught already with the with-block hooks mentioned earlier
  yield my_other_coroutine()

Removing finish-span-on-close means asynchronous frameworks instrumentation will have to handle their Spans lifetime themselves, even if the above case covers the common case:

span = tracer.start_span('foo')
with tracer.start_span('foo') as span:
  with tracer.scope_manager.activate(span): # Remember, not closing Span anymore
    # do things

Or, when reporting errors, a full try statement as described above.

Scope.span being deprecated

As part of the OT Java change, Scope.span access has been deprecated, as we want to prevent the users from passing Scope objects between threads.

However, Tornado instrumentation already is passing Scope objects down the chain between coroutines (the equivalent to threads), even if it's passed in a defacto read-only fashion - similarly, C# with its AsyncLocalScopeManager implementation passes a reference to Scope down the callbacks (which is also how the future contextvars module will work for Python 3.7). Probably it's all-right to do that as long as we do it behind the scenes to not tempt the user to juggle the Scope object himself?

Also, deprecating it here would make the usage of start_active_span() slightly complicated, as we wouldn't have a direct reference to Span:

with tracer.start_active_span('foo') as scope:
   # no scope.span
   tracer.active_span('...', '...')

@yurishkuro @pglombardo @opentracing/opentracing-python-maintainers
cc @indrekj (would be of interest, as we are touching a dynamic language ;) )

[carlosalberto]

My personal take: keep start_active_span() (maybe simply changing the name, as we have considered before), because of the 1) with hooks to automatically report an error, and 2) Easy integration with asynchronous frameworks. Reporting an error by the user himself feels as a more advanced, specific scenario to me ;)

Also, deprecating Scope.span would make things slightly complicated, specially if we keep start_active_span(). And given the Tornado/C# handling, we ought to make super clear in the documents about the risks of it - and "in case of doubt, pass a Span instead".