This repository has been archived by the owner on Oct 3, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 50
Add details about Sampler interface and sampling decision. #186
Open
bogdandrutu
wants to merge
3
commits into
census-instrumentation:master
Choose a base branch
from
bogdandrutu:samplers
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 hereThere 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:
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:
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)
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.