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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use current span for SDK-less context propagation #510

Merged
merged 2 commits into from Apr 6, 2021

Conversation

frigus02
Copy link
Member

@frigus02 frigus02 commented Apr 5, 2021

In the absence of an SDK the API should not record spans anymore but it should propagate the span context. In our case absence of an SDK means we're using the NoopTracer.

See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#behavior-of-the-api-in-the-absence-of-an-installed-sdk

There are different ways to create new spans and they provide different ways of specifying the parent context:

  • Tracer::start: uses Context::current()
  • Tracer::build: uses the context from the span builder or falls back to the current context. BUT we did not have this fallback in the NoopTracer.
  • Tracer::start_with_context: uses the specified context

This changes the SpanBuilder to make its parent_context non-optional. That way the current context fallback is done on
SpanBuilder creation. Tracer implementations just need to read the parent_context from the span builder.


  • I hope I understood the spec correctly here 馃檪
  • I'm not entirely sure if this is the best way to do it. Happy to take suggestions.

In the absence of an SDK the API should not record spans anymore but it
should propagate the span context. In our case absence of an SDK means
we're using the `NoopTracer`.

There are different ways to create new spans and they provide different
ways of specifying the parent context:

- Tracer::start: uses Context::current()
- Tracer::build: uses the context from the span builder or falls back to
  the current context. BUT it did not have the fallback in the
  NoopTracer.
- Tracer::start_with_context: uses the specified context

This changes the `SpanBuilder` to make the `parent_context` in it
non-optional. That way the current context fallback is done on
`SpanBuilder` creation. Tracer implementations just need to read the
`parparent_context` from the span builder.
@frigus02 frigus02 requested a review from a team as a code owner April 5, 2021 09:35
Copy link
Contributor

@TommyCpp TommyCpp 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. LGTM

@jtescher
Copy link
Member

jtescher commented Apr 6, 2021

This looks good, wondering if there are cases where people may want to build up span data before they have the context, but can't think of any reason currently.

@jtescher jtescher merged commit f7db050 into open-telemetry:main Apr 6, 2021
@frigus02 frigus02 deleted the noop-propagation branch May 3, 2021 06:41
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.

None yet

3 participants