-
Notifications
You must be signed in to change notification settings - Fork 50
Add details about Sampler interface and sampling decision. #186
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
trace/Sampling.md
Outdated
* The current `TraceId`; | ||
* The current `SpanId`; | ||
* The current `Span`'s name; | ||
* The current `Span`'s kind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some implementations also have HasRemoteParent, e.g. https://godoc.org/go.opencensus.io/trace#SamplingParameters. I don't have the historical background what it is but FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After we added the Span.kind that argument is not that useful. We need to decide if we still need it. As the current text mention the least can include more sampling params if needed.
#### Sampling parameters | ||
Based on the language the sampling parameters can be embedded into an object called | ||
`SamplingParameters` or explicitly passed as arguments. The set of sampling parameters SHOULD | ||
include at least: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the mechanism for sampler to set a sampling flag for downstream components? Or sampling score be added in tracestate
? Or who's responsibility is it? Should propagator deal with this? Worth mentioning here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it is a race condition here, what is the order of these events:
- Modify the Tracestate
- Call sampler.
If we call sampler first, we cannot use the current Tracestate (only parent Tracestate) in the decision. If we create the Tracestate first we cannot modify it after the Sampler is called (or I guess we can create it again).
What is your thought on the order of these two events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good document explaining ordering of calls? @lmolkova, you had a picture I remember, can you send it to me pls?
Sampler may take tracestate
object as an argument and modify when wants to.
One may also need to read and modify tracestate before span is created. See #131. This makes matter even more complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmolkova any update on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any pictures about it.
My take on this:
tracing system should have a chance to modify tracestate before sampler makes a decision to support scenarios where sampling is controlled with tracestate (mentioned by Sergey).
I don't see an obvious scenario when tracestate is modified after sampling decision is made or span is started. Do we really want to support this?
Tracing system is the only thing that should touch tracestate and it should have just one opportunity to do so: after span is created, so I don't see how we can have a race condition here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think what I called "race condition" is not the best term. Here are the sequence of events that I can see in time order:
- Tracestate is modified before the sampler gets called as you suggested.
- Sampler gets called.
- Tracestate is modified with sampling results - I heard from @SergeyKanzhelev that here some systems may want to modify the trace state with results from the sampling
Do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also often accidentally say "race condition" when mean "chicken egg"
Though I wonder if tracestate needs to be exposed in-process or not. For example, if it is only expected to be written on remote links, is it necessary for us to write our own here? For example, it seems necessary to write upstream, but possibly not to write our own trace state until the edge where it is exported to headers..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergeyKanzhelev could you please elaborate on the scenarios where sampler needs to update tracestate after sampling decision? (once you are back from vacation)
what's the mechanism for sampler to set a sampling flag for downstream components? Or sampling score be added in tracestate?
Is sampling score a result of sampling decision or parameter sampler takes or both? This is a chichen-egg problem for sure.
@adriancole one of scenarios for tracestate requires in-process exposure and modification. Some systems want to keep (and update) legacy ids at least for a while. assuming we can only access tracestate on the boundaries, all spans in the middle (if any) will not have legacy ids and it'll be hard to discover parents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmolkova @SergeyKanzhelev any conclusion on this? I would like to finish this document.
@adriancole <https://github.com/adriancole> one of scenarios for
tracestate requires in-process exposure and modification. Some systems want
to keep (and update) legacy ids at least for a while. assuming we can only
access tracestate on the boundaries, all spans in the middle (if any) will
not have legacy ids and it'll be hard to discover parents.
ok. thanks. thinking about this.. we have some scenarios like this but it
isnt a user api thing rather an SPI one. I personally would not like to
expose a mutable api for trace state to in routine APIs, but this is
different than wanting to allow hooks to be developed.
… |
PS I think reserializing the tracestate into a string for every
intermediate span will be unnecessary overhead for a lot of folks. We
already imply a lot of overhead to eagerly serialize traceparent.
Consider that between an entry and exit we have arbitrary number of
intermediate steps.
I think this is another case of something we are trying to go to spec
about with very little practice. Is there a way libraries can just
experiment and see if they really need something *before* it goes into
a spec?
…On Fri, Oct 12, 2018 at 11:50 AM Adrian Cole ***@***.***> wrote:
>
> @adriancole one of scenarios for tracestate requires in-process exposure and modification. Some systems want to keep (and update) legacy ids at least for a while. assuming we can only access tracestate on the boundaries, all spans in the middle (if any) will not have legacy ids and it'll be hard to discover parents.
ok. thanks. thinking about this.. we have some scenarios like this but it isnt a user api thing rather an SPI one. I personally would not like to expose a mutable api for trace state to in routine APIs, but this is different than wanting to allow hooks to be developed.
|
No description provided.