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

Move context utils outside of the Tracer #527

Closed
wants to merge 9 commits into from
56 changes: 31 additions & 25 deletions specification/api-tracing.md
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` main responsibility it to allow creating `Spans`.
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

### 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,32 @@ 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
Tracing API is responsible to provide functionality to track the currently
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
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 to (depending on the language, global functions
or util class e.g. `TracingContextUtils`):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

also s/TracingContextUtils/TracingContext/

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need something as suffix or prefix if we want to be consistent for CorrelationContext which has Context at the end, because will be ugly to name that CorrelationContextContext :(. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

If context interaction is done via static util class, then TracingContext.activeSpan() looks a lot better than TracingContextUtils.activeSpan().

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree with you but I am trying to think how would I name this class for "CorrelationContext" package. If I find a good name for that and ensure consistency I am all to remove Utils.

Copy link
Member

Choose a reason for hiding this comment

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

CorrelationContext can remain CorrelationContext. It can represent both the object that stores correlations and the namespace for static functions.

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'd be up for not using TraceContext and keeping the activation bits in CorrelationContext - we will end up with too many classes having the Context suffix (some of them doing different things, i.e. TraceContext doing activation handling only, CorrelationContext doing both activation handling PLUS as a container of key-values).

There was already some confusion in the past because of the fact we had both CorrelationContext and Context, so this could get worse ;)


* Get the currently active Span
* Make a given Span as active
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

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 state and how Spans are passed across process boundaries.
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

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

When an active Span is made inactive, the previously-active Span SHOULD be made
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
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.

### Span Creation

Implementations MUST provide a way to create `Span`s via a `Tracer`. By default,
Expand Down Expand Up @@ -581,7 +586,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