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.
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.
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
Added sampler to Activity - v1 #683
Added sampler to Activity - v1 #683
Changes from 7 commits
c44d367
a3552b0
a6b03d0
6407145
c93cb01
c7c3205
8f17e1b
031e673
6bcca6c
70b96d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 wonder whats the value of spanid here - the span/activity is not yet created, and will be done based on the outcome of this method.
Can't think of the use of spanid in a typical sampling algorithm, as new spanid is just random.
(With activity there is no option to know the spanid ahead of Activity creation, and no option to change spanid after creation).
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.
IIRC an OTel SDK should be able to generate the IDs before calling the sampler, and historically some distributed tracing systems actually used the span ID instead of the trace ID to make their decision.
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.
If this is a requirement, we need to address this,
Adding it here for tracking: #675 (comment)
cc: @tarekgh
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 am wondering how a SpanId (for a span which not created yet) will be used in the sampling decision? this looks weird to me as this Id never been used till the sampling time. do we expect the samplers recognize specific patterns of Span Ids to make the decision accordingly? I can understand it could make sense to use the Span Id for sampling if the span is already created but not if the span really not created at all.
@cijothomas maybe the trace id is interesting too here.
@noahfalk
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.
since traceid for parent, and the child-to-be-created are going to be the same, I dont see how traceid is an issue?
Unless parent is null?
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.
@reyang here and there I tried to dig something about this and I didn't see any PR/discussion mentioning the
SpanID
perhaps it was an oversight? Unless @SergeyKanzhelev has something to add we should propose a simple change of removing it to see if anyone complains :)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 far my archeology work traced back to OpenCensus 😆
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.
yeah 🤠 (they didn't have an Indiana Jones emoji but the cowboy has a hat 🤷♂️), it seems very much like OpenCensus - that said everything that I see is using traceId to make their decisions, now will be the moment to remove it before some "creative person" starts to depend on it :)
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've added this topic to Specification SIG meeting 05/26/2020 agenda.
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.
Discussed during the SIG meeting and agreed to remove it from the spec.
open-telemetry/opentelemetry-specification#621
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.
nit: this comment should be on the builder to increase visibility.