Skip to content

Commit f11bb04

Browse files
junaed-optimizelyjunaed-optimizely
committed
[FSSDK-11119] bucketer adjustment
1 parent 3740e6d commit f11bb04

File tree

5 files changed

+87
-56
lines changed

5 files changed

+87
-56
lines changed

‎lib/core/bucketer/bucket_value_generator.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/**
2+
* Copyright 2025, Optimizely
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
import murmurhash from 'murmurhash';
17+
import { INVALID_BUCKETING_ID } from 'error_message';
18+
import { OptimizelyError } from '../../error/optimizly_error';
19+
20+
const HASH_SEED = 1;
21+
const MAX_HASH_VALUE = Math.pow(2, 32);
22+
const MAX_TRAFFIC_VALUE = 10000;
23+
24+
/**
25+
* Helper function to generate bucket value in half-closed interval [0, MAX_TRAFFIC_VALUE)
26+
* @param {string} bucketingKey String value for bucketing
27+
* @return {number} The generated bucket value
28+
* @throws If bucketing value is not a valid string
29+
*/
30+
export const _generateBucketValue = function(bucketingKey: string): number {
31+
try {
32+
// NOTE: the mmh library already does cast the hash value as an unsigned 32bit int
33+
// https://github.com/perezd/node-murmurhash/blob/master/murmurhash.js#L115
34+
const hashValue = murmurhash.v3(bucketingKey, HASH_SEED);
35+
const ratio = hashValue / MAX_HASH_VALUE;
36+
return Math.floor(ratio * MAX_TRAFFIC_VALUE);
37+
} catch (ex) {
38+
throw new OptimizelyError(INVALID_BUCKETING_ID, bucketingKey, ex.message);
39+
}
40+
};

‎lib/core/bucketer/index.spec.ts

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import projectConfig, { ProjectConfig } from '../../project_config/project_confi
1919
import { getTestProjectConfig } from '../../tests/test_data';
2020
import { INVALID_BUCKETING_ID, INVALID_GROUP_ID } from 'error_message';
2121
import * as bucketer from './';
22+
import * as bucketValueGenerator from './bucket_value_generator';
23+
2224
import {
2325
USER_BUCKETED_INTO_EXPERIMENT_IN_GROUP,
2426
USER_NOT_BUCKETED_INTO_EXPERIMENT_IN_GROUP,
@@ -79,6 +81,10 @@ describe('excluding groups', () => {
7981
groupIdMap: configObj.groupIdMap,
8082
logger: mockLogger,
8183
};
84+
85+
vi.spyOn(bucketValueGenerator, '_generateBucketValue')
86+
.mockReturnValueOnce(50)
87+
.mockReturnValueOnce(50000);
8288
});
8389

8490
afterEach(() => {
@@ -95,10 +101,9 @@ describe('excluding groups', () => {
95101

96102
const bucketerParamsTest2 = cloneDeep(bucketerParams);
97103
bucketerParamsTest2.userId = 'ppid2';
98-
bucketerParamsTest2.bucketingId = 'test_3166_1739796928766';
99104
const decisionResponse2 = bucketer.bucket(bucketerParamsTest2);
100105

101-
expect(decisionResponse2.result).toBe(null);
106+
expect(decisionResponse2.result).toBeNull();
102107
expect(mockLogger.debug).toHaveBeenCalledWith(USER_ASSIGNED_TO_EXPERIMENT_BUCKET, expect.any(Number), 'ppid2');
103108
});
104109
});
@@ -122,7 +127,6 @@ describe('including groups: random', () => {
122127
groupIdMap: configObj.groupIdMap,
123128
logger: mockLogger,
124129
userId: 'testUser',
125-
bucketingId: 'test_303_1739432593254',
126130
};
127131
});
128132

@@ -131,6 +135,10 @@ describe('including groups: random', () => {
131135
});
132136

133137
it('should return decision response with the proper variation for a user in a grouped experiment', () => {
138+
vi.spyOn(bucketValueGenerator, '_generateBucketValue')
139+
.mockReturnValueOnce(50)
140+
.mockReturnValueOnce(50);
141+
134142
const decisionResponse = bucketer.bucket(bucketerParams);
135143

136144
expect(decisionResponse.result).toBe('551');
@@ -146,7 +154,8 @@ describe('including groups: random', () => {
146154
});
147155

148156
it('should return decision response with variation null when a user is bucketed into a different grouped experiment than the one speicfied', () => {
149-
bucketerParams.bucketingId = '123456789';
157+
vi.spyOn(bucketValueGenerator, '_generateBucketValue').mockReturnValue(5000);
158+
150159
const decisionResponse = bucketer.bucket(bucketerParams);
151160

152161
expect(decisionResponse.result).toBeNull();
@@ -162,21 +171,23 @@ describe('including groups: random', () => {
162171
});
163172

164173
it('should return decision response with variation null when a user is not bucketed into any experiments in the random group', () => {
165-
bucketerParams.bucketingId = 'test_1228_1739468735344';
174+
vi.spyOn(bucketValueGenerator, '_generateBucketValue').mockReturnValue(50000);
175+
166176
const decisionResponse = bucketer.bucket(bucketerParams);
167177

168-
expect(decisionResponse.result).toBe(null);
178+
expect(decisionResponse.result).toBeNull();
169179
expect(mockLogger.debug).toHaveBeenCalledTimes(1);
170180
expect(mockLogger.info).toHaveBeenCalledTimes(1);
171181
expect(mockLogger.debug).toHaveBeenCalledWith(USER_ASSIGNED_TO_EXPERIMENT_BUCKET, expect.any(Number), 'testUser');
172182
expect(mockLogger.info).toHaveBeenCalledWith(USER_NOT_IN_ANY_EXPERIMENT, 'testUser', '666');
173183
});
174184

175185
it('should return decision response with variation null when a user is bucketed into traffic space of deleted experiment within a random group', () => {
176-
bucketerParams.bucketingId = 'test_1228_1739468735344';
186+
vi.spyOn(bucketValueGenerator, '_generateBucketValue').mockReturnValueOnce(9000);
187+
177188
const decisionResponse = bucketer.bucket(bucketerParams);
178189

179-
expect(decisionResponse.result).toBe(null);
190+
expect(decisionResponse.result).toBeNull();
180191
expect(mockLogger.debug).toHaveBeenCalledTimes(1);
181192
expect(mockLogger.info).toHaveBeenCalledTimes(1);
182193
expect(mockLogger.debug).toHaveBeenCalledWith(USER_ASSIGNED_TO_EXPERIMENT_BUCKET, expect.any(Number), 'testUser');
@@ -220,7 +231,8 @@ describe('including groups: overlapping', () => {
220231
});
221232

222233
it('should return decision response with variation when a user falls into an experiment within an overlapping group', () => {
223-
bucketerParams.bucketingId = 'test_4283_1739793857480';
234+
vi.spyOn(bucketValueGenerator, '_generateBucketValue').mockReturnValueOnce(0);
235+
224236
const decisionResponse = bucketer.bucket(bucketerParams);
225237

226238
expect(decisionResponse.result).toBe('553');
@@ -229,10 +241,10 @@ describe('including groups: overlapping', () => {
229241
});
230242

231243
it('should return decision response with variation null when a user does not fall into an experiment within an overlapping group', () => {
232-
bucketerParams.bucketingId = 'test_9318_1739793997430';
244+
vi.spyOn(bucketValueGenerator, '_generateBucketValue').mockReturnValueOnce(3000);
233245
const decisionResponse = bucketer.bucket(bucketerParams);
234246

235-
expect(decisionResponse.result).toBe(null);
247+
expect(decisionResponse.result).toBeNull();
236248
});
237249
});
238250

@@ -275,7 +287,7 @@ describe('bucket value falls into empty traffic allocation ranges', () => {
275287
bucketerParamsTest1.userId = 'ppid1';
276288
const decisionResponse = bucketer.bucket(bucketerParamsTest1);
277289

278-
expect(decisionResponse.result).toBe(null);
290+
expect(decisionResponse.result).toBeNull();
279291
});
280292

281293
it('should not log an invalid variation ID warning', () => {
@@ -324,7 +336,7 @@ describe('traffic allocation has invalid variation ids', () => {
324336
bucketerParamsTest1.userId = 'ppid1';
325337
const decisionResponse = bucketer.bucket(bucketerParamsTest1);
326338

327-
expect(decisionResponse.result).toBe(null);
339+
expect(decisionResponse.result).toBeNull();
328340
});
329341
});
330342

@@ -336,16 +348,18 @@ describe('_generateBucketValue', () => {
336348
const bucketingKey3 = sprintf('%s%s', 'ppid2', 1886780722);
337349
const bucketingKey4 = sprintf('%s%s', 'ppid3', experimentId);
338350

339-
expect(bucketer._generateBucketValue(bucketingKey1)).toBe(5254);
340-
expect(bucketer._generateBucketValue(bucketingKey2)).toBe(4299);
341-
expect(bucketer._generateBucketValue(bucketingKey3)).toBe(2434);
342-
expect(bucketer._generateBucketValue(bucketingKey4)).toBe(5439);
351+
expect(bucketValueGenerator._generateBucketValue(bucketingKey1)).toBe(5254);
352+
expect(bucketValueGenerator._generateBucketValue(bucketingKey2)).toBe(4299);
353+
expect(bucketValueGenerator._generateBucketValue(bucketingKey3)).toBe(2434);
354+
expect(bucketValueGenerator._generateBucketValue(bucketingKey4)).toBe(5439);
343355
});
344356

345357
it('should return an error if it cannot generate the hash value', () => {
346358
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
347359
// @ts-ignore
348-
expect(() => bucketer._generateBucketValue(null)).toThrowError(new OptimizelyError(INVALID_BUCKETING_ID));
360+
expect(() => bucketValueGenerator._generateBucketValue(null)).toThrowError(
361+
new OptimizelyError(INVALID_BUCKETING_ID)
362+
);
349363
});
350364
});
351365

‎lib/core/bucketer/index.tests.js

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import sinon from 'sinon';
1717
import { assert, expect } from 'chai';
1818
import { cloneDeep, create } from 'lodash';
1919
import { sprintf } from '../../utils/fns';
20-
20+
import * as bucketValueGenerator from './bucket_value_generator'
2121
import * as bucketer from './';
2222
import { LOG_LEVEL } from '../../utils/enums';
2323
import projectConfig from '../../project_config/project_config';
@@ -76,15 +76,15 @@ describe('lib/core/bucketer', function () {
7676
logger: createdLogger,
7777
};
7878
sinon
79-
.stub(bucketer, '_generateBucketValue')
79+
.stub(bucketValueGenerator, '_generateBucketValue')
8080
.onFirstCall()
8181
.returns(50)
8282
.onSecondCall()
8383
.returns(50000);
8484
});
8585

8686
afterEach(function () {
87-
bucketer._generateBucketValue.restore();
87+
bucketValueGenerator._generateBucketValue.restore();
8888
});
8989

9090
it('should return decision response with correct variation ID when provided bucket value', function () {
@@ -116,11 +116,11 @@ describe('lib/core/bucketer', function () {
116116
groupIdMap: configObj.groupIdMap,
117117
logger: createdLogger,
118118
};
119-
bucketerStub = sinon.stub(bucketer, '_generateBucketValue');
119+
bucketerStub = sinon.stub(bucketValueGenerator, '_generateBucketValue');
120120
});
121121

122122
afterEach(function () {
123-
bucketer._generateBucketValue.restore();
123+
bucketValueGenerator._generateBucketValue.restore();
124124
});
125125

126126
describe('random groups', function () {
@@ -336,15 +336,15 @@ describe('lib/core/bucketer', function () {
336336
var bucketingKey3 = sprintf('%s%s', 'ppid2', 1886780722);
337337
var bucketingKey4 = sprintf('%s%s', 'ppid3', experimentId);
338338

339-
expect(bucketer._generateBucketValue(bucketingKey1)).to.equal(5254);
340-
expect(bucketer._generateBucketValue(bucketingKey2)).to.equal(4299);
341-
expect(bucketer._generateBucketValue(bucketingKey3)).to.equal(2434);
342-
expect(bucketer._generateBucketValue(bucketingKey4)).to.equal(5439);
339+
expect(bucketValueGenerator._generateBucketValue(bucketingKey1)).to.equal(5254);
340+
expect(bucketValueGenerator._generateBucketValue(bucketingKey2)).to.equal(4299);
341+
expect(bucketValueGenerator._generateBucketValue(bucketingKey3)).to.equal(2434);
342+
expect(bucketValueGenerator._generateBucketValue(bucketingKey4)).to.equal(5439);
343343
});
344344

345345
it('should return an error if it cannot generate the hash value', function() {
346346
const response = assert.throws(function() {
347-
bucketer._generateBucketValue(null);
347+
bucketValueGenerator._generateBucketValue(null);
348348
} );
349349
expect(response.baseMessage).to.equal(INVALID_BUCKETING_ID);
350350
});

‎lib/core/bucketer/index.ts

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -17,27 +17,22 @@
1717
/**
1818
* Bucketer API for determining the variation id from the specified parameters
1919
*/
20-
import murmurhash from 'murmurhash';
2120
import { LoggerFacade } from '../../logging/logger';
2221
import {
2322
DecisionResponse,
2423
BucketerParams,
2524
TrafficAllocation,
2625
Group,
2726
} from '../../shared_types';
28-
29-
import { INVALID_BUCKETING_ID, INVALID_GROUP_ID } from 'error_message';
27+
import { INVALID_GROUP_ID } from 'error_message';
3028
import { OptimizelyError } from '../../error/optimizly_error';
29+
import { _generateBucketValue } from './bucket_value_generator';
3130

3231
export const USER_NOT_IN_ANY_EXPERIMENT = 'User %s is not in any experiment of group %s.';
3332
export const USER_NOT_BUCKETED_INTO_EXPERIMENT_IN_GROUP = 'User %s is not in experiment %s of group %s.';
3433
export const USER_BUCKETED_INTO_EXPERIMENT_IN_GROUP = 'User %s is in experiment %s of group %s.';
3534
export const USER_ASSIGNED_TO_EXPERIMENT_BUCKET = 'Assigned bucket %s to user with bucketing ID %s.';
3635
export const INVALID_VARIATION_ID = 'Bucketed into an invalid variation ID. Returning null.';
37-
38-
const HASH_SEED = 1;
39-
const MAX_HASH_VALUE = Math.pow(2, 32);
40-
const MAX_TRAFFIC_VALUE = 10000;
4136
const RANDOM_POLICY = 'random';
4237

4338
/**
@@ -208,26 +203,7 @@ export const _findBucket = function(
208203
return null;
209204
};
210205

211-
/**
212-
* Helper function to generate bucket value in half-closed interval [0, MAX_TRAFFIC_VALUE)
213-
* @param {string} bucketingKey String value for bucketing
214-
* @return {number} The generated bucket value
215-
* @throws If bucketing value is not a valid string
216-
*/
217-
export const _generateBucketValue = function(bucketingKey: string): number {
218-
try {
219-
// NOTE: the mmh library already does cast the hash value as an unsigned 32bit int
220-
// https://github.com/perezd/node-murmurhash/blob/master/murmurhash.js#L115
221-
const hashValue = murmurhash.v3(bucketingKey, HASH_SEED);
222-
const ratio = hashValue / MAX_HASH_VALUE;
223-
return Math.floor(ratio * MAX_TRAFFIC_VALUE);
224-
} catch (ex: any) {
225-
throw new OptimizelyError(INVALID_BUCKETING_ID, bucketingKey, ex.message);
226-
}
227-
};
228-
229206
export default {
230207
bucket: bucket,
231208
bucketUserIntoExperiment: bucketUserIntoExperiment,
232-
_generateBucketValue: _generateBucketValue,
233209
};

‎lib/core/decision_service/index.tests.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { sprintf } from '../../utils/fns';
2020

2121
import { createDecisionService } from './';
2222
import * as bucketer from '../bucketer';
23+
import * as bucketValueGenerator from '../bucketer/bucket_value_generator';
2324
import {
2425
LOG_LEVEL,
2526
DECISION_SOURCES,
@@ -2227,7 +2228,7 @@ describe('lib/core/decision_service', function() {
22272228
var generateBucketValueStub;
22282229
beforeEach(function() {
22292230
feature = configObj.featureKeyMap.test_feature_in_exclusion_group;
2230-
generateBucketValueStub = sandbox.stub(bucketer, '_generateBucketValue');
2231+
generateBucketValueStub = sandbox.stub(bucketValueGenerator, '_generateBucketValue');
22312232
});
22322233

22332234
it('returns a decision with a variation in mutex group bucket less than 2500', function() {
@@ -2407,7 +2408,7 @@ describe('lib/core/decision_service', function() {
24072408
var generateBucketValueStub;
24082409
beforeEach(function() {
24092410
feature = configObj.featureKeyMap.test_feature_in_multiple_experiments;
2410-
generateBucketValueStub = sandbox.stub(bucketer, '_generateBucketValue');
2411+
generateBucketValueStub = sandbox.stub(bucketValueGenerator, '_generateBucketValue');
24112412
});
24122413

24132414
it('returns a decision with a variation in mutex group bucket less than 2500', function() {

0 commit comments

Comments
 (0)