Skip to content
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

feat: support TraceState in SamplingResult #3530

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
177931d
feat(sampling): add traceState field to SamplingResult
raphael-theriault-swi Jan 11, 2023
c709487
feat(tracer): use SamplingResult's traceState
raphael-theriault-swi Jan 11, 2023
af52040
feat(sampling): forward traceState in builtin samplers
raphael-theriault-swi Jan 11, 2023
467b282
test(sampling): update existing sampler tests with traceState
raphael-theriault-swi Jan 12, 2023
e008229
test(sampling): builtin samplers should forward traceState
raphael-theriault-swi Jan 12, 2023
a886d08
test(tracer): spans should use traceState from sampling result
raphael-theriault-swi Jan 12, 2023
861adce
chore: update changelog
raphael-theriault-swi Jan 12, 2023
11e8265
Merge branch 'main' into samplingresult-tracestate
dyladan Jan 13, 2023
c4f174b
fix(opentelemetry-core): traceState in sampler tests
raphael-theriault-swi Jan 13, 2023
5527f6d
fix(sampling): better specify the behaviour of traceState in Sampling…
raphael-theriault-swi Jan 13, 2023
ac2bc02
fix(sampling-result): clarify where the tracestate should originate from
raphael-theriault-swi Jan 18, 2023
3aae7ee
Merge branch 'main' into samplingresult-tracestate
Flarna Jan 30, 2023
b45c670
Revert "feat(sampling): forward traceState in builtin samplers"
raphael-theriault-swi Feb 2, 2023
3099265
Revert "test(sampling): builtin samplers should forward traceState"
raphael-theriault-swi Feb 2, 2023
b51fdea
Revert "test(sampling): update existing sampler tests with traceState"
raphael-theriault-swi Feb 2, 2023
a27dbac
Revert "fix(opentelemetry-core): traceState in sampler tests"
raphael-theriault-swi Feb 2, 2023
6573804
merge main into samplingresult-tracestate
raphael-theriault-swi Feb 2, 2023
c5526fa
Merge branch 'main' into samplingresult-tracestate
legendecas Feb 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :rocket: (Enhancement)

* feat: support TraceState in SamplingResult [#3530](https://github.com/open-telemetry/opentelemetry-js/pull/3530) @raphael-theriault-swi

### :bug: (Bug Fix)

### :books: (Refine Doc)
Expand Down
7 changes: 7 additions & 0 deletions api/src/trace/SamplingResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import { SpanAttributes } from './attributes';
import { TraceState } from './trace_state';

/**
* @deprecated use the one declared in @opentelemetry/sdk-trace-base instead.
Expand Down Expand Up @@ -55,4 +56,10 @@ export interface SamplingResult {
* can safely cache the returned value.
*/
attributes?: Readonly<SpanAttributes>;
/**
* A {@link TraceState} that will be associated with the {@link Span}
* through the new {@link SpanContext}. Samplers SHOULD return the
* passed-in {@link TraceState} if they do not intend to change it.
*/
traceState?: TraceState;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,23 @@
* limitations under the License.
*/

import { Sampler, SamplingDecision, SamplingResult } from '@opentelemetry/api';
import {
Context,
Sampler,
SamplingDecision,
SamplingResult,
trace,
} from '@opentelemetry/api';

/**
* @deprecated Use the one defined in @opentelemetry/sdk-trace-base instead.
* Sampler that samples no traces.
*/
export class AlwaysOffSampler implements Sampler {
shouldSample(): SamplingResult {
shouldSample(context: Context): SamplingResult {
return {
decision: SamplingDecision.NOT_RECORD,
traceState: trace.getSpanContext(context)?.traceState,
};
}

Expand Down
11 changes: 9 additions & 2 deletions packages/opentelemetry-core/src/trace/sampler/AlwaysOnSampler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,23 @@
* limitations under the License.
*/

import { Sampler, SamplingDecision, SamplingResult } from '@opentelemetry/api';
import {
Context,
Sampler,
SamplingDecision,
SamplingResult,
trace,
} from '@opentelemetry/api';

/**
* @deprecated Use the one defined in @opentelemetry/sdk-trace-base instead.
* Sampler that samples all traces.
*/
export class AlwaysOnSampler implements Sampler {
shouldSample(): SamplingResult {
shouldSample(context: Context): SamplingResult {
return {
decision: SamplingDecision.RECORD_AND_SAMPLED,
traceState: trace.getSpanContext(context)?.traceState,
legendecas marked this conversation as resolved.
Show resolved Hide resolved
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
SamplingDecision,
SamplingResult,
isValidTraceId,
Context,
trace,
} from '@opentelemetry/api';

/**
Expand All @@ -33,12 +35,13 @@ export class TraceIdRatioBasedSampler implements Sampler {
this._upperBound = Math.floor(this._ratio * 0xffffffff);
}

shouldSample(context: unknown, traceId: string): SamplingResult {
shouldSample(context: Context, traceId: string): SamplingResult {
return {
decision:
isValidTraceId(traceId) && this._accumulate(traceId) < this._upperBound
? SamplingDecision.RECORD_AND_SAMPLED
: SamplingDecision.NOT_RECORD,
traceState: trace.getSpanContext(context)?.traceState,
};
}

Expand Down
14 changes: 13 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/Sampler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@
* limitations under the License.
*/

import { Context, Link, SpanAttributes, SpanKind } from '@opentelemetry/api';
import {
Context,
Link,
SpanAttributes,
SpanKind,
TraceState,
} from '@opentelemetry/api';

/**
* A sampling decision that determines how a {@link Span} will be recorded
Expand Down Expand Up @@ -53,6 +59,12 @@ export interface SamplingResult {
* can safely cache the returned value.
*/
attributes?: Readonly<SpanAttributes>;
/**
* A {@link TraceState} that will be associated with the {@link Span}
* through the new {@link SpanContext}. Samplers SHOULD return the
* passed-in {@link TraceState} if they do not intend to change it.
*/
traceState?: TraceState;
}

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/opentelemetry-sdk-trace-base/src/Tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ export class Tracer implements api.Tracer {
links
);

// according to the spec an empty Tracestate means it should be cleared but
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this depends on how you define empty tracestate.
I would interpret new TraceState() as empty and this is working as specified.

not interpreting samplingResult.traceState == null sounds correct to me and not just a backward compat solution.

Copy link
Contributor Author

@raphael-theriault-swi raphael-theriault-swi Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to handle null given the same can be done by passing the result of createTraceState ? It's shorter but I don't remember seeing null anywhere else in the API.

Either way agreed the comment isn't great, and the behaviour should probably also be described in the new field's doc comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add special handling for null. I used == null above which includes null and undefined like the ?? operator used in your change.

If a sampler want's to clear the TraceState it should return an empty instance by calling api.createTraceState().
But I would expect that a sample is usually not clearing the whole TraceState as it may contain arbitrary keys not related to sampling. Instead I assume they only set/clear/update sampler specific keys like the ot key used by consistent probability samplers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, so the behaviour should already be correct. I updated the comments.

// to maintain backwards compatibility we ignore empty values
traceState = samplingResult.traceState ?? traceState;

const traceFlags =
samplingResult.decision === api.SamplingDecision.RECORD_AND_SAMPLED
? api.TraceFlags.SAMPLED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@
* limitations under the License.
*/

import { Context, trace } from '@opentelemetry/api';
import { Sampler, SamplingDecision, SamplingResult } from '../Sampler';

/** Sampler that samples no traces. */
export class AlwaysOffSampler implements Sampler {
shouldSample(): SamplingResult {
shouldSample(context: Context): SamplingResult {
return {
decision: SamplingDecision.NOT_RECORD,
traceState: trace.getSpanContext(context)?.traceState,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@
* limitations under the License.
*/

import { Context, trace } from '@opentelemetry/api';
import { Sampler, SamplingDecision, SamplingResult } from '../Sampler';

/** Sampler that samples all traces. */
export class AlwaysOnSampler implements Sampler {
shouldSample(): SamplingResult {
shouldSample(context: Context): SamplingResult {
return {
decision: SamplingDecision.RECORD_AND_SAMPLED,
traceState: trace.getSpanContext(context)?.traceState,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { isValidTraceId } from '@opentelemetry/api';
import { Context, isValidTraceId, trace } from '@opentelemetry/api';
import { Sampler, SamplingDecision, SamplingResult } from '../Sampler';

/** Sampler that samples a given fraction of traces based of trace id deterministically. */
Expand All @@ -26,12 +26,13 @@ export class TraceIdRatioBasedSampler implements Sampler {
this._upperBound = Math.floor(this._ratio * 0xffffffff);
}

shouldSample(context: unknown, traceId: string): SamplingResult {
shouldSample(context: Context, traceId: string): SamplingResult {
return {
decision:
isValidTraceId(traceId) && this._accumulate(traceId) < this._upperBound
? SamplingDecision.RECORD_AND_SAMPLED
: SamplingDecision.NOT_RECORD,
traceState: trace.getSpanContext(context)?.traceState,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ import {
Context,
context,
createContextKey,
createTraceState,
INVALID_TRACEID,
Link,
ROOT_CONTEXT,
SpanContext,
SpanKind,
trace,
TraceFlags,
TraceState,
} from '@opentelemetry/api';
import { getSpan } from '@opentelemetry/api/build/src/trace/context-utils';
import {
Expand Down Expand Up @@ -57,6 +59,8 @@ describe('Tracer', () => {
}

class TestSampler implements Sampler {
constructor(private readonly traceState?: TraceState) {}

shouldSample(
_context: Context,
_traceId: string,
Expand All @@ -80,6 +84,7 @@ describe('Tracer', () => {
// invalid attributes should be sanitized.
...invalidAttributes,
} as unknown as SpanAttributes,
traceState: this.traceState,
};
}
}
Expand Down Expand Up @@ -160,6 +165,17 @@ describe('Tracer', () => {
span.end();
});

it('should start a span with traceState in sampling result', () => {
const traceState = createTraceState();
const tracer = new Tracer(
{ name: 'default', version: '0.0.1' },
{ sampler: new TestSampler(traceState) },
tracerProvider
);
const span = tracer.startSpan('stateSpan');
assert.strictEqual(span.spanContext().traceState, traceState);
});

it('should have an instrumentationLibrary', () => {
const tracer = new Tracer(
{ name: 'default', version: '0.0.1' },
Expand Down Expand Up @@ -192,11 +208,13 @@ describe('Tracer', () => {
});
});

it('should use traceId and spanId from parent', () => {
it('should use traceId, spanId and traceState from parent', () => {
const traceState = createTraceState();
const parent: SpanContext = {
traceId: '00112233445566778899001122334455',
spanId: '0011223344556677',
traceFlags: TraceFlags.SAMPLED,
traceState,
};
const tracer = new Tracer(
{ name: 'default', version: '0.0.1' },
Expand All @@ -210,6 +228,7 @@ describe('Tracer', () => {
);
assert.strictEqual((span as Span).parentSpanId, parent.spanId);
assert.strictEqual(span.spanContext().traceId, parent.traceId);
assert.strictEqual(span.spanContext().traceState, traceState);
});

it('should not use spanId from invalid parent', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,23 @@ describe('AlwaysOffSampler', () => {

it('should return decision: api.SamplingDecision.NOT_RECORD for AlwaysOffSampler', () => {
const sampler = new AlwaysOffSampler();
assert.deepStrictEqual(sampler.shouldSample(), {
assert.deepStrictEqual(sampler.shouldSample(api.ROOT_CONTEXT), {
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
});
});

it('should forward the traceState', () => {
const sampler = new AlwaysOffSampler();
const traceState = api.createTraceState();
assert.strictEqual(
sampler.shouldSample(
api.trace.setSpanContext(api.ROOT_CONTEXT, {
...api.INVALID_SPAN_CONTEXT,
traceState,
})
).traceState,
traceState
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,23 @@ describe('AlwaysOnSampler', () => {

it('should return api.SamplingDecision.RECORD_AND_SAMPLED for AlwaysOnSampler', () => {
const sampler = new AlwaysOnSampler();
assert.deepStrictEqual(sampler.shouldSample(), {
assert.deepStrictEqual(sampler.shouldSample(api.ROOT_CONTEXT), {
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
});
});

it('should forward the traceState', () => {
const sampler = new AlwaysOnSampler();
const traceState = api.createTraceState();
assert.deepStrictEqual(
sampler.shouldSample(
api.trace.setSpanContext(api.ROOT_CONTEXT, {
...api.INVALID_SPAN_CONTEXT,
traceState,
})
).traceState,
traceState
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -87,6 +88,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -105,6 +107,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -128,6 +131,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -146,6 +150,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -164,6 +169,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand Down
Loading