Refactor current span handling for newly created spans.

[carlosalberto]

1. Make Tracer.start_span() simply create and start the Span,
   without setting it as the current instance.
2. Add an extra Tracer.start_as_current_span() to create the
   Span and set it as the current instance automatically.

Notes:

• Made Span implement the context manager interface so it can be used in with blocks, by simply ending it.
• No end_on_close parameter for start_as_current_span() - I think this should stay as an advanced scenario, which can be obtained by directly using use_span().
• I'm not totally bought on the name, so feel free to suggest a better name ;)
• Refactored the Span creation tests to use Tracer.start_span() . instead, and have extra (minimalistic) tests for start_as_current_span() that simply assert the current Span handling.

[c24t]

A few non-blocking comments. The changes look good, but I'm worried this makes the API more cumbersome since I'd expect more calls to start_as_current_span than start_span.

[c24t]

This is kind of confusing since Span is the context manager now, not start_span.

[carlosalberto]

Oh, right, this needs to be cleared up.

[c24t]

Suggested change

	with tracer.start_span("two") as child:
	with tracer.start_span("one") as child:

If you want to make this consistent with the other example.

[carlosalberto]

Makes sense, will update.

[c24t]

Unless I'm missing some use case, now that Span calls end on exit it looks like you could lose end_on_exit and just let use_span be responsible for setting the context:

with self.start_span(name, parent, kind) as span:
    with self.use_span(span):
        yield(span)

[carlosalberto]

This one didn't work - calling yield() on the Span didn't to the trick/chaining, so I had to keep the original one ;)

[c24t]

Suggested change

	) -> bool:
	) -> Optional[bool]:

According to https://github.com/python/mypy/blob/master/docs/source/protocols.rst#context-manager-protocols, and IIRC most context managers like this return null.

[carlosalberto]

Yeah, I was wondering about this one later on (specially about the null case - I'm wondering why there's nothing for this already available in the typing module ;) )

[c24t]

Why keep this one a context manager instead of returning the span like the other?

[carlosalberto]

Oh, I didn't try that out. I will give it a spin and see how it looks ;)

[carlosalberto]

So I'd have no problem making start_as_current_span() the default, as long as the 'standard' start_span() does not set the new Span as the current one (to keep uniformity with other implementations). And yeah, I'd also expect more calls to start_as_current_span().

(Btw, in OpenTracing we had start_active_span() which was fine for us, but using start_current_span() will probably not be a popular choice here ;) )

[Oberon00]

Seeing open-telemetry/opentelemetry-specification#21, what about instead of having a start_as_current_span on the tracer, we just have start_span on the tracer (that starts a new span but does not set it as active) and instead of having Tracer.use_span, we have Span.use() with the same functionality (implicitly using the Tracer that created the span). Then you would write a typical example like this:

with tracer().start_span("foo").use() as span:
   span.set_attribute("foo", "bar")

[reyang]

I've always been struggling with naming.
Given this technically is start_span then use_span, maybe we should just call it start_and_use_span❓

[Oberon00]

I can add one more suggestion: use_new_span.

[toumorokoshi]

I'd actually argue that "start_span" should remain as is, and we should maybe provide separate terminology for the two components of span creation and use.

I don't imagine most consumers will really need to separate span creation and use, and may find it as needlessly verbose for "start_and_use_span".

[Oberon00]

This is also about consistency across languages.

[reyang]

LGTM. We might want to update the examples as well?

[Oberon00]

Definitely an improvement. We shouldn't bikeshed for too long here. @carlosalberto please merge if you yourself think this is ready.

[Oberon00]

👍

[Oberon00]

Suggested change

	"""Ends context manager and calls end() on the `Span`.
	"""Ends context manager and calls `.end` on the `Span`.

[Oberon00]

I'd re-add

Suggested change

	with self.use_span(span, True):
	with self.use_span(span, end_on_exit=True):

[Oberon00]

Actually the function can be implemented more simply and efficiently without @contextmanager and Iterator by replaing the with statement: return self.use_span(span, True).

[carlosalberto]

Hah! Right. I remember this is how I envisioned it first, but I guess I was decaffeinated when I wrote it ;(

[Oberon00]

Actually, I think we should implement start_as_current_span in the API. Other API-implementations that need special behavior here can still override. EDIT: But this is not why I "requested changes".

[Oberon00]

Iterator is the wrong type here. Correct would be ContextManger[Span] but that does not exist in Python 3.5. So I guess this is a case where we have to use something like -> SpanContextManager and above

MYPY = False
if MYPY:
    SpanContextManager = typing.ContextManager[Span]
else:
    SpanContextManager = typing.Any

https://mypy.readthedocs.io/en/latest/common_issues.html#import-cycles

[carlosalberto]

Honestly I'd leave this out for now, as it makes the code ugly ;)

(Else, I'd add it in another PR)

[Oberon00]

I don't want to hold this PR up and I don't know why mypy accepts it and I agree that this (my suggested) code is very ugly. But: If I'm reading this correctly, the annotation is in fact wrong. If it were correct, you'd use this function as for span in tracer().start_as_current_span(). An iterator has no set_attribute, but it has __next__. A context manager has neither.

[carlosalberto]

I honestly feel like this is not the place to discuss this - this change it's even now, as we speak, in master - check out https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/trace/__init__.py#L385

-> It's still defined as start_span instead of start_as_current_span and in both cases, a Span is yielded.

I have no problem adjusting things that were changed in this PR, but I honestly see no reason to change things that were there before, unless they are fatal. Otherwise, we will in my next review something else that could be improved, and then we will have to discuss them and have yet another iteration ;)

[Oberon00]

The point is that your removed the @contextmanager and consequently changed yield to return so it keeps working fine at runtime, but I think it messes up the type annotation. Unless the interaction between @contextmanager and the function return type is not taken into account by mypy, then it would already have been wrong before.

[Oberon00]

--> #219

[carlosalberto]

So I think this is ready to go, after applying the latest of @Oberon00 's feedback.

We can iterate the the mypy/start_as_current_span() in the API changes in other PRs. Let me know ;)

fixed