Generalize span operations #209
Conversation
c24t
requested review from
AloisReitbauer,
bogdandrutu,
carlosalberto,
iredelmeier,
reyang,
SergeyKanzhelev,
songy23,
tedsuo and
yurishkuro
as code owners
specification/api-tracing.md
Outdated
| for use-cases when a single instrumentation code runs for multiple deployments. | ||
| The `Tracer` is responsible for tracking the currently active `Span`, and | ||
| exposes methods for creating and activating new `Span`s. The `Tracer` is | ||
| configured with `Propagator`s which support transferring context across process |
There was a problem hiding this comment.
It is unclear whether Propagator will be part of the Tracer.
For example, the OpenCensus Go SDK puts some propagation logic inside integrations https://github.com/census-instrumentation/opencensus-go/tree/master/plugin/ochttp/propagation/b3.
There was a problem hiding this comment.
It is also unclear that whether Propagator will handle SpanContext only, or we intend to handle DistributedContext as well?
There was a problem hiding this comment.
For reference: in Java, the Propagator interface has a generic argument, which can be 'married' to either a SpanContext or a DistributedContext, handling effectively both.
There was a problem hiding this comment.
I agree with removing propagators from the tracer, but since this is part of another (and likely longer) discussion I'd prefer to keep these changes as-is and reword it once we have a decision.
specification/api-tracing.md
Outdated
| exposes methods for creating and activating new `Span`s. The `Tracer` is | ||
| configured with `Propagator`s which support transferring context across process | ||
| boundaries, and `Exporter`s and `SpanProcessor`s which control how spans are | ||
| exported to APMs. |
There was a problem hiding this comment.
SpanProcessor might have other use cases besides exporting data to APMs.
There was a problem hiding this comment.
Changed this to read:
The
Traceris responsible for tracking the currently activeSpan, and exposes methods for creating and activating newSpans. TheTraceris configured withPropagators which support transferring span context across process boundaries,Exporters responsible for exporting span data to APMs and Z pages, andSpanProcessors which affect how spans are exported and allow end users to extend the export pipeline with custom behavior.
Is this what you had in mind?
There was a problem hiding this comment.
Propagator is a bit tricky, we might need to put it as part of the integration (framework, client libs) instead of Tracer.
Take one example, if there is one service which receives incoming HTTP request in Zipkin/B3 format, and makes outgoing gRPC calls using W3C format, how do we expect the user to specify propagators?
Co-Authored-By: Yang Song <songy23@users.noreply.github.com>
|
@reyang if you're ok with deferring the propagator changes this PR should be ready to merge. |
specification/api-tracing.md
Outdated
| Usually this will be the W3C Trace Context as the HTTP text format. For more | ||
| The `Tracer` MUST provide a way to update its active `Span`, and MAY provide | ||
| convenience methods to manage a `Span`'s lifetime and the scope in which a | ||
| `Span` is active. When an active `Span` is finished, the previously-active |
There was a problem hiding this comment.
Activating a Span vs lifetime of the Span are independent.
It is hard to ensure what you say here that:
When an active
Spanis finished, the previously-activeSpanSHOULD be made active.
Span may be active on multiple threads and this is impossible.
So I would actually encourage to say that this should NOT happen.
There was a problem hiding this comment.
Does this capture everything?
The `Tracer` MUST provide a way to update its active `Span`, and MAY provide
convenience methods 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
time) but stil active. A `Span` may be active on one thread after it has been
made inactive on another.
|
@c24t please respond and resolve the last comments still active. I will merge after that. |
* s/content/contents/ * Fix typo * Remove impl-specific details, shuffle text * Shuffle section on multi-deployment runtime * Less flowery language * Backtick fixes * Apply @songy23's suggestions from PR Co-Authored-By: Yang Song <songy23@users.noreply.github.com> * Reword tracer config description * Note that SpanProcessor does more than export * s/set/make/ * Lose refs to exporters, processors * Separate span lifetime and activation
* s/content/contents/ * Fix typo * Remove impl-specific details, shuffle text * Shuffle section on multi-deployment runtime * Less flowery language * Backtick fixes * Apply @songy23's suggestions from PR Co-Authored-By: Yang Song <songy23@users.noreply.github.com> * Reword tracer config description * Note that SpanProcessor does more than export * s/set/make/ * Lose refs to exporters, processors * Separate span lifetime and activation
Like #190, this PR removes references to the java implementation and specific method names in favor of a general description of tracing API.
Updates #165.