Added sampler to Activity - v1 #683
Conversation
cijothomas
requested review from
bruno-garcia,
discostu105,
lmolkova,
MikeGoldsmith and
SergeyKanzhelev
as code owners
| /// </summary> | ||
| /// <param name="parentContext">Parent activity context. Typically taken from the wire.</param> | ||
| /// <param name="traceId">Trace ID of a activity to be created.</param> | ||
| /// <param name="spanId">Span ID of a activity to be created.</param> |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
There was a problem hiding this comment.
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.
@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.
So far my archeology work traced back to OpenCensus 😆
There was a problem hiding this comment.
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.
I've added this topic to Specification SIG meeting 05/26/2020 agenda.
There was a problem hiding this comment.
Discussed during the SIG meeting and agreed to remove it from the spec.
There was a problem hiding this comment.
@cijothomas LGTM with the except of the if on the always sampler and a suggestion to add tests to enforce the behavior of the samplers.
| { | ||
| /// <summary> | ||
| /// Sampler implementation which will sample in all the spans. | ||
| /// This sampler will be used as the default Sampler, if no other Sampler is configured. |
There was a problem hiding this comment.
nit: this comment should be on the builder to increase visibility.
|
@pjanotti Addressed comments, added test. Also added followup for "spanid" of Activity yet to be created issue. |
There was a problem hiding this comment.
@cijothomas LGTM it is at a good point to merge we will build on top of it.
Following step1(#660) in the direction of replacing OT Span with .NET Activity APIs, this PR modifies OpenTelemetryBuilder to support sampling.
This is v1, so I added AlwaysOn, AlwaysOff samplers only.
This addresses 2a from #684
Checklist
For significant contributions please make sure you have completed the following items:
The PR will automatically request reviews from code owners and trigger CI build and tests.