Skip to content

Clarify context interaction for trace module #1063

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

Merged
merged 11 commits into from
Oct 15, 2020
Merged

Clarify context interaction for trace module #1063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cf67c3d
Clarify context interaction for trace module
bogdandrutu Oct 6, 2020
682cefa
Add changelog entry
bogdandrutu Oct 6, 2020
db1ea77
Update specification/trace/api.md
bogdandrutu Oct 7, 2020
e03ec77
Update specification/trace/api.md
bogdandrutu Oct 7, 2020
dd12c37
Update specification/trace/api.md
bogdandrutu Oct 7, 2020
b7f2152
Update specification/trace/api.md
bogdandrutu Oct 7, 2020
16e17ec
Update specification/trace/api.md
bogdandrutu Oct 7, 2020
ce0c497
Update api.md
bogdandrutu Oct 15, 2020
74d72de
Update api.md
bogdandrutu Oct 15, 2020
d027841
Merge branch 'master' into newcontextkey
bogdandrutu Oct 15, 2020
58ce14d
Update specification/trace/api.md
bogdandrutu Oct 15, 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
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ New:
([#942](https://github.com/open-telemetry/opentelemetry-specification/pull/942))
- Add Metric SDK specification (partial): covering terminology and Accumulator component
([#626](https://github.com/open-telemetry/opentelemetry-specification/pull/626))
- Add `Shutdown` function to `*Provider` SDK ([#1074](https://github.com/open-telemetry/opentelemetry-specification/pull/1074))
- Clarify context interaction for trace module
([#1063](https://github.com/open-telemetry/opentelemetry-specification/pull/1063))
- Add `Shutdown` function to `*Provider` SDK
([#1074](https://github.com/open-telemetry/opentelemetry-specification/pull/1074))

Updates:

Expand Down
40 changes: 22 additions & 18 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Table of Contents
* [Duration](#duration)
* [TracerProvider](#tracerprovider)
* [TracerProvider operations](#tracerprovider-operations)
* [Tracing Context Utilities](#tracing-context-utilities)
* [Context Interaction](#context-interaction)
* [Tracer](#tracer)
* [Tracer operations](#tracer-operations)
* [SpanReference](#spanreference)
Expand Down Expand Up @@ -127,21 +127,32 @@ the tracer could, for example, do a look-up with its name+version in a map in
the `TracerProvider`, or the `TracerProvider` could maintain a registry of all
returned `Tracer`s and actively update their configuration if it changes.

## Tracing Context Utilities
## Context Interaction

`Tracing Context Utilities` contains all operations within tracing that
modify the [`Context`](../context/context.md).
This section defines all operations within the Tracing API that interact with the
[`Context`](../context/context.md).

As these utilities operate solely on the context API, they MAY be exposed
as static methods on the trace module instead of a class.
The API MUST provide the following functionality to interact with a `Context`
instance:

The `Tracing Context Utilities` MUST provide the following functions:
- Extract the `Span` from a `Context` instance
- Insert the `Span` to a `Context` instance

- Get the currently active span
- Set the currently active span
The functionality listed above is necessary because API users SHOULD NOT have
access to the [Context Key](../context/context.md#create-a-key) used by the Tracing API implementation.

The above methods MUST be equivalent to a single parameterized method call of
the [`Context`](../context/context.md) management system.
If the language has support for implicitly propagated `Context` (see
[here](../context/context.md#optional-global-operations)), the API SHOULD also provide
the following functionality:

- Get the currently active span from the implicit context. This is equivalent to getting the implicit context, then extracting the `Span` from the context.
- Set the currently active span to the implicit context. This is equivalent to getting the implicit context, then inserting the `Span` to the context.

All the above functionalities operate solely on the context API, and they MAY be
exposed as static methods on the trace module, as static methods on a class
inside the trace module (it MAY be named `TracingContextUtilities`), or on the
[`Tracer`](#tracer) class. This functionality SHOULD be fully implemented in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking news - there seems to be a lot of support for removing the mention of Tracer here. What do you think?

open-telemetry/opentelemetry-java#1807

Copy link
Member

Choose a reason for hiding this comment

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

The spec here does not say that this should be on the tracer, and if it is, it says it should be static.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah static is written twice, but not a third time. So with this wording, static does not apply to Tracer I think and I didn't read it that way. If it's also static it makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

What one language decided to do is not breaking news :) In the Ruby implementation they want to work directly with Tracer (cc @mwear). The options are given above, and the language is free to implement them either way they want.

@bogdandrutu to just clarify, we still allow this to exist as instance methods in Tracer, correct?

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 we should discourage an instance method on tracer, as this has been known to cause confusion and has caused people to call for allowing an unnamed tracer just to access the current span.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should discourage an instance method on tracer

I agree.

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 the current text does exactly that, and in edge cases where that cannot be avoided, it allows to be an instance method.

Copy link
Contributor

Choose a reason for hiding this comment

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

open-telemetry/opentelemetry-ruby#425 (comment)

TL;DR: I agree, including for Ruby.

API when possible.

## Tracer

Expand All @@ -156,13 +167,6 @@ The `Tracer` MUST provide functions to:

- [Create a new `Span`](#span-creation) (see the section on `Span`)

The `Tracer` MAY provide functions to:

- Get the currently active span
- Set the currently active span

These functions MUST delegate to the `Tracing Context Utilities`.

## SpanReference

A `SpanReference` represents the portion of a `Span` which must be serialized and
Expand Down