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

Remove restriction that SpanContext must be a final class, specify creation. #969

Merged
merged 12 commits into from
Sep 26, 2020
Merged

Remove restriction that SpanContext must be a final class, specify creation. #969

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
09c18ec
Remove restriction that SpanContext must be a final class
Sep 18, 2020
e5a7786
Methods for SpanContext.
Sep 19, 2020
9bd8dd7
Lint
Sep 19, 2020
1aa7d16
No bookmark?
Sep 19, 2020
e9fafdb
Root span needs sampling
Sep 23, 2020
b04b3d4
Non-root needs sampling too
Sep 23, 2020
7fb7b4b
API-only
Sep 24, 2020
5b5a5df
Update specification/trace/api.md
Sep 24, 2020
ca2a68b
Update specification/trace/api.md
Sep 24, 2020
6c37693
Note
Sep 24, 2020
d36af6c
less is more
Sep 24, 2020
c18cabc
Update specification/trace/api.md
Sep 24, 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
11 changes: 10 additions & 1 deletion specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ the same `TracerProvider`.

A `SpanContext` represents the portion of a `Span` which must be serialized and
propagated along side of a distributed context. `SpanContext`s are immutable.
`SpanContext` MUST be a final (sealed) class.

The OpenTelemetry `SpanContext` representation conforms to the [W3C TraceContext
specification](https://www.w3.org/TR/trace-context/). It contains two
Expand All @@ -180,6 +179,16 @@ of key-value pairs. TraceState allows multiple tracing
systems to participate in the same trace. It is fully described in the [W3C Trace Context
specification](https://www.w3.org/TR/trace-context/#tracestate-header).

The API must provide methods to create `SpanContext`. There are three ways to create
a `SpanContext`
Copy link
Member

@Oberon00 Oberon00 Sep 19, 2020 •
edited
Loading

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 create `SpanContext`. There are three ways to create
a `SpanContext`
The API MUST implement methods to create `SpanContext` which MUST NOT require any other API/SDK object
(i.e. they SHOULD be provided as free functions, or as static functions/constructors in the ` SpanContext` interface/class).
The produced SpanContext implementation when no API implementation is available
MUST be interchangeable with the SpanContext produced after installing any SDK.
There are three ways to create a `SpanContext`:

or, what I would prefer but it's probably too late for that rather large change:

Suggested change
The API must provide methods to create `SpanContext`. There are three ways to create
a `SpanContext`
The API MUST provide methods to create a `SpanContext` object on the `Tracer`.
This MUST be the only way to create a `SpanContext`.
For example, there must not be publicly available constructors on the `SpanContext`.
This could, for example, be achieved by having the `SpanContext` type be an interface or abstract base class in the API.
There are three ways to create a `SpanContext`:


- By generating a new `SpanId` and `TraceId` when there is no in-process or remote parent.
Copy link
Member

@Oberon00 Oberon00 Sep 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There should be a way to provide TraceFlags and TraceState for root spans too. Actually at least TraceFlags should be a required parameter (sampled vs unsampled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - yeah I added the point about TraceFlags, I don't think we currently support TraceState. I'd like to keep this PR to reflect what we currently do if it's ok. I'd also like to avoid getting into too much detail about implementation, for example, SamplingContext.newForSampler(Sampler) and generate all IDs / trace flags and then call SpanContext.of(...) are equivalent IMO as far as the spec is concerned. I have found a general theme of over-specifying implementation detail in the spec even though it's too different based on language idioms.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we currently support TraceState

We absolutely do. @iNikem added some descriptions how a tracestate can be "modifified" recently (the actual object instances are immutable but you can create a modified copy). Of course there must be some way of associating a tracestate with a spancontext too.

- By generating a new `SpanId` and copying the `TraceId`, `TraceFlags`, and `TraceState` from
Copy link
Member

@Oberon00 Oberon00 Sep 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There must definitely be a way to change the TraceFlags (sampled) at this point. I think there should also be a way to change the TraceState. Maybe we could cover this by adding an additional mode of creation that accepts only a TraceState + original SpanContext and leaves everything else (including the "remoteness") unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup makes sense. I think #988 addresses being able to mutate TraceState, if it's ok want to keep this reflecting the current state.

Copy link
Member

@Oberon00 Oberon00 Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

#998 would allow the sampler to choose a tracestate, but the SDK would then need to construct a new SpanContext with that tracestate, which will only work if there is an option to create a SpanContext with a tracestate. I also want to keep reflecting the current state, which is, for example in Java, that you have the option (actually the obligation) to pass a tracestate, no matter whether the spancontext is remote or local.

an in-process parent.
- By accepting values for the `SpanId`, `TraceId`, `TraceFlags`, and `TraceState` for a remote
parent. In this case, the `SpanId`, `TraceId`, and `TraceFlags` must be used as is, and
`TraceState` may be modified based on the rules for modification described below.
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved

### Retrieving the TraceId and SpanId

The API MUST allow retrieving the `TraceId` and `SpanId` in the following forms:
Expand Down