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

Use start_as_current_span by default #246

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

c24t
Copy link
Member

@c24t c24t commented Oct 25, 2019

This is a follow up to #198, which renamed start_span to start_as_current_span.

There were several examples and tests with calls to start_span that look like they should have been updated, including the example in the main README.

@c24t
Copy link
Member Author

c24t commented Oct 25, 2019

To @toumorokoshi's point in https://github.com/open-telemetry/opentelemetry-python/pull/198/files#r333820669, I think there's a good argument for using the shorter start_span the default name.

@@ -65,7 +65,7 @@ def instrumented_request(self, method, url, *args, **kwargs):
path = "<URL parses to None>"
path = parsed_url.path

with tracer.start_span(path, kind=SpanKind.CLIENT) as span:
with tracer.start_as_current_span(path, kind=SpanKind.CLIENT) as span:
Copy link
Member

Choose a reason for hiding this comment

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

This package doesn't need/use current span reference, feels like start_span is actually available for this kind of scenarios

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, maybe we do want start_span here.

We may want to put spans into the context by default, even if we close the span before returning control. If start_span started and activated the span, and we had another method named start_span_but_dont_activate, would it still be a good idea to use the latter here?

@Oberon00 what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

the current span is actually referenced here, abstracted in the propagators layer:

propagators.inject(tracer, type(headers).__setitem__, headers)

https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/propagators/__init__.py#L47

Which I think is the correct behavior. I'm sure others will also rely on the assumption that the span was started here, regardless of whether the integration directly uses the context or not.

I would argue that most common cases require the tracer to use the new span, and we should only opt out of that behavior in advanced cases.

I'm +1 for start_span_but_dont_activate, or maybe pass that it as a keyword argument. (will_activate=False).

Copy link
Member

Choose a reason for hiding this comment

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

I think this is either a misusage of the inject API or the inject API should provide a way to supply the Span to propagate.

But I definitely agree on using start_as_current_span here. E.g. consider what should happen if we decided to represent HTTP redirects as their own spans: They would need to become child spans.

Copy link
Member

Choose a reason for hiding this comment

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

In general, I view the active span more as an inter-instrumentation communication. If only a single instrumentation library was involved it could devise its own mechanism to manage parent spans easily (although ofc relying on the current span should still be preferred for parent/child relations).

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Nice catch! I just noticed this a couple days ago, as it's causing a few of the tests in the tracecontext suite to fail (since spans are created but not activated, so the same span is used over and over again).

This PR is needed to unblock:
#228

@Oberon00 Oberon00 changed the title Make start_as_current_span default Use start_as_current_span by default Oct 28, 2019
@c24t c24t merged commit f803b30 into open-telemetry:master Oct 28, 2019
@c24t c24t deleted the start-as-current-default branch October 28, 2019 18:49
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
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

Successfully merging this pull request may close these issues.

4 participants