Initial support for explicit Span parent support.#33

Merged

reyang merged 7 commits intoopen-telemetry:masterfrom

carlosalberto:span_explicit_parent

Jul 3, 2019

Contributor

carlosalberto commented Jun 26, 2019

Addresses #23

It uses a new public constant CURRENT_SPAN to signal that the default parent is the current Span.

I applied the same logic to create_span, to have uniformity. OC Java, for example, allows users to start a Span as either the current instance or not, and both implicitly used the previous current Span as parent. We also had this in OT Python and it worked fine.

Opinions on this last change?


          Initial support for explicit Span parent support.

6b3e057

carlosalberto requested review from Oberon00, c24t and reyang as code owners

June 26, 2019 18:13

reyang reviewed

View reviewed changes

opentelemetry-api/opentelemetry/trace/__init__.py Outdated

+                  """Constant used to represent the current span being used as a parent.
+                  This is the default behavior when creating spans.
+                  """
+                  CURRENT_SPAN = object()

Member

reyang Jun 26, 2019

Consider _CURRENT_SPAN if this is meant to be used inside this file.

Member

Oberon00 Jun 27, 2019

I think exposing this constant is fine, but maybe the name may be a bit misleading in a broader context. Maybe name it CURRENT_SPAN_AS_PARENT?

Actually we need to expose it, as API implementations will need to compare against it.

Member

Oberon00 Jun 27, 2019

One more point: This will not typecheck (CURRENT_SPAN does not have type Span). We should use typing.NewType to create a dummy type for this object and then accept that type in the functions that support CURRENT_SPAN (in a typing.Union).

Contributor Author

carlosalberto Jun 27, 2019

Actually we need to expose it, as API implementations will need to compare against it.

Correct. My initial thought had been to hide it but we have to expose it ;)

And yes, we can use typing.NewType for this. But I'm actually thinking we could simply try to use the Java approach here, by having a specific default/no-op Span that will effectively act as such dummy. Will update the PR ;)

reyang reviewed

View reviewed changes

opentelemetry-api/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved

c24t approved these changes

View reviewed changes

Member

c24t left a comment

Using a sentinel for the span kwarg is a great solution, and does make for a nice API.

It took me a few passes to understand that CURRENT_SPAN wasn't meant to be the actual span, if other people find this confusing we might want to call it something else.

opentelemetry-api/opentelemetry/trace/__init__.py Outdated

+                  """Constant used to represent the current span being used as a parent.
+                  This is the default behavior when creating spans.
+                  """

Member

c24t Jun 26, 2019

Suggested change

      
                """
          
                # Constant used to represent the current span being used as a parent. This
          
                # is the default behavior when creating spans.

Member

Oberon00 Jun 27, 2019 •

edited

Loading

The Sphinx syntax for documentation comments uses #:

Contributor Author

carlosalberto Jun 27, 2019

Aha! Got it.

Oberon00 reviewed

View reviewed changes

opentelemetry-api/opentelemetry/trace/__init__.py Outdated

+                  """Constant used to represent the current span being used as a parent.
+                  This is the default behavior when creating spans.
+                  """

Member

Oberon00 Jun 27, 2019 •

edited

Loading

The Sphinx syntax for documentation comments uses #:

opentelemetry-api/opentelemetry/trace/__init__.py Outdated

+                  """Constant used to represent the current span being used as a parent.
+                  This is the default behavior when creating spans.
+                  """
+                  CURRENT_SPAN = object()

Member

Oberon00 Jun 27, 2019

I think exposing this constant is fine, but maybe the name may be a bit misleading in a broader context. Maybe name it CURRENT_SPAN_AS_PARENT?

Actually we need to expose it, as API implementations will need to compare against it.

opentelemetry-api/opentelemetry/trace/__init__.py Outdated

-                  def start_span(self, name: str, parent: 'Span') -> Iterator['Span']:
+                  def start_span(self,
+                                 name: str,
+                                 parent: 'Span' = CURRENT_SPAN) -> Iterator['Span']:

Member

Oberon00 Jun 27, 2019

Nit: Normally you don't put spaces around the = in a default argument, but I don't know how the situation is with type annotations.

Contributor Author

carlosalberto Jun 27, 2019

Same, was not sure about how things work with annotations (had barely used them before ;) )

Hope we can get a linter enabled soon :)

Member

c24t Jun 28, 2019

This is right! And pylint actually does report on this if you remove the spaces:

opentelemetry/trace/__init__.py:95:33: C0326: Exactly one space required around keyword argument assignment
                   parent: 'Span'=CURRENT_SPAN) -> Iterator['Span']:
                                 ^ (bad-whitespace)

opentelemetry-api/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved

opentelemetry-api/opentelemetry/trace/__init__.py

+                      Applications that need to create spans detached from the tracer's
+                      context should use this method.
                           with tracer.start_span(name) as span:

Member

Oberon00 Jun 27, 2019

I think this sample code is a copy & paste error. It should probably not be here (although it made more sense when the paragraph talking about use_span was directly before it).

Contributor Author

carlosalberto Jun 27, 2019

Was wondering the same.

opentelemetry-api/opentelemetry/trace/__init__.py Outdated

+                  """Constant used to represent the current span being used as a parent.
+                  This is the default behavior when creating spans.
+                  """
+                  CURRENT_SPAN = object()

Member

Oberon00 Jun 27, 2019

One more point: This will not typecheck (CURRENT_SPAN does not have type Span). We should use typing.NewType to create a dummy type for this object and then accept that type in the functions that support CURRENT_SPAN (in a typing.Union).

Contributor Author

carlosalberto commented Jun 27, 2019

It took me a few passes to understand that CURRENT_SPAN wasn't meant to be the actual span, if other people find this confusing we might want to call it something else.

Agreed.

reyang approved these changes

View reviewed changes

c24t added this to the Tracing API milestone

carlosalberto added 2 commits

July 2, 2019 16:43


          Merge branch 'master' into span_explicit_parent

dad0199


          Update the PR based on the latest feedback.

14e7aaf

Contributor Author

carlosalberto commented Jul 2, 2019

Hey hey - I've updated the PR, but the part regarding CURRENT_SPAN using either a dummy object (through NewType()) or through actually using Span isn't done - reason is that Span needs to appear before Tracer for either case.

My initial idea was to move Span right above Tracer, but that led me to wonder whether we should split each class in their own file, for a start - we did this for OT and it worked fine. Any opinion here, before I proceed?

Member

c24t commented Jul 2, 2019

I vote for keeping them in the same module and moving Span above Tracer. I know this is largely a matter of personal taste, but I'd prefer to use modules (as opposed to packages) to organize related classes. I know I find it easier to grok code that uses modules to model the logical structure of the project than as containers for individual classes.

Some other benefits:

Nicer automatic docs generation with e.g. sphinx-apidoc since conceptually related classes are in the same generated doc.
Keeps import paths short and avoids stutter, e.g. to avoid ot.trace.tracer.Tracer.
Nicer diffs: fewer separate files to review per PR, the change history for a given module describes the evolution of a feature, and changes to classes (including name changes) don't break history.

we did this for OT and it worked fine

We had the opposite experience in OC. One of the original sins of OC-python was that some features were transliterated straight from java, including a bunch of classes that (as in java) each got their own file. As the implementations drifted apart to better suit each language, the fact that the two projects had similar structure became more confusing than helpful since assumptions from one project didn't always carry over to the other.

I may be biased in seeing single-class modules as "java-like" and multiple-class modules as pythonic, but in my experience modules -- like namespaces -- are one of the nice features of the language, and make for a more easily comprehensible project.

c24t reviewed

View reviewed changes

opentelemetry-api/src/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved

opentelemetry-api/src/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved

Contributor Author

carlosalberto commented Jul 3, 2019

I vote for keeping them in the same module and moving Span above Tracer. I know this is largely a matter of personal taste

Lets use this one for now then. If/when we feel we need a different way, we can go for it.

We had the opposite experience in OC. One of the original sins of OC-python was that some features were transliterated straight from java

Interesting point. In any case, lets then keep what we have now ;)

carlosalberto and others added 4 commits

July 3, 2019 13:22


          Update opentelemetry-api/src/opentelemetry/trace/__init__.py

e46599f

Co-Authored-By: Chris Kleinknecht <libc@google.com>


          Update opentelemetry-api/src/opentelemetry/trace/__init__.py

464c963

Co-Authored-By: Chris Kleinknecht <libc@google.com>


          Move Span/SpanContext up so we can reference it from Tracer.

9dc87e8

Specifically for CURRENT_SPAN usage.


          Fix a line-too-long.

30b6c21

Contributor Author

carlosalberto commented Jul 3, 2019

Hey, I've updated the code so now CURRENT_SPAN = Span().

Two items remain, and they should do as separated PRs in my opinion:

Remove the defacto duplicated example in the code, as pointed out by @Oberon00
Consider (and discuss) defining a specific no-op Span instance - in Java, for example, we have DefaultSpan which is used as no-op Span (for things like Tracer.getCurrentSpan()), which can be accessed through DefaultSpan.getInvalid().

Let me know. If all is fine, I will go ahead and merge this PR.

Oberon00 approved these changes

View reviewed changes

Member

Oberon00 left a comment

LGTM 👍

c24t mentioned this pull request

Standardize import style #36

Closed

reyang merged commit bd13411 into open-telemetry:master

carlosalberto mentioned this pull request

Add explicit parent Span support for Tracer.start_span(). #23

Closed

toumorokoshi mentioned this pull request

sdk: span parents are now always spancontext #548

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet