Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offer a specific/static no-op Span instance. #41

Closed
carlosalberto opened this issue Jul 3, 2019 · 5 comments
Closed

Offer a specific/static no-op Span instance. #41

carlosalberto opened this issue Jul 3, 2019 · 5 comments
Assignees

Comments

@carlosalberto
Copy link
Contributor

In Java, we have a DefaultSpan which does nothing by default (it can propagate SpanContext too, but that's more of additional stuff), and can be used as both a no-op object.

Specifically, DefaultSpan.getInvalid() offers a static instance that can also be used when setting parent (through SpanBuilder.serParent()) and also for Tracer.getCurrentSpan() (so the user doesn't have to do a null check).

As Span at this moment already has this no-op functionality, maybe it makes sense to add a class field? This way we could do:

def start_span(..., parent = Span.invalid(), ...):
   if parent is Span.invalid():
     # fetch the actual current Span as parent.

def get_current_span(self):
   ...
   # Return a no-op in case there's no current Span.
   return Span.invalid()
@Oberon00
Copy link
Member

Oberon00 commented Jul 3, 2019

Yeah, a designated instance would be nice, if only to improve performance. If we want to enforce that there can be only one instance of plain Span we could even override __init__ to throw if type(self) is Span (one can set __init__ after creating the singleton instance). But instead of a method I'd prefer a module or class-level attribute.

We could even override __new__(cls, *args, **kwargs) to always return the same instance (falling back to Python's default of creating a new instance if cls is not Span), but that is probably too much magic.

@c24t
Copy link
Member

c24t commented Jul 8, 2019

👍 for having our own designated default span singleton.

One argument for making this a module-level constant is that it means one less concrete method in the API class (assuming you're saying we'd implement invalid in the API), but either way SGTM.

@Oberon00 you think it's worth locking down instantiation of Span? It does seem like it'd always be a mistake to create more than one plain Span, but it's not clear that it's worth the extra complexity to forbid it.

@Oberon00
Copy link
Member

Oberon00 commented Jul 9, 2019

@Oberon00 you think it's worth locking down instantiation of Span?

If you think that it is always a mistake to create a new Span then yes. It's easy after all:

class Span:
    """"..."""

INVALID_SPAN = Span()

def _init_span(self):
  if type(self) is Span: raise RuntimeError("Cannot create a new plain Span")

Span.__init__ = _init_span

Then there is also always the argument that allowing something later is a new feature but banning something later is a breaking change.

@carlosalberto
Copy link
Contributor Author

It does seem like it'd always be a mistake to create more than one plain Span, but it's not clear that it's worth the extra complexity to forbid it.

At least in Java, a few DefaultSpan objects can be created (indirectly), while users are encouraged to use DefaultSpan.getInvalid(). Once the objects make this assumption is very clear, we should be fine.

@carlosalberto carlosalberto self-assigned this Jul 18, 2019
@c24t
Copy link
Member

c24t commented Jul 30, 2019

Closed by #63.

@c24t c24t closed this as completed Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants