Add specs on Span creation. #124
Conversation
songy23
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
reyang,
SergeyKanzhelev,
tedsuo and
yurishkuro
as code owners
|
On a second thought I found it's easier to describe span creation in a more language-agnostic way, so I removed |
specification/tracing-api.md
Outdated
| - The option to become a root `Span` for a new trace. | ||
| If not set, the value of [Tracer.getCurrentSpan](#getcurrentspan) at `StartSpan` | ||
| time will be used as parent. | ||
| - `Sampler` to the newly created `Span`. If not set, the implementation should provide a default. |
There was a problem hiding this comment.
If not set, the implementation should provide a default.
hm... it feels like we missed to add a sampler getter to the tracer. I think Tracer's sampler will be used as a default.
There was a problem hiding this comment.
I think default sampler is a SDK issue rather than API? (#33)
There was a problem hiding this comment.
Access to sampler is needed if you plan to record spans using recordSpanData. I'll file an issue
There was a problem hiding this comment.
here I'd suggest replace "default" with "default sampler used by Tracer" or simply "Tracer-defined sampler".
specification/tracing-api.md
Outdated
| If not set, the value of [Tracer.getCurrentSpan](#getcurrentspan) at `StartSpan` | ||
| time will be used as parent. | ||
| - `Sampler` to the newly created `Span`. If not set, the implementation should provide a default. | ||
| - `Link` to the newly created `Span`. |
There was a problem hiding this comment.
it looks like a copy/paste issue. You may say "collection of Links that will be associated with the newly created Span"
There was a problem hiding this comment.
You're right, this is copied from the Javadoc of addLink (while addLink can be called multiple times). Updated.
specification/tracing-api.md
Outdated
|
|
||
| Starts a new `Span`. | ||
|
|
||
| This MUST not be called multiple times with `Builder` pattern. |
There was a problem hiding this comment.
what will it return? Should we say that the same Span will be returned on every call to this method?
There was a problem hiding this comment.
That sounds a better statement for Builder pattern - updated. (Note in languages that uses Options, each call will create a new Span.)
Co-Authored-By: Sergey Kanzhelev <[email protected]>
| when manual Context propagation is used OR when creating a root `Span` with | ||
| a parent with an invalid `SpanContext`. | ||
| - Parent `SpanContext`. If not set, the value of [Tracer.getCurrentSpan](#getcurrentspan) | ||
| at `StartSpan` time will be used as parent. MUST be used to create a `Span` |
There was a problem hiding this comment.
you can only use Span or SpanContext but not both. If both is used then should it return an error, or one takes precedence over the other.
There was a problem hiding this comment.
Agreed, at least for the non-builder pattern (as for the builder pattern it simply overrides any previous value).
| - Parent `SpanContext`. If not set, the value of [Tracer.getCurrentSpan](#getcurrentspan) | ||
| at `StartSpan` time will be used as parent. MUST be used to create a `Span` | ||
| when the parent is in a different process. | ||
| - The option to become a root `Span` for a new trace. |
There was a problem hiding this comment.
If there is an option to make it a root Span then in the previous option (Span and SpanContext) it should not be 'MUST' in "MUST be used to create a Span when creating a root Span with a parent with an invalid SpanContext.
I think if an option is provided to make it a root span then parent span/span context should be ignored. There should not be an option to pass Invalid Span or Invalid SpanContext to create a root span. Just provide one way.
There was a problem hiding this comment.
Agreed as well, also for the non-builder pattern ;)
There was a problem hiding this comment.
I think if an option is provided to make it a root span then parent span/span context should be ignored.
Good catch, and that's consistent with current Java APIs. Created #131 to clarify.
|
I'm merging. Please address extra feedback as a separate PR with improvements |
* Add specs on Span creation. Fixes open-telemetry#37. * Put back Tracer.SpanBuilder * s/called/set Co-Authored-By: Sergey Kanzhelev <[email protected]> * Fix spaces. Add a note about span creation from Tracer. * Update recordEvents and StartSpan. * Update default samplers, links and StartSpan.
* Add compare operators to nostd::string_view * Added tests
Also update limit unset value to `""`
* Add specs on Span creation. Fixes open-telemetry#37. * Put back Tracer.SpanBuilder * s/called/set Co-Authored-By: Sergey Kanzhelev <[email protected]> * Fix spaces. Add a note about span creation from Tracer. * Update recordEvents and StartSpan. * Update default samplers, links and StartSpan.
Fixes #37.