Skip to content

Commit dc762ec

Browse files
jj22eewangzlei
jj22ee
and
wangzlei
authored
[X-Ray Sampler] fix truthy checks, update sampler unit tests (#234)
*Issue #, if available:* In `aws-distro-opentelemetry-node-autoinstrumentation/src/sampler/sampling-rule-applier.ts`, truthy checks are done on `ReservoirQuota/ReservoirQuotaTTL/target.FixedRate` to update the Sampling Rule Applier. However for example, if `target.FixedRate` is 0, then it will fail the truthy check. *Description of changes:* 1. Instead of a truthy check on `ReservoirQuota/ReservoirQuotaTTL/target.FixedRate`, validate if their value is a number. 2. Sync unit tests with the Sampler PR to upstream. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. Co-authored-by: Lei Wang <[email protected]>
1 parent 7151d66 commit dc762ec

File tree

8 files changed

+93
-69
lines changed

8 files changed

+93
-69
lines changed

‎aws-distro-opentelemetry-node-autoinstrumentation/src/sampler/fallback-sampler.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ export class FallbackSampler implements Sampler {
1010
private fixedRateSampler: TraceIdRatioBasedSampler;
1111
private rateLimitingSampler: RateLimitingSampler;
1212

13-
constructor() {
14-
this.fixedRateSampler = new TraceIdRatioBasedSampler(0.05);
15-
this.rateLimitingSampler = new RateLimitingSampler(1);
13+
constructor(ratio: number = 0.05, quota: number = 1) {
14+
this.fixedRateSampler = new TraceIdRatioBasedSampler(ratio);
15+
this.rateLimitingSampler = new RateLimitingSampler(quota);
1616
}
1717

1818
shouldSample(

‎aws-distro-opentelemetry-node-autoinstrumentation/src/sampler/rule-cache.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -94,24 +94,24 @@ export class RuleCache {
9494

9595
// Update ruleAppliers based on the targets fetched from X-Ray service
9696
public updateTargets(targetDocuments: TargetMap, lastRuleModification: number): [boolean, number] {
97-
let minPollingInteral: number | undefined = undefined;
97+
let minPollingInterval: number | undefined = undefined;
9898
let nextPollingInterval: number = DEFAULT_TARGET_POLLING_INTERVAL_SECONDS;
9999
this.ruleAppliers.forEach((rule: SamplingRuleApplier, index: number) => {
100100
const target: SamplingTargetDocument = targetDocuments[rule.samplingRule.RuleName];
101101
if (target) {
102102
this.ruleAppliers[index] = rule.withTarget(target);
103-
if (target.Interval) {
104-
if (minPollingInteral === undefined || minPollingInteral > target.Interval) {
105-
minPollingInteral = target.Interval;
103+
if (typeof target.Interval === 'number') {
104+
if (minPollingInterval === undefined || minPollingInterval > target.Interval) {
105+
minPollingInterval = target.Interval;
106106
}
107107
}
108108
} else {
109109
diag.debug('Invalid sampling target: missing rule name');
110110
}
111111
});
112112

113-
if (minPollingInteral) {
114-
nextPollingInterval = minPollingInteral;
113+
if (typeof minPollingInterval === 'number') {
114+
nextPollingInterval = minPollingInterval;
115115
}
116116

117117
const refreshSamplingRules: boolean = lastRuleModification * 1000 > this.lastUpdatedEpochMillis;

‎aws-distro-opentelemetry-node-autoinstrumentation/src/sampler/sampling-rule-applier.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,17 @@ export class SamplingRuleApplier {
5757

5858
if (target) {
5959
this.borrowingEnabled = false;
60-
if (target.ReservoirQuota) {
60+
if (typeof target.ReservoirQuota === 'number') {
6161
this.reservoirSampler = new RateLimitingSampler(target.ReservoirQuota);
6262
}
6363

64-
if (target.ReservoirQuotaTTL) {
64+
if (typeof target.ReservoirQuotaTTL === 'number') {
6565
this.reservoirExpiryTimeInMillis = new Date(target.ReservoirQuotaTTL * 1000).getTime();
6666
} else {
6767
this.reservoirExpiryTimeInMillis = Date.now();
6868
}
6969

70-
if (target.FixedRate) {
70+
if (typeof target.FixedRate === 'number') {
7171
this.fixedRateSampler = new TraceIdRatioBasedSampler(target.FixedRate);
7272
}
7373
}

‎aws-distro-opentelemetry-node-autoinstrumentation/test/sampler/aws-xray-remote-sampler.test.ts‎

Lines changed: 54 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
22
// SPDX-License-Identifier: Apache-2.0
33

4-
import { context, Span, SpanKind } from '@opentelemetry/api';
4+
import { Attributes, Context, context, Link, Span, SpanKind } from '@opentelemetry/api';
55
import { Resource } from '@opentelemetry/resources';
66
import { SamplingDecision, Tracer } from '@opentelemetry/sdk-trace-base';
77
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
@@ -10,12 +10,18 @@ import { expect } from 'expect';
1010
import * as nock from 'nock';
1111
import * as sinon from 'sinon';
1212
import { _AwsXRayRemoteSampler, AwsXRayRemoteSampler } from '../../src/sampler/aws-xray-remote-sampler';
13+
import { FallbackSampler } from '../../src/sampler/fallback-sampler';
1314

1415
const DATA_DIR_SAMPLING_RULES = __dirname + '/data/test-remote-sampler_sampling-rules-response-sample.json';
1516
const DATA_DIR_SAMPLING_TARGETS = __dirname + '/data/test-remote-sampler_sampling-targets-response-sample.json';
1617
const TEST_URL = 'http://localhost:2000';
18+
export const testTraceId = '0af7651916cd43dd8448eb211c80319c';
1719

1820
describe('AwsXrayRemoteSampler', () => {
21+
afterEach(() => {
22+
sinon.restore();
23+
});
24+
1925
it('testCreateRemoteSamplerWithEmptyResource', () => {
2026
const sampler: AwsXRayRemoteSampler = new AwsXRayRemoteSampler({ resource: Resource.EMPTY });
2127

@@ -81,21 +87,21 @@ describe('AwsXrayRemoteSampler', () => {
8187
setTimeout(() => {
8288
expect(((sampler as any)._root._root.ruleCache as any).ruleAppliers[0].samplingRule.RuleName).toEqual('test');
8389
expect(
84-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
90+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
8591
).toEqual(SamplingDecision.NOT_RECORD);
8692

8793
setTimeout(() => {
8894
// restore function
8995
(_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval = tmp;
9096

9197
expect(
92-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
98+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
9399
).toEqual(SamplingDecision.RECORD_AND_SAMPLED);
94100
expect(
95-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
101+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
96102
).toEqual(SamplingDecision.RECORD_AND_SAMPLED);
97103
expect(
98-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
104+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, { abc: '1234' }, []).decision
99105
).toEqual(SamplingDecision.RECORD_AND_SAMPLED);
100106

101107
done();
@@ -112,40 +118,38 @@ describe('AwsXrayRemoteSampler', () => {
112118
});
113119
const attributes = { abc: '1234' };
114120

115-
// Patch default target polling interval
116-
const tmp = (_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval;
117-
(_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval = () => {
118-
return 0.2; // seconds
119-
};
120121
const sampler = new AwsXRayRemoteSampler({
121122
resource: resource,
122123
});
124+
const internalXraySampler = sampler['_root']['_root'] as _AwsXRayRemoteSampler;
125+
internalXraySampler['getAndUpdateSamplingRules']();
123126

124127
setTimeout(() => {
125128
expect(((sampler as any)._root._root.ruleCache as any).ruleAppliers[0].samplingRule.RuleName).toEqual('test');
126-
expect(sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, attributes, []).decision).toEqual(
127-
SamplingDecision.NOT_RECORD
128-
);
129+
expect(
130+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, attributes, []).decision
131+
).toEqual(SamplingDecision.NOT_RECORD);
132+
internalXraySampler['getAndUpdateSamplingTargets']();
129133

130134
setTimeout(() => {
131135
let sampled = 0;
132-
for (let i = 0; i < 100000; i++) {
136+
const clock = sinon.useFakeTimers(Date.now());
137+
clock.tick(1000);
138+
for (let i = 0; i < 1000; i++) {
133139
if (
134-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, attributes, []).decision !==
140+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, attributes, []).decision !==
135141
SamplingDecision.NOT_RECORD
136142
) {
137143
sampled++;
138144
}
139145
}
146+
clock.restore();
140147

141-
// restore function
142-
(_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval = tmp;
143-
144-
expect((sampler as any)._root._root.ruleCache.ruleAppliers[0].reservoirSampler.quota).toEqual(100000);
145-
expect(sampled).toEqual(100000);
148+
expect((sampler as any)._root._root.ruleCache.ruleAppliers[0].reservoirSampler.quota).toEqual(1000);
149+
expect(sampled).toEqual(1000);
146150
done();
147-
}, 2000);
148-
}, 100);
151+
}, 300);
152+
}, 300);
149153
});
150154

151155
it('testSomeReservoir', done => {
@@ -157,39 +161,54 @@ describe('AwsXrayRemoteSampler', () => {
157161
});
158162
const attributes = { abc: 'non-matching attribute value, use default rule' };
159163

160-
// Patch default target polling interval
161-
const tmp = (_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval;
162-
(_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval = () => {
163-
return 2; // seconds
164-
};
165164
const sampler = new AwsXRayRemoteSampler({
166165
resource: resource,
167166
});
167+
const internalXraySampler = sampler['_root']['_root'] as _AwsXRayRemoteSampler;
168+
sinon
169+
.stub(sampler['_root']['_root'].fallbackSampler as FallbackSampler, 'shouldSample')
170+
.callsFake(
171+
(
172+
context: Context,
173+
traceId: string,
174+
spanName: string,
175+
spanKind: SpanKind,
176+
attributes: Attributes,
177+
links: Link[]
178+
) => {
179+
return {
180+
decision: SamplingDecision.NOT_RECORD,
181+
attributes: attributes,
182+
};
183+
}
184+
);
168185

186+
internalXraySampler['getAndUpdateSamplingRules']();
169187
setTimeout(() => {
170188
expect(((sampler as any)._root._root.ruleCache as any).ruleAppliers[0].samplingRule.RuleName).toEqual('test');
171-
expect(sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, attributes, []).decision).toEqual(
172-
SamplingDecision.NOT_RECORD
173-
);
189+
expect(
190+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, attributes, []).decision
191+
).toEqual(SamplingDecision.RECORD_AND_SAMPLED);
192+
193+
internalXraySampler['getAndUpdateSamplingTargets']();
174194

175195
setTimeout(() => {
176196
const clock = sinon.useFakeTimers(Date.now());
177-
clock.tick(2000);
197+
clock.tick(1000);
178198
let sampled = 0;
179199
for (let i = 0; i < 100000; i++) {
180200
if (
181-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, attributes, []).decision !==
201+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, attributes, []).decision !==
182202
SamplingDecision.NOT_RECORD
183203
) {
184204
sampled++;
185205
}
186206
}
187207
clock.restore();
188-
// restore function
189-
(_AwsXRayRemoteSampler.prototype as any).getDefaultTargetPollingInterval = tmp;
208+
190209
expect(sampled).toEqual(100);
191210
done();
192-
}, 2000);
211+
}, 300);
193212
}, 300);
194213
});
195214

‎aws-distro-opentelemetry-node-autoinstrumentation/test/sampler/data/test-remote-sampler_sampling-targets-response-sample.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
{
55
"FixedRate": 0.0,
66
"Interval": 100000,
7-
"ReservoirQuota": 100000,
7+
"ReservoirQuota": 1000,
88
"ReservoirQuotaTTL": 9999999999.0,
99
"RuleName": "test"
1010
},

‎aws-distro-opentelemetry-node-autoinstrumentation/test/sampler/fallback-sampler.test.ts‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,29 +6,33 @@ import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
66
import { expect } from 'expect';
77
import * as sinon from 'sinon';
88
import { FallbackSampler } from '../../src/sampler/fallback-sampler';
9+
import { testTraceId } from './aws-xray-remote-sampler.test';
910

1011
let clock: sinon.SinonFakeTimers;
1112
describe('FallBackSampler', () => {
1213
beforeEach(() => {
1314
clock = sinon.useFakeTimers(Date.now());
1415
});
16+
1517
afterEach(() => {
1618
try {
1719
clock.restore();
1820
} catch {
1921
// do nothing
2022
}
2123
});
22-
it('testShouldSample', () => {
23-
const sampler = new FallbackSampler();
2424

25-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []);
25+
it('testShouldSampleWithQuotaOnly', () => {
26+
// Ensure FallbackSampler's internal TraceIdRatioBasedSampler will always return SamplingDecision.NOT_RECORD
27+
const sampler = new FallbackSampler(0);
28+
29+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []);
2630

2731
// 0 seconds passed, 0 quota available
2832
let sampled = 0;
2933
for (let i = 0; i < 30; i++) {
3034
if (
31-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
35+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
3236
SamplingDecision.NOT_RECORD
3337
) {
3438
sampled += 1;
@@ -41,7 +45,7 @@ describe('FallBackSampler', () => {
4145
clock.tick(0.4 * 1000);
4246
for (let i = 0; i < 30; i++) {
4347
if (
44-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
48+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
4549
SamplingDecision.NOT_RECORD
4650
) {
4751
sampled += 1;
@@ -54,7 +58,7 @@ describe('FallBackSampler', () => {
5458
clock.tick(0.4 * 1000);
5559
for (let i = 0; i < 30; i++) {
5660
if (
57-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
61+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
5862
SamplingDecision.NOT_RECORD
5963
) {
6064
sampled += 1;
@@ -67,7 +71,7 @@ describe('FallBackSampler', () => {
6771
clock.tick(0.4 * 1000);
6872
for (let i = 0; i < 30; i++) {
6973
if (
70-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
74+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
7175
SamplingDecision.NOT_RECORD
7276
) {
7377
sampled += 1;
@@ -80,7 +84,7 @@ describe('FallBackSampler', () => {
8084
clock.tick(0.4 * 1000);
8185
for (let i = 0; i < 30; i++) {
8286
if (
83-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
87+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
8488
SamplingDecision.NOT_RECORD
8589
) {
8690
sampled += 1;
@@ -93,7 +97,7 @@ describe('FallBackSampler', () => {
9397
clock.tick(0.4 * 1000);
9498
for (let i = 0; i < 30; i++) {
9599
if (
96-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
100+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
97101
SamplingDecision.NOT_RECORD
98102
) {
99103
sampled += 1;
@@ -106,7 +110,7 @@ describe('FallBackSampler', () => {
106110
clock.tick(0.4 * 1000);
107111
for (let i = 0; i < 30; i++) {
108112
if (
109-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
113+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
110114
SamplingDecision.NOT_RECORD
111115
) {
112116
sampled += 1;
@@ -119,7 +123,7 @@ describe('FallBackSampler', () => {
119123
clock.tick(100 * 1000);
120124
for (let i = 0; i < 30; i++) {
121125
if (
122-
sampler.shouldSample(context.active(), '1234', 'name', SpanKind.CLIENT, {}, []).decision !==
126+
sampler.shouldSample(context.active(), testTraceId, 'name', SpanKind.CLIENT, {}, []).decision !==
123127
SamplingDecision.NOT_RECORD
124128
) {
125129
sampled += 1;

0 commit comments

Comments
 (0)