Move useSpan() out of Tracer

[carlosalberto]

Currently the Specification expects that the current Span handling happens in a single place, either module level functions or in a class, and whereas set_span_in_context() and get_current_span() live in the trace module, useSpan() exists in the Tracer.

[owais]

I can take this. To clarify, we want to move userSpan() method from Tracer to a module level function in the trace module, right?

[lzchen]

@owais
I believe it would exist in trace.propagation

[owais]

Propagation module makes sense as it already contains set_span_in_context and get_current_span but wondering if ease of use triumphs more logical source organization. useSpan() is after all a convenience method and I think most project import trace anyway and trace.useSpan() is probably a lot more convenient.

I don't feel too strongly about it though so I'm fine either way.

[lzchen]

I don't feel too strongly either but I'd to not take on more work than necessary.
@carlosalberto
Is this a must-have as defined by the specs?

[carlosalberto]

opentelemetry.trace.use_span() sounds fine as it's a convenience method as @owais pointed out, so I suggest going that path ;)

[lzchen]

Sure, @owais let's go with the above suggestion.

[owais]

I have a WIP branch for this but we might have a problem here. The use_span method implicitly depends on the SDK. It doesn't import or refer to anything in the SDK but the logic it contains depends on SDK's Span implementation:

Lines 968, 974 and 975 here: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L954

I got rid of 968 and 975 by making use_span accept record_exception and set_status_on_exception as explicit arguments but 975 is still there. (https://github.com/open-telemetry/opentelemetry-python/pull/1668/files#diff-129656b44f9a21329fa1a15877054e2a83b1e5f75b553064cf4d55dc50fc4cdeR443)

While this works and doesn't add SDK as a dependency to API, it still embeds knowledge about the SDK Span implementation inside an API function. It means 3rd party Span implementations might not work as expected.

I'm not sure if it is worth moving out of tracer at this point. The logic doesn't need tracer but it is implicitly bound to the span implementation.

What do you think? @lzchen @carlosalberto @codeboten

[owais]

BTW what are your thoughts removing the check entirely and always setting status. How bad is it if a status is already set on span but use_span() overwrites it in case of an exception? If a user is setting status manually and doesn't want use_span() override it, they can pass set_status_on_exception=False so we do provide control over it.

If we move the check, we remove the implicit dependency. Thoughts?

[lzchen]

@owais
I feel like with the tight coupling with span implementation, use_span shouldn't be moved out of the tracer. Perhaps we can leave use_span inside tracer, but in the sdk implementation, for the "setting current span", we could call set_span_in_context() directly. That way, use_span isn't really responsible for "span handling" and more of a convenience method on tracer.

[owais]

I agree but the more I think about it, the more I'm convinced current behavior of use_span() is actually wrong. Consider the following example

def func():
    with use_span(span, set_status_on_exception=True, end_on_exit=True):
        result = some_operation()
        if result.ok:
             span.set_status(Status(StatusCode.OK))
        return json.dumps(result)

Intuitively user would expect span to have OK unless json.dumps() fails. In that case user would expect use_span() to correct set status to Error.

Span API allows overwriting status so looks like we are fine with that.

If we "fix" this behavior, we can move forward with this change but even if we don't, I think use_span should still be updated to overwrite status when an exception is raised and set_status_on_exception is set to True.
I'm not sure what the spec says about replacing

[owais]

The spec actually clarifies this at the very end in the following section.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status

Only the value of the last call will be recorded, and implementations are free to ignore previous calls.

So looks like this check is unnecessary and should actually be removed. I'll update the PR accordingly.