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.

Sign up for GitHub

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

Jump to bottom

feat: support TraceState in SamplingResult #3530

Merged
merged 18 commits into from
Feb 9, 2023
Merged

feat: support TraceState in SamplingResult #3530

Show file tree
Hide file tree
Changes from 10 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
View file
Open in desktop
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
8 changes: 8 additions & 0 deletions api/src/trace/SamplingResult.ts
View file
Open in desktop
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,11 @@ 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
* TraceState if they do not intend to change it. Leaving the value undefined
* will also leave the TraceState unchanged.
*/
traceState?: TraceState;
}
11 changes: 9 additions & 2 deletions packages/opentelemetry-core/src/trace/sampler/AlwaysOffSampler.ts
View file
Open in desktop
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
View file
Open in desktop
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
5 changes: 4 additions & 1 deletion packages/opentelemetry-core/src/trace/sampler/TraceIdRatioBasedSampler.ts
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion packages/opentelemetry-core/test/trace/AlwaysOffSampler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ 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,
});
});
});
3 changes: 2 additions & 1 deletion packages/opentelemetry-core/test/trace/AlwaysOnSampler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ 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,
});
});
});
6 changes: 6 additions & 0 deletions packages/opentelemetry-core/test/trace/ParentBasedSampler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -85,6 +86,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -103,6 +105,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -126,6 +129,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -144,6 +148,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -162,6 +167,7 @@ describe('ParentBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand Down
28 changes: 23 additions & 5 deletions packages/opentelemetry-core/test/trace/TraceIdRatioBasedSampler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import * as assert from 'assert';
import * as api from '@opentelemetry/api';
import { TraceIdRatioBasedSampler } from '../../src/trace/sampler/TraceIdRatioBasedSampler';

const spanContext = (traceId = '1') => ({
traceId,
spanId: '1.1',
traceFlags: api.TraceFlags.NONE,
});
const spanContext = (traceId = '1') =>
api.trace.setSpanContext(api.ROOT_CONTEXT, {
traceId,
spanId: '1.1',
traceFlags: api.TraceFlags.NONE,
});

const traceId = (part: string) => ('0'.repeat(32) + part).slice(-32);

Expand Down Expand Up @@ -59,6 +60,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -69,6 +71,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);
});
Expand All @@ -79,6 +82,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -89,6 +93,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -100,6 +105,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -111,6 +117,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -122,6 +129,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -132,6 +140,7 @@ describe('TraceIdRatioBasedSampler', () => {
sampler.shouldSample(spanContext(traceId('1')), traceId('1')),
{
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
}
);

Expand All @@ -142,6 +151,7 @@ describe('TraceIdRatioBasedSampler', () => {
),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -150,12 +160,14 @@ describe('TraceIdRatioBasedSampler', () => {
const sampler = new TraceIdRatioBasedSampler(1);
assert.deepStrictEqual(sampler.shouldSample(spanContext(''), ''), {
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
});

assert.deepStrictEqual(
sampler.shouldSample(spanContext(traceId('g')), traceId('g')),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand All @@ -167,30 +179,36 @@ describe('TraceIdRatioBasedSampler', () => {
const id1 = traceId((Math.floor(0xffffffff * 0.1) - 1).toString(16));
assert.deepStrictEqual(sampler10.shouldSample(spanContext(id1), id1), {
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
});
assert.deepStrictEqual(sampler20.shouldSample(spanContext(id1), id1), {
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
});

const id2 = traceId((Math.floor(0xffffffff * 0.2) - 1).toString(16));
assert.deepStrictEqual(sampler10.shouldSample(spanContext(id2), id2), {
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
});
assert.deepStrictEqual(sampler20.shouldSample(spanContext(id2), id2), {
decision: api.SamplingDecision.RECORD_AND_SAMPLED,
traceState: undefined,
});

const id2delta = traceId(Math.floor(0xffffffff * 0.2).toString(16));
assert.deepStrictEqual(
sampler10.shouldSample(spanContext(id2delta), id2delta),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
assert.deepStrictEqual(
sampler20.shouldSample(spanContext(id2delta), id2delta),
{
decision: api.SamplingDecision.NOT_RECORD,
traceState: undefined,
}
);
});
Expand Down
15 changes: 14 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/Sampler.ts
View file
Open in desktop
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,13 @@ 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
* TraceState if they do not intend to change it. Leaving the value undefined
Copy link

Choose a reason for hiding this comment

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

Question: The "passed-in TraceState", is it assumed that the sampler will read it off of the context that is passed in? Would it make sense to pass the tracestate as a separate argument to shouldSample?

Copy link

Choose a reason for hiding this comment

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

Alternatively, perhaps this comment should mention that the TraceState from the passed-in context, just to avoid confusion?

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 it would be quite confusing to pass context and some information which is usually on context as an additional parameter. Samplers have to decide then which source they prefer and if they should use the other as fallback,

Copy link

@henrinormak henrinormak Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, but reading this comment made me wonder, "passed-in where?" and immediately assumed that the function signature is out of date with this comment (that's me coming from a background of having a few custom samplers, but never having worked with tracestate). I agree that having a single source of truth is preferred, so perhaps the comment should be modified to clarify what "passed-in" refers to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated the doc comment to clarify

* will also leave the TraceState unchanged.
*/
traceState?: TraceState;
}

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

traceState = samplingResult.traceState ?? traceState;

const traceFlags =
samplingResult.decision === api.SamplingDecision.RECORD_AND_SAMPLED
? api.TraceFlags.SAMPLED
Expand Down
4 changes: 3 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/sampler/AlwaysOffSampler.ts
View file
Open in desktop
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
4 changes: 3 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/sampler/AlwaysOnSampler.ts
View file
Open in desktop
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
Loading