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

Clarify where Context.Key lives, and API to insert/extract from a Context instance #1020

Closed
wants to merge 4 commits into from
Closed

Clarify where Context.Key lives, and API to insert/extract from a Context instance #1020

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
faa7af0
Clarify where Context.Key lives, and API to insert/extract from a Con…
bogdandrutu Sep 25, 2020
3b9184b
Update based on comments
bogdandrutu Sep 30, 2020
8355e91
Suggest recommended name
bogdandrutu Sep 30, 2020
4a354bd
Update specification/trace/api.md
bogdandrutu Oct 1, 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
13 changes: 10 additions & 3 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,19 +133,26 @@ returned `Tracer`s and actively update their configuration if it changes.
## Tracing Context Utilities

`Tracing Context Utilities` contains all operations within tracing that
modify the [`Context`](../context/context.md).
modify the [`Context`](../context/context.md). It also holds the reference to
the `Context.Key` (the name MAY be `"span_context_key"`).
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear what Context.Key is. How about this?

Suggested change
the `Context.Key` (the name MAY be `"span_context_key"`).
the `Context` key used for storing the currently active span
(usually called `Context.Key` or `span_context_key` in implementations).

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not necessary to store the active Span. Is to store a span in the context. The fact that context is active or not is an orthogonal thing.

Copy link
Member

Choose a reason for hiding this comment

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

What about the "the span active in a particular Context"?

Copy link
Contributor

@anuraaga anuraaga Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I guess reference to context key is too much of an implementation detail. The semantic way of saying it is, "the only way of setting a span in a context is through methods on the trace context utilities". Does that sound right?

Note, of course I'm hoping the "trace context utilities" are allowed to be merged into other concepts like TracerProvider if it makes sense for the language as per my previous comment ;)


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 `Tracing Context Utilities` MUST provide the following functions:

- Get the currently active span
- Set the currently active span
- Get the currently active `Span`
- Set the currently active `Span`
- Extract the `Span` from a `Context` instance
Copy link
Member

Choose a reason for hiding this comment

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

Should we also allow (not necessarily recommend) to just expose the key instead? With that, all the methods provided here should "be equivalent to a single parameterized method call of the Context management system".

Copy link
Member Author

@bogdandrutu bogdandrutu Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We discussed that, please look for history in the context PR why not to do that. So the answer is NO, https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/context.md#create-a-key

- Insert the `Span` to a `Context` instance

The above methods MUST be equivalent to a single parameterized method call of
the [`Context`](../context/context.md) management system.

The last two methods listed above are necessary because API users don't have access to
the `Context.Key` and they need to be able to insert/extract a `Span`
to/from `Context` inside `Propagator` or `Tracer` implementations.

## Tracer

The tracer is responsible for creating `Span`s.
Expand Down