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

distinguish starting an active span and creating an inactive span #485

Closed
wants to merge 9 commits into from
Closed

distinguish starting an active span and creating an inactive span #485

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
240aeb7
distinguish starting an active span and creating an inactive span
tsloughter Feb 22, 2020
bb813e8
remove use of 'created span' and simply call it 'start inactive'
tsloughter Feb 22, 2020
f026458
Update specification/api-tracing.md
tsloughter Feb 26, 2020
a48c99a
remove talk of creating a span, only start which is active or inactive
tsloughter Feb 26, 2020
91a7830
move paragraph on use of async callbacks to Tracer operations
tsloughter Feb 26, 2020
c3e88b2
move 'make given span active' operation to a MUST
tsloughter Mar 10, 2020
d436555
Update specification/api-tracing.md
tsloughter Mar 11, 2020
82cef8c
update based on yuri and carlos suggestions
tsloughter Apr 28, 2020
bec2d01
make start inactive span the required operation
tsloughter Apr 28, 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
35 changes: 20 additions & 15 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,31 +120,41 @@ mechanism, for instance the `ServiceLoader` class in Java.

### Tracer operations

The currently active `Span` is the one that is tracked in the current `Context`
by the `Tracer`. An inactive `Span` is not currently tracked in any `Context`.

The `Tracer` MUST provide functions to:

- Create a new `Span`
- Start a new inactive `Span`

The `Tracer` SHOULD provide methods to:

- Start a new active `Span`
- Get the currently active `Span`
- Make a given `Span` as active
- Make a given `Span` active
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 there is still some vagueness here. I have users at Uber who are absolutely opposed to thread-local based context propagation, and instead insist that in their services all context propagation is done explicitly, similar to Go. That means that while they may still use the same Context implementation, the three SHOULD operations here are against the explicitly passed Context object, rather than building an assumption that a thread-local mechanism is being used and understood by the tracer. In other words, the "Get the currently active Span" is equivalent to this function in OpenTracing Go:

func SpanFromContext(ctx context.Context) Span {
	val := ctx.Value(activeSpanKey)
	if sp, ok := val.(Span); ok {
		return sp
	}
	return nil
}

Which opens up another trail of questions: why does this need to be a functionality of the Tracer? Tracer is an API that can be implemented differently. Accessing spans in the Context is not related to specific tracer implementation, it's a common behavior at the API level.

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 consider it part of the Tracer functionality because the Tracer is what knows where its data is stored in the context.

In your example from OpenTracing that would be activeSpanKey.

Copy link
Member

Choose a reason for hiding this comment

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

In OpenTracing it's not part of Tracer interface, it's a shared static function. If it was part of Tracer, then every tracer implementation would have to implement it identically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming Tracers from multiple Tracer Providers in a single application are meant to be able to read traces from the same contexts, yes they'd have to use the same key.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, wouldn't we want tracers from different providers to be able to act separate and not clash?

Copy link
Contributor

Choose a reason for hiding this comment

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

Observe this will probably go away with #527 - Maybe hold on till that one is resolved?

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 doesn't really change this PR much since it is mainly about starting the spans and #527 isn't. Having re-read it I'm not opposed to #527 anymore either, I had thought it also moved starting a span to "utils".

The change in this PR is adding an additional function for starting a span, the "make span active" already exists in the spec and is not changed by this PR. So I think this should be considered separate.

And don't think the function to make a span active should hold up this PR since it already exists in the spec and is not being changed here.


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.
currently active `Span` and how `Span`s are passed across process boundaries. A
`Span` that is started but inactive is not tracked in the `Context`.

A common case of starting an inactive `Span` is with asynchronous
callbacks. Before the callback is set up, an inactive `Span` is started, with
the currently active `Span`, if one exists, as the parent. This new `Span` is
passed to the callback as an argument and to be set as active within the body of
the callback when it is run.

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.
When starting a new `Span`, the `Tracer` MUST allow the caller to specify the
new `Span`'s parent in the form of a `Span` or `SpanContext`. See [Determining
the Parent Span from a Context](#determining-the-parent-span-from-a-context) for
how parent is deteremined if it is not provided as an argument.

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
`Span` SHOULD be made active. A `Span` may be 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.

Expand Down Expand Up @@ -179,7 +189,6 @@ TraceID and a non-zero SpanID.

`IsRemote` is a boolean flag which returns true if the SpanContext was propagated
from a remote parent.
When creating children from remote spans, their IsRemote flag MUST be set to false.

Please review the W3C specification for details on the [Tracestate
field](https://www.w3.org/TR/trace-context/#tracestate-field).
Expand Down Expand Up @@ -244,11 +253,7 @@ Implementations MUST provide a way to create `Span`s via a `Tracer`. By default,
the currently active `Span` is set as the new `Span`'s parent. The `Tracer`
MAY provide other default options for newly created `Span`s.

`Span` creation MUST NOT set the newly created `Span` as the currently
active `Span` by default, but this functionality MAY be offered additionally
as a separate operation.

The API MUST accept the following parameters:
The API functions for starting a `Span` MUST accept the following parameters:

- The span name. This is a required parameter.
- The parent `Span` or a `Context` containing a parent `Span` or `SpanContext`,
Expand Down