Skip to content

Commit 67398a6

Browse files
author
Kartik Raj
authored
Ensure experiment storage is cleaned up once Python: Clear Internal extension storage command is run (#18807)
1 parent 3c0f29c commit 67398a6

File tree

6 files changed

+61
-38
lines changed

6 files changed

+61
-38
lines changed

‎src/client/common/experiments/service.ts

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,14 @@
33

44
'use strict';
55

6-
import { inject, injectable, named } from 'inversify';
7-
import { Memento } from 'vscode';
6+
import { inject, injectable } from 'inversify';
87
import { getExperimentationService, IExperimentationService, TargetPopulation } from 'vscode-tas-client';
98
import { traceLog } from '../../logging';
109
import { sendTelemetryEvent } from '../../telemetry';
1110
import { EventName } from '../../telemetry/constants';
1211
import { IApplicationEnvironment, IWorkspaceService } from '../application/types';
1312
import { PVSC_EXTENSION_ID } from '../constants';
14-
import { GLOBAL_MEMENTO, IExperimentService, IMemento } from '../types';
13+
import { IExperimentService, IPersistentStateFactory } from '../types';
1514
import { Experiments } from '../utils/localize';
1615
import { ExperimentationTelemetry } from './telemetry';
1716

@@ -30,14 +29,19 @@ export class ExperimentService implements IExperimentService {
3029
*/
3130
public _optOutFrom: string[] = [];
3231

32+
private readonly experiments = this.persistentState.createGlobalPersistentState<{ features: string[] }>(
33+
EXP_MEMENTO_KEY,
34+
{ features: [] },
35+
);
36+
3337
private readonly enabled: boolean;
3438

3539
private readonly experimentationService?: IExperimentationService;
3640

3741
constructor(
3842
@inject(IWorkspaceService) readonly workspaceService: IWorkspaceService,
3943
@inject(IApplicationEnvironment) private readonly appEnvironment: IApplicationEnvironment,
40-
@inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalState: Memento,
44+
@inject(IPersistentStateFactory) private readonly persistentState: IPersistentStateFactory,
4145
) {
4246
const settings = this.workspaceService.getConfiguration('python');
4347
// Users can only opt in or out of experiment groups, not control groups.
@@ -73,7 +77,7 @@ export class ExperimentService implements IExperimentService {
7377
this.appEnvironment.packageJson.version!,
7478
targetPopulation,
7579
telemetryReporter,
76-
this.globalState,
80+
this.experiments.storage,
7781
);
7882
}
7983

@@ -82,8 +86,7 @@ export class ExperimentService implements IExperimentService {
8286
const initStart = Date.now();
8387
await this.experimentationService.initializePromise;
8488

85-
const experiments = this.globalState.get<{ features: string[] }>(EXP_MEMENTO_KEY, { features: [] });
86-
if (experiments.features.length === 0) {
89+
if (this.experiments.value.features.length === 0) {
8790
// Only await on this if we don't have anything in cache.
8891
// This means that we start the session with partial experiment info.
8992
// We accept this as a compromise to avoid delaying startup.
@@ -190,9 +193,8 @@ export class ExperimentService implements IExperimentService {
190193
});
191194

192195
if (!experimentsDisabled) {
193-
const experiments = this.globalState.get<{ features: string[] }>(EXP_MEMENTO_KEY, { features: [] });
194196
// Log experiments that users are added to by the exp framework
195-
experiments.features.forEach((exp) => {
197+
this.experiments.value.features.forEach((exp) => {
196198
// Filter out experiment groups that are not from the Python extension.
197199
// Filter out experiment groups that are not already opted out or opted into.
198200
if (

‎src/client/common/persistentState.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { cache } from './utils/decorators';
2121

2222
export class PersistentState<T> implements IPersistentState<T> {
2323
constructor(
24-
private storage: Memento,
24+
public readonly storage: Memento,
2525
private key: string,
2626
private defaultValue?: T,
2727
private expiryDurationMs?: number,

‎src/client/common/types.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
Event,
1515
Extension,
1616
ExtensionContext,
17+
Memento,
1718
OutputChannel,
1819
Uri,
1920
WorkspaceEdit,
@@ -36,6 +37,11 @@ export const WORKSPACE_MEMENTO = Symbol('IWorkspaceMemento');
3637

3738
export type Resource = Uri | undefined;
3839
export interface IPersistentState<T> {
40+
/**
41+
* Storage is exposed in this type to make sure folks always use persistent state
42+
* factory to access any type of storage as all storages are tracked there.
43+
*/
44+
readonly storage: Memento;
3945
readonly value: T;
4046
updateValue(value: T): Promise<void>;
4147
}

‎src/test/common/experiments/service.unit.test.ts

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import { IApplicationEnvironment, IWorkspaceService } from '../../../client/comm
1414
import { WorkspaceService } from '../../../client/common/application/workspace';
1515
import { Channel } from '../../../client/common/constants';
1616
import { ExperimentService } from '../../../client/common/experiments/service';
17+
import { PersistentState } from '../../../client/common/persistentState';
18+
import { IPersistentStateFactory } from '../../../client/common/types';
1719
import { Experiments } from '../../../client/common/utils/localize';
1820
import { registerLogger } from '../../../client/logging';
1921
import { OutputChannelLogger } from '../../../client/logging/outputChannelLogger';
@@ -25,17 +27,23 @@ import { MockMemento } from '../../mocks/mementos';
2527

2628
suite('Experimentation service', () => {
2729
const extensionVersion = '1.2.3';
30+
const dummyExperimentKey = 'experimentsKey';
2831

2932
let workspaceService: IWorkspaceService;
3033
let appEnvironment: IApplicationEnvironment;
34+
let stateFactory: IPersistentStateFactory;
3135
let globalMemento: MockMemento;
3236
let outputChannel: MockOutputChannel;
3337
let disposeLogger: Disposable;
3438

3539
setup(() => {
3640
appEnvironment = mock(ApplicationEnvironment);
3741
workspaceService = mock(WorkspaceService);
42+
stateFactory = mock<IPersistentStateFactory>();
3843
globalMemento = new MockMemento();
44+
when(stateFactory.createGlobalPersistentState(anything(), anything())).thenReturn(
45+
new PersistentState(globalMemento, dummyExperimentKey, { features: [] }),
46+
);
3947
outputChannel = new MockOutputChannel('');
4048
disposeLogger = registerLogger(new OutputChannelLogger(outputChannel));
4149
});
@@ -78,7 +86,7 @@ suite('Experimentation service', () => {
7886
configureApplicationEnvironment('stable', extensionVersion);
7987

8088
// eslint-disable-next-line no-new
81-
new ExperimentService(instance(workspaceService), instance(appEnvironment), globalMemento);
89+
new ExperimentService(instance(workspaceService), instance(appEnvironment), instance(stateFactory));
8290

8391
sinon.assert.calledWithExactly(
8492
getExperimentationServiceStub,
@@ -97,7 +105,7 @@ suite('Experimentation service', () => {
97105
configureApplicationEnvironment('insiders', extensionVersion);
98106

99107
// eslint-disable-next-line no-new
100-
new ExperimentService(instance(workspaceService), instance(appEnvironment), globalMemento);
108+
new ExperimentService(instance(workspaceService), instance(appEnvironment), instance(stateFactory));
101109

102110
sinon.assert.calledWithExactly(
103111
getExperimentationServiceStub,
@@ -118,7 +126,7 @@ suite('Experimentation service', () => {
118126
const experimentService = new ExperimentService(
119127
instance(workspaceService),
120128
instance(appEnvironment),
121-
globalMemento,
129+
instance(stateFactory),
122130
);
123131

124132
assert.deepEqual(experimentService._optInto, ['Foo - experiment']);
@@ -132,25 +140,22 @@ suite('Experimentation service', () => {
132140
const experimentService = new ExperimentService(
133141
instance(workspaceService),
134142
instance(appEnvironment),
135-
globalMemento,
143+
instance(stateFactory),
136144
);
137145

138146
assert.deepEqual(experimentService._optOutFrom, ['Foo - experiment']);
139147
});
140148

141149
test('Experiment data in Memento storage should be logged if it starts with "python"', async () => {
142150
const experiments = ['ExperimentOne', 'pythonExperiment'];
143-
globalMemento = mock(MockMemento);
151+
globalMemento.update(dummyExperimentKey, { features: experiments });
144152
configureSettings(true, [], []);
145153
configureApplicationEnvironment('stable', extensionVersion, { configuration: { properties: {} } });
146154

147-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
148-
when(globalMemento.get(anything(), anything())).thenReturn({ features: experiments } as any);
149-
150155
const exp = new ExperimentService(
151156
instance(workspaceService),
152157
instance(appEnvironment),
153-
instance(globalMemento),
158+
instance(stateFactory),
154159
);
155160
await exp.activate();
156161
const output = `${Experiments.inGroup().format('pythonExperiment')}\n`;
@@ -192,7 +197,7 @@ suite('Experimentation service', () => {
192197
const experimentService = new ExperimentService(
193198
instance(workspaceService),
194199
instance(appEnvironment),
195-
globalMemento,
200+
instance(stateFactory),
196201
);
197202
const result = experimentService.inExperimentSync(experiment);
198203

@@ -207,7 +212,7 @@ suite('Experimentation service', () => {
207212
const experimentService = new ExperimentService(
208213
instance(workspaceService),
209214
instance(appEnvironment),
210-
globalMemento,
215+
instance(stateFactory),
211216
);
212217
const result = experimentService.inExperimentSync(experiment);
213218

@@ -222,7 +227,7 @@ suite('Experimentation service', () => {
222227
const experimentService = new ExperimentService(
223228
instance(workspaceService),
224229
instance(appEnvironment),
225-
globalMemento,
230+
instance(stateFactory),
226231
);
227232
const result = experimentService.inExperimentSync(experiment);
228233

@@ -236,7 +241,7 @@ suite('Experimentation service', () => {
236241
const experimentService = new ExperimentService(
237242
instance(workspaceService),
238243
instance(appEnvironment),
239-
globalMemento,
244+
instance(stateFactory),
240245
);
241246
const result = experimentService.inExperimentSync(experiment);
242247

@@ -251,7 +256,7 @@ suite('Experimentation service', () => {
251256
const experimentService = new ExperimentService(
252257
instance(workspaceService),
253258
instance(appEnvironment),
254-
globalMemento,
259+
instance(stateFactory),
255260
);
256261
const result = experimentService.inExperimentSync(experiment);
257262

@@ -266,7 +271,7 @@ suite('Experimentation service', () => {
266271
const experimentService = new ExperimentService(
267272
instance(workspaceService),
268273
instance(appEnvironment),
269-
globalMemento,
274+
instance(stateFactory),
270275
);
271276
const result = experimentService.inExperimentSync(experiment);
272277

@@ -281,7 +286,7 @@ suite('Experimentation service', () => {
281286
const experimentService = new ExperimentService(
282287
instance(workspaceService),
283288
instance(appEnvironment),
284-
globalMemento,
289+
instance(stateFactory),
285290
);
286291
const result = experimentService.inExperimentSync(experiment);
287292

@@ -296,7 +301,7 @@ suite('Experimentation service', () => {
296301
const experimentService = new ExperimentService(
297302
instance(workspaceService),
298303
instance(appEnvironment),
299-
globalMemento,
304+
instance(stateFactory),
300305
);
301306
const result = experimentService.inExperimentSync(experiment);
302307

@@ -311,7 +316,7 @@ suite('Experimentation service', () => {
311316
const experimentService = new ExperimentService(
312317
instance(workspaceService),
313318
instance(appEnvironment),
314-
globalMemento,
319+
instance(stateFactory),
315320
);
316321
const result = experimentService.inExperimentSync(experiment);
317322

@@ -340,7 +345,7 @@ suite('Experimentation service', () => {
340345
const experimentService = new ExperimentService(
341346
instance(workspaceService),
342347
instance(appEnvironment),
343-
globalMemento,
348+
instance(stateFactory),
344349
);
345350
const result = await experimentService.getExperimentValue(experiment);
346351

@@ -354,7 +359,7 @@ suite('Experimentation service', () => {
354359
const experimentService = new ExperimentService(
355360
instance(workspaceService),
356361
instance(appEnvironment),
357-
globalMemento,
362+
instance(stateFactory),
358363
);
359364
const result = await experimentService.getExperimentValue(experiment);
360365

@@ -368,7 +373,7 @@ suite('Experimentation service', () => {
368373
const experimentService = new ExperimentService(
369374
instance(workspaceService),
370375
instance(appEnvironment),
371-
globalMemento,
376+
instance(stateFactory),
372377
);
373378
const result = await experimentService.getExperimentValue(experiment);
374379

@@ -382,7 +387,7 @@ suite('Experimentation service', () => {
382387
const experimentService = new ExperimentService(
383388
instance(workspaceService),
384389
instance(appEnvironment),
385-
globalMemento,
390+
instance(stateFactory),
386391
);
387392
const result = await experimentService.getExperimentValue(experiment);
388393

@@ -417,7 +422,7 @@ suite('Experimentation service', () => {
417422
const experimentService = new ExperimentService(
418423
instance(workspaceService),
419424
instance(appEnvironment),
420-
globalMemento,
425+
instance(stateFactory),
421426
);
422427

423428
await experimentService.activate();
@@ -450,7 +455,7 @@ suite('Experimentation service', () => {
450455
const experimentService = new ExperimentService(
451456
instance(workspaceService),
452457
instance(appEnvironment),
453-
globalMemento,
458+
instance(stateFactory),
454459
);
455460

456461
await experimentService.activate();
@@ -482,7 +487,7 @@ suite('Experimentation service', () => {
482487
const experimentService = new ExperimentService(
483488
instance(workspaceService),
484489
instance(appEnvironment),
485-
globalMemento,
490+
instance(stateFactory),
486491
);
487492

488493
await experimentService.activate();
@@ -514,7 +519,7 @@ suite('Experimentation service', () => {
514519
const experimentService = new ExperimentService(
515520
instance(workspaceService),
516521
instance(appEnvironment),
517-
globalMemento,
522+
instance(stateFactory),
518523
);
519524

520525
await experimentService.activate();
@@ -536,7 +541,7 @@ suite('Experimentation service', () => {
536541
const experimentService = new ExperimentService(
537542
instance(workspaceService),
538543
instance(appEnvironment),
539-
globalMemento,
544+
instance(stateFactory),
540545
);
541546

542547
await experimentService.activate();
@@ -567,7 +572,7 @@ suite('Experimentation service', () => {
567572
const experimentService = new ExperimentService(
568573
instance(workspaceService),
569574
instance(appEnvironment),
570-
globalMemento,
575+
instance(stateFactory),
571576
);
572577

573578
await experimentService.activate();

0 commit comments

Comments
 (0)