Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Remove span_stack and store parent_span in span object#52

Merged
liyanhui1228 merged 7 commits intocensus-instrumentation:masterfrom
liyanhui1228:context
Oct 31, 2017
Merged

Remove span_stack and store parent_span in span object#52
liyanhui1228 merged 7 commits intocensus-instrumentation:masterfrom
liyanhui1228:context

Conversation

@liyanhui1228
Copy link
Copy Markdown
Contributor

@liyanhui1228 liyanhui1228 commented Oct 30, 2017

Add get/set_current_span methods in execution_context, use the current span as the parent span when creating new spans. Remove span_stack, and instead use the parent_span object in span to record the relationship between spans.

Closes #51

@liyanhui1228 liyanhui1228 requested a review from duggelz October 30, 2017 18:48
parent_span_id = self.span_context.span_id
parent_span = self.current_span()

if parent_span is None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment describing the django case you explained to me?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return

cur_span.finish()
self.span_context.span_id = cur_span.parent_span.span_id
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it valid to manually create a span like Span(parent_span=None,...)? In that case, we would need to check for None here, and set span_id to 0 or something. It seems like we have 3 cases:

  1. parent_span is a Span object
  2. parent_span is a NullContextManager because we got the span_id from an HTTP header
  3. parent_span is None because we manually created a root span.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we have these 3 cases, I added check for parent_span in span constructor and documented that user should start and end span using RequestTracer instead of directly using the span class.

Comment thread trace/opencensus/trace/span.py Outdated
labels = {}

# Do not manipulate spans directly using the methods in Span Class,
# make usre to use the RequestTracer.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usre -> sure

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@liyanhui1228 liyanhui1228 merged commit a155f47 into census-instrumentation:master Oct 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants