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.

Sign up for GitHub

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

Jump to bottom

Move context utils outside of the Tracer #527

Closed
wants to merge 9 commits into from
Closed

Move context utils outside of the Tracer #527

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9442fa3
Move context utils outside of the Tracer
bogdandrutu Mar 24, 2020
f29a958
Apply feedback.
bogdandrutu Mar 24, 2020
7248740
Apply more feedback.
bogdandrutu Mar 24, 2020
25365ce
Update specification/api-tracing.md
bogdandrutu Mar 24, 2020
b61b7fd
Update specification/api-tracing.md
bogdandrutu Mar 27, 2020
93d9e14
Update specification/api-tracing.md
bogdandrutu Mar 27, 2020
33649f0
Update specification/api-tracing.md
bogdandrutu Mar 27, 2020
d9a767d
Update specification/api-tracing.md
bogdandrutu Mar 27, 2020
57774d4
Update api-tracing.md
bogdandrutu Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 32 additions & 25 deletions specification/api-tracing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Table of Contents
* [Tracer operations](#tracer-operations)
* [SpanContext](#spancontext)
* [Span](#span)
* [Span Context Interaction](#span-context-interaction)
* [Span creation](#span-creation)
* [Determining the Parent Span from a Context](#determining-the-parent-span-from-a-context)
* [Add Links](#add-links)
Expand Down Expand Up @@ -68,13 +69,7 @@ A duration is the elapsed time between two events.

## Tracer

The OpenTelemetry library achieves in-process context propagation of `Span`s by
way of the `Tracer`.

The `Tracer` is responsible for tracking the currently active `Span`, and
exposes functions for creating and activating new `Span`s. The `Tracer` is
configured with `Propagator`s which support transferring span context across
process boundaries.
The `Tracer`'s main responsibility is to allow creating `Spans`.

### Obtaining a Tracer

Expand Down Expand Up @@ -124,29 +119,13 @@ The `Tracer` MUST provide functions to:

- Create a new `Span`

The `Tracer` SHOULD provide methods to:

- Get the currently active `Span`
- Make a given `Span` as active

The `Tracer` MUST internally leverage the `Context` in order to get and set the
current `Span` state and how `Span`s are passed across process boundaries.

When getting the current span, the `Tracer` MUST return a placeholder `Span`
with an invalid `SpanContext` if there is no currently active `Span`.

When creating a new `Span`, the `Tracer` MUST allow the caller to specify the
new `Span`'s parent in the form of a `Span` or `SpanContext`. The `Tracer`
SHOULD create each new `Span` as a child of its active `Span` unless an
explicit parent is provided or the option to create a span without a parent is
selected, or the current active `Span` is invalid.

The `Tracer` SHOULD provide a way to update its active `Span` and MAY provide
convenience functions to manage a `Span`'s lifetime and the scope in which a
`Span` is active. When an active `Span` is made inactive, the previously-active
`Span` SHOULD be made active. A `Span` maybe finished (i.e. have a non-null end
time) but still active. A `Span` may be active on one thread after it has been
made inactive on another.
The `Tracer` MAY provide convenience functions to manage a `Span`'s lifetime.
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

## SpanContext

Expand Down Expand Up @@ -237,6 +216,33 @@ Vendors may implement the `Span` interface to effect vendor-specific logic.
However, alternative implementations MUST NOT allow callers to create `Span`s
directly. All `Span`s MUST be created via a `Tracer`.

### Span Context Interaction

The OpenTelemetry library achieves in-process context propagation of `Span`s by
way of the [Context](./context.md).

bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
An `active Span` referes to a Span that is set to an
Copy link
Member

Choose a reason for hiding this comment

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

That means, for other languages (e.g. Go) all Spans are inactive?

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 means there is no concept of active span. The current span is defined as the Span in the context passed to the function so no need to getCurrentSpan just getSpanFromContext.

Copy link
Member

@yurishkuro yurishkuro Mar 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest:

  • having a clear definition of "current" in the context.md, which can be defined both for implicit prop and for explicit prop (closest context in the lexical scope)
  • converge on a single term for spans, either "active" or "current", not both, and define it as "span stored in the current context"

Copy link
Contributor

Choose a reason for hiding this comment

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

@yurishkuro I agree on keeping either current or active, not both terms. However, I'm wondering if this should be done in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

@carlosalberto I agree but this PR adds wording using both terms, at least within the PR it should be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR does not add any new reference to either active or current, both were in the previous text.

Copy link
Member

Choose a reason for hiding this comment

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

"a span that is set to an attached context" sounds unclear to me

[Attached Context](./context.md#attach-context). This applies only to languages
using `Context` implicitly.

The API is responsible to provide functionality to track the currently
active Span, and exposes functionality to activate new Spans. The API MAY
provide default Propagators which support transferring span context across
process boundaries.

The API MUST provide methods (or, depending on the language, global functions
or a util class e.g. `TracingContextUtils`) to:

* Get the currently active Span
* Get/Set a Span from/to a Context
* Make a given Span active

The API MUST internally leverage the Context in order to get and set the current
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like logic is going into the API? Before the API was almost entirely interfaces.

Unless I'm misunderstanding this it seems instead of the Tracer simply calling to the sdk with a context and some options to start a span there is log in the API for juggling the span in the context, resulting in multiple calls from the API to the SDK?

How does this work out for the streaming SDK for example?

Copy link
Member

Choose a reason for hiding this comment

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

It was my understanding that the API should never call the SDK and cannot depend on it. Otherwise it would be impossible to have third party SDKs

Copy link
Member

Choose a reason for hiding this comment

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

The API is functions/methods that are implemented by the SDK, calling API functions results in whatever SDK is being used (if any) being called. Users do not call the SDK directly.

Copy link
Member

Choose a reason for hiding this comment

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

@tsloughter As far as I'm aware, Context is not a part of the SDK but a third top-level item that the API and SDK depend on.

Span and for passing Spans across process boundaries.

When getting the current span, the API MUST return a placeholder Span with an
invalid SpanContext if there is no currently active Span.

### Span Creation

Implementations MUST provide a way to create `Span`s via a `Tracer`. By default,
Expand Down Expand Up @@ -581,7 +587,8 @@ Returns the `StatusCanonicalCode` of this `Status`.
### GetDescription

Returns the description of this `Status`.
Languages should follow their usual conventions on whether to return `null` or an empty string here if no description was given.
Languages should follow their usual conventions on whether to return `null` or
an empty string here if no description was given.

### GetIsOk

Expand Down