Skip to content

Commit e2f97f5

Browse files
jj22eejj22ee
committed
address comments
1 parent 496cc32 commit e2f97f5

File tree

8 files changed

+81
-50
lines changed

8 files changed

+81
-50
lines changed

‎incubator/opentelemetry-sampler-aws-xray/src/aws-xray-sampling-client.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export class AWSXRaySamplingClient {
4545
this.makeSamplingRequest<GetSamplingTargetsResponse>(
4646
this.samplingTargetsEndpoint,
4747
callback,
48-
this.samplerDiag.debug.bind(this.samplerDiag),
48+
(message: string) => this.samplerDiag.debug(message),
4949
JSON.stringify(requestBody)
5050
);
5151
}
@@ -56,7 +56,7 @@ export class AWSXRaySamplingClient {
5656
this.makeSamplingRequest<GetSamplingRulesResponse>(
5757
this.getSamplingRulesEndpoint,
5858
callback,
59-
this.samplerDiag.error.bind(this.samplerDiag)
59+
(message: string) => this.samplerDiag.error(message)
6060
);
6161
}
6262

‎incubator/opentelemetry-sampler-aws-xray/src/fallback-sampler.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,9 @@ export class FallbackSampler implements Sampler {
3232
private fixedRateSampler: TraceIdRatioBasedSampler;
3333
private rateLimitingSampler: RateLimitingSampler;
3434

35-
constructor() {
36-
this.fixedRateSampler = new TraceIdRatioBasedSampler(0.05);
37-
this.rateLimitingSampler = new RateLimitingSampler(1);
35+
constructor(ratio = 0.05, quota = 1) {
36+
this.fixedRateSampler = new TraceIdRatioBasedSampler(ratio);
37+
this.rateLimitingSampler = new RateLimitingSampler(quota);
3838
}
3939

4040
shouldSample(

‎incubator/opentelemetry-sampler-aws-xray/src/rate-limiter.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,8 @@ export class RateLimiter {
4242
return false;
4343
}
4444

45-
const quotaPerMillis: number = this.quota / 1000.0;
46-
4745
// assume divide by zero not possible
48-
const costInMillis: number = cost / quotaPerMillis;
46+
const costInMillis: number = (cost * 1000.0) / this.quota;
4947

5048
const walletCeilingMillis: number = Date.now();
5149
let currentBalanceMillis: number =

‎incubator/opentelemetry-sampler-aws-xray/src/rule-cache.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -127,28 +127,29 @@ export class RuleCache {
127127
targetDocuments: TargetMap,
128128
lastRuleModification: number
129129
): [boolean, number] {
130-
let minPollingInteral: number | undefined = undefined;
130+
let minPollingInterval: number | undefined = undefined;
131131
let nextPollingInterval: number = DEFAULT_TARGET_POLLING_INTERVAL_SECONDS;
132-
this.ruleAppliers.forEach((rule: SamplingRuleApplier, index: number) => {
132+
133+
for (const [index, rule] of this.ruleAppliers.entries()) {
133134
const target: SamplingTargetDocument =
134135
targetDocuments[rule.samplingRule.RuleName];
135136
if (target) {
136137
this.ruleAppliers[index] = rule.withTarget(target);
137-
if (target.Interval) {
138+
if (typeof target.Interval === 'number') {
138139
if (
139-
minPollingInteral === undefined ||
140-
minPollingInteral > target.Interval
140+
minPollingInterval === undefined ||
141+
minPollingInterval > target.Interval
141142
) {
142-
minPollingInteral = target.Interval;
143+
minPollingInterval = target.Interval;
143144
}
144145
}
145146
} else {
146147
diag.debug('Invalid sampling target: missing rule name');
147148
}
148-
});
149+
}
149150

150-
if (minPollingInteral) {
151-
nextPollingInterval = minPollingInteral;
151+
if (typeof minPollingInterval === 'number') {
152+
nextPollingInterval = minPollingInterval;
152153
}
153154

154155
const refreshSamplingRules: boolean =

‎incubator/opentelemetry-sampler-aws-xray/src/sampling-rule-applier.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,19 +98,19 @@ export class SamplingRuleApplier {
9898

9999
if (target) {
100100
this.borrowingEnabled = false;
101-
if (target.ReservoirQuota) {
101+
if (typeof target.ReservoirQuota === 'number') {
102102
this.reservoirSampler = new RateLimitingSampler(target.ReservoirQuota);
103103
}
104104

105-
if (target.ReservoirQuotaTTL) {
105+
if (typeof target.ReservoirQuotaTTL === 'number') {
106106
this.reservoirExpiryTimeInMillis = new Date(
107107
target.ReservoirQuotaTTL * 1000
108108
).getTime();
109109
} else {
110110
this.reservoirExpiryTimeInMillis = Date.now();
111111
}
112112

113-
if (target.FixedRate) {
113+
if (typeof target.FixedRate === 'number') {
114114
this.fixedRateSampler = new TraceIdRatioBasedSampler(target.FixedRate);
115115
}
116116
}

‎incubator/opentelemetry-sampler-aws-xray/test/fallback-sampler.test.ts‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@ import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
2323
import { expect } from 'expect';
2424
import * as sinon from 'sinon';
2525
import { FallbackSampler } from '../src/fallback-sampler';
26+
import { testTraceId } from './remote-sampler.test';
2627

2728
let clock: sinon.SinonFakeTimers;
29+
2830
describe('FallBackSampler', () => {
2931
beforeEach(() => {
3032
clock = sinon.useFakeTimers(Date.now());
@@ -36,12 +38,13 @@ describe('FallBackSampler', () => {
3638
// do nothing
3739
}
3840
});
39-
it('testShouldSample', () => {
40-
const sampler = new FallbackSampler();
41+
it('testShouldSampleWithQuotaOnly', () => {
42+
// Ensure FallbackSampler's internal TraceIdRatioBasedSampler will always return SamplingDecision.NOT_RECORD
43+
const sampler = new FallbackSampler(0);
4144

4245
sampler.shouldSample(
4346
context.active(),
44-
'1234',
47+
testTraceId,
4548
'name',
4649
SpanKind.CLIENT,
4750
{},
@@ -54,7 +57,7 @@ describe('FallBackSampler', () => {
5457
if (
5558
sampler.shouldSample(
5659
context.active(),
57-
'1234',
60+
testTraceId,
5861
'name',
5962
SpanKind.CLIENT,
6063
{},
@@ -73,7 +76,7 @@ describe('FallBackSampler', () => {
7376
if (
7477
sampler.shouldSample(
7578
context.active(),
76-
'1234',
79+
testTraceId,
7780
'name',
7881
SpanKind.CLIENT,
7982
{},
@@ -92,7 +95,7 @@ describe('FallBackSampler', () => {
9295
if (
9396
sampler.shouldSample(
9497
context.active(),
95-
'1234',
98+
testTraceId,
9699
'name',
97100
SpanKind.CLIENT,
98101
{},
@@ -111,7 +114,7 @@ describe('FallBackSampler', () => {
111114
if (
112115
sampler.shouldSample(
113116
context.active(),
114-
'1234',
117+
testTraceId,
115118
'name',
116119
SpanKind.CLIENT,
117120
{},
@@ -130,7 +133,7 @@ describe('FallBackSampler', () => {
130133
if (
131134
sampler.shouldSample(
132135
context.active(),
133-
'1234',
136+
testTraceId,
134137
'name',
135138
SpanKind.CLIENT,
136139
{},
@@ -149,7 +152,7 @@ describe('FallBackSampler', () => {
149152
if (
150153
sampler.shouldSample(
151154
context.active(),
152-
'1234',
155+
testTraceId,
153156
'name',
154157
SpanKind.CLIENT,
155158
{},
@@ -168,7 +171,7 @@ describe('FallBackSampler', () => {
168171
if (
169172
sampler.shouldSample(
170173
context.active(),
171-
'1234',
174+
testTraceId,
172175
'name',
173176
SpanKind.CLIENT,
174177
{},
@@ -187,7 +190,7 @@ describe('FallBackSampler', () => {
187190
if (
188191
sampler.shouldSample(
189192
context.active(),
190-
'1234',
193+
testTraceId,
191194
'name',
192195
SpanKind.CLIENT,
193196
{},

‎incubator/opentelemetry-sampler-aws-xray/test/rate-limiting-sampler.test.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
2323
import { expect } from 'expect';
2424
import * as sinon from 'sinon';
2525
import { RateLimitingSampler } from '../src/rate-limiting-sampler';
26+
import { testTraceId } from './remote-sampler.test';
2627

2728
let clock: sinon.SinonFakeTimers;
2829

@@ -41,7 +42,7 @@ describe('RateLimitingSampler', () => {
4142
if (
4243
sampler.shouldSample(
4344
context.active(),
44-
'1234',
45+
testTraceId,
4546
'name',
4647
SpanKind.CLIENT,
4748
{},
@@ -60,7 +61,7 @@ describe('RateLimitingSampler', () => {
6061
if (
6162
sampler.shouldSample(
6263
context.active(),
63-
'1234',
64+
testTraceId,
6465
'name',
6566
SpanKind.CLIENT,
6667
{},
@@ -79,7 +80,7 @@ describe('RateLimitingSampler', () => {
7980
if (
8081
sampler.shouldSample(
8182
context.active(),
82-
'1234',
83+
testTraceId,
8384
'name',
8485
SpanKind.CLIENT,
8586
{},
@@ -98,7 +99,7 @@ describe('RateLimitingSampler', () => {
9899
if (
99100
sampler.shouldSample(
100101
context.active(),
101-
'1234',
102+
testTraceId,
102103
'name',
103104
SpanKind.CLIENT,
104105
{},
@@ -117,7 +118,7 @@ describe('RateLimitingSampler', () => {
117118
if (
118119
sampler.shouldSample(
119120
context.active(),
120-
'1234',
121+
testTraceId,
121122
'name',
122123
SpanKind.CLIENT,
123124
{},
@@ -138,7 +139,7 @@ describe('RateLimitingSampler', () => {
138139
if (
139140
sampler.shouldSample(
140141
context.active(),
141-
'1234',
142+
testTraceId,
142143
'name',
143144
SpanKind.CLIENT,
144145
{},
@@ -157,7 +158,7 @@ describe('RateLimitingSampler', () => {
157158
if (
158159
sampler.shouldSample(
159160
context.active(),
160-
'1234',
161+
testTraceId,
161162
'name',
162163
SpanKind.CLIENT,
163164
{},
@@ -176,7 +177,7 @@ describe('RateLimitingSampler', () => {
176177
if (
177178
sampler.shouldSample(
178179
context.active(),
179-
'1234',
180+
testTraceId,
180181
'name',
181182
SpanKind.CLIENT,
182183
{},
@@ -195,7 +196,7 @@ describe('RateLimitingSampler', () => {
195196
if (
196197
sampler.shouldSample(
197198
context.active(),
198-
'1234',
199+
testTraceId,
199200
'name',
200201
SpanKind.CLIENT,
201202
{},

0 commit comments

Comments
 (0)