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

Span parent contexts should always be a Span or SpanContext #345

Closed
toumorokoshi opened this issue Dec 20, 2019 · 5 comments
Closed

Span parent contexts should always be a Span or SpanContext #345

toumorokoshi opened this issue Dec 20, 2019 · 5 comments
Labels
api Affects the API package. discussion Issue or PR that needs/is extended discussion.

Comments

@toumorokoshi
Copy link
Member

Currently, the Span object's parent can be both a Span, or a SpanContext object.

This is resulting in multiple parts of the code where one has to handle the Span parent case, and the SpanContext parent case.

It would be great if we decided to go one or the other. As a proposal, I'll suggest it should be a Span, as currently Span, as it contains additional data in addition to the context such as Events.

@toumorokoshi toumorokoshi added api Affects the API package. discussion Issue or PR that needs/is extended discussion. labels Dec 20, 2019
@carlosalberto
Copy link
Contributor

There was a discussion in the past about this, as extraction provides a SpanContext, and whether a 'wrapper' Span around this would be feasible.

I suggest to discuss this in the Specification repo, btw (so whether one way or another is decided, we will have more pieces to consider).

@Oberon00
Copy link
Member

Oberon00 commented Jan 10, 2020

This is resulting in multiple parts of the code where one has to handle the Span parent case, and the SpanContext parent case.

Without actually looking at all the cases, I would assume that this can be rectified simply by providing an utility function (that could even be in the API layer) like this:

def as_context(span_or_context):
  if isinstance(span_or_context, SpanContext): return span_or_context
  return span_or_context.get_context()

@toumorokoshi
Copy link
Member Author

yes, agreed this can be done on the api side by providing a utility function. That said, it's not immediately clear to me why ParentSpan needs to accept both. Generally, I think it's easier if we don't use more algebraic types than we need.

@codeboten
Copy link
Contributor

Here's an issue filed in the oteps repo that's somewhat related: open-telemetry/oteps#73

@mauriciovasquezbernal
Copy link
Member

Solved by #548.

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
…#391)

closes open-telemetry#345

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

No branches or pull requests

5 participants