Add support for overriding sampling per span #394
Conversation
| */ | ||
| constructor(tracer: types.Tracer, context?: types.TraceOptions) { | ||
| constructor( | ||
| tracer: types.Tracer, name: string, kind: types.SpanKind, traceId: string, |
There was a problem hiding this comment.
Should these new parameters be optional to make this less of a breaking change? If we keep them, could you modify the change list to include a mention of it?
There was a problem hiding this comment.
Possible. On the other hand, this is internal API and won't affect users in any way.
There was a problem hiding this comment.
OK, I see, this does not change the RootSpan interface type, just the implementations. In OpenCensus Web, I create spans directly from performance timings, but it seems like the instrumented Node libraries use the helper utilities in Tracer to create, start and end spans. So makes sense this is not really part of the exported API.
| @@ -232,6 +232,8 @@ export interface TraceOptions { | |||
| spanContext?: SpanContext; | |||
| /** Span kind */ | |||
| kind?: SpanKind; | |||
| /** Determines the sampling rate. Ranges from 0.0 to 1.0 */ | |||
| samplingRate?: number; | |||
There was a problem hiding this comment.
Should this be a Sampler instance per the specs?
There was a problem hiding this comment.
I am happy to change here, but the global sampler takes the same input
To keep things consistent I usenumber. WDYT?
There was a problem hiding this comment.
Yeah, I'm OK with that, but could you create an issue to track changing it in both places to bring it closer to the specs?
| */ | ||
| constructor(tracer: types.Tracer, context?: types.TraceOptions) { | ||
| constructor( | ||
| tracer: types.Tracer, name: string, kind: types.SpanKind, traceId: string, |
There was a problem hiding this comment.
OK, I see, this does not change the RootSpan interface type, just the implementations. In OpenCensus Web, I create spans directly from performance timings, but it seems like the instrumented Node libraries use the helper utilities in Tracer to create, start and end spans. So makes sense this is not really part of the exported API.
mayurkale22
force-pushed
the
sampling_per_span
branch
from
dd493ec
to
9c6d148
Compare
Specs: https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/Sampling.md#how-can-users-control-the-sampler-that-is-used-for-sampling
The logic for allocating
traceIdmoved intracerclass. This helps to make sampling decision first and depending on decision we can then create eitherRecordRootSpan(RootSpan)orNoRecordRootSpaninstead of always creatingRootSpan(just to get traceId for sampling decision) and discarding it later.This introduces signature changes to internal
RootSpanclass, But I think this won't be breaking change for users.Fixes #391