Skip to content

Deprecate inExperiment API in favor of inExperimentSync #16848

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

Merged
karthiknadig merged 3 commits into from
Aug 6, 2021
Merged

Deprecate inExperiment API in favor of inExperimentSync #16848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6526517
Deprecate inExperiment API in favour of inExperimentSync
Aug 2, 2021
da101a1
Ensure experiments are activated before they are used
Aug 5, 2021
b7bf651
News entry
Aug 5, 2021
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
1 change: 1 addition & 0 deletions news/2 Fixes/16768.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix random delay before several actions.
25 changes: 1 addition & 24 deletions src/client/common/experiments/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,30 +88,7 @@ export class ExperimentService implements IExperimentService {
}

public async inExperiment(experiment: string): Promise<boolean> {
if (!this.experimentationService) {
return false;
}

// Currently the service doesn't support opting in and out of experiments.
// so we need to perform these checks manually.
if (this._optOutFrom.includes('All') || this._optOutFrom.includes(experiment)) {
return false;
}

if (this._optInto.includes('All') || this._optInto.includes(experiment)) {
// Check if the user was already in the experiment server-side. We need to do
// this to ensure the experiment service is ready and internal states are fully
// synced with the experiment server.
await this.experimentationService.getTreatmentVariableAsync(EXP_CONFIG_ID, experiment, true);
return true;
}

const treatmentVariable = await this.experimentationService.getTreatmentVariableAsync(
EXP_CONFIG_ID,
experiment,
true,
);
return treatmentVariable !== undefined;
return this.inExperimentSync(experiment);
}

public inExperimentSync(experiment: string): boolean {
Expand Down
6 changes: 5 additions & 1 deletion src/client/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { ProgressLocation, ProgressOptions, window } from 'vscode';
import { buildApi, IExtensionApi } from './api';
import { IApplicationShell } from './common/application/types';
import { traceError } from './common/logger';
import { IAsyncDisposableRegistry, IExtensionContext } from './common/types';
import { IAsyncDisposableRegistry, IExperimentService, IExtensionContext } from './common/types';
import { createDeferred } from './common/utils/async';
import { Common } from './common/utils/localize';
import { activateComponents } from './extensionActivation';
Expand Down Expand Up @@ -103,6 +103,10 @@ async function activateUnsafe(
// Note standard utils especially experiment and platform code are fundamental to the extension
// and should be available before we activate anything else.Hence register them first.
initializeStandard(ext);
// We need to activate experiments before initializing components as objects are created or not created based on experiments.
const experimentService = activatedServiceContainer.get<IExperimentService>(IExperimentService);
// This guarantees that all experiment information has loaded & all telemetry will contain experiment info.
await experimentService.activate();
const components = await initializeComponents(ext);

// Then we finish activating.
Expand Down
2 changes: 0 additions & 2 deletions src/client/extensionActivation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,6 @@ async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {
tensorBoardRegisterTypes(serviceManager);

const experimentService = serviceContainer.get<IExperimentService>(IExperimentService);
// This guarantees that all experiment information has loaded & all telemetry will contain experiment info.
await experimentService.activate();

const workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
const extensions = serviceContainer.get<IExtensions>(IExtensions);
Expand Down
170 changes: 0 additions & 170 deletions src/test/common/experiments/service.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,176 +155,6 @@ suite('Experimentation service', () => {
});
});

suite('In-experiment check', () => {
const experiment = 'Test Experiment - experiment';
let telemetryEvents: { eventName: string; properties: Record<string, unknown> }[] = [];
let getTreatmentVariableAsyncStub: sinon.SinonStub;
let sendTelemetryEventStub: sinon.SinonStub;

setup(() => {
sendTelemetryEventStub = sinon
.stub(Telemetry, 'sendTelemetryEvent')
.callsFake((eventName: string, _, properties: Record<string, unknown>) => {
const telemetry = { eventName, properties };
telemetryEvents.push(telemetry);
});

getTreatmentVariableAsyncStub = sinon.stub().returns(Promise.resolve(true));
sinon.stub(tasClient, 'getExperimentationService').returns(({
getTreatmentVariableAsync: getTreatmentVariableAsyncStub,
} as unknown) as tasClient.IExperimentationService);

configureApplicationEnvironment('stable', extensionVersion);
});

teardown(() => {
telemetryEvents = [];
});

test('If the opt-in and opt-out arrays are empty, return the value from the experimentation framework for a given experiment', async () => {
configureSettings(true, [], []);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isTrue(result);
sinon.assert.notCalled(sendTelemetryEventStub);
sinon.assert.calledOnce(getTreatmentVariableAsyncStub);
});

test('If the experiment setting is disabled, inExperiment should return false', async () => {
configureSettings(false, [], []);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isFalse(result);
sinon.assert.notCalled(sendTelemetryEventStub);
sinon.assert.notCalled(getTreatmentVariableAsyncStub);
});

test('If the opt-in setting contains "All", inExperiment should return true', async () => {
configureSettings(true, ['All'], []);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isTrue(result);
assert.strictEqual(telemetryEvents.length, 0);
});

test('If the opt-in setting contains `All`, inExperiment should check the value cached by the experiment service', async () => {
configureSettings(true, ['All'], []);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isTrue(result);
sinon.assert.notCalled(sendTelemetryEventStub);
sinon.assert.calledOnce(getTreatmentVariableAsyncStub);
});

test('If the opt-in setting contains `All` and the experiment setting is disabled, inExperiment should return false', async () => {
configureSettings(false, ['All'], []);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isFalse(result);
sinon.assert.notCalled(sendTelemetryEventStub);
sinon.assert.notCalled(getTreatmentVariableAsyncStub);
});

test('If the opt-in setting contains the experiment name, inExperiment should return true', async () => {
configureSettings(true, [experiment], []);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isTrue(result);
assert.strictEqual(telemetryEvents.length, 0);
sinon.assert.calledOnce(getTreatmentVariableAsyncStub);
});

test('If the opt-out setting contains "All", inExperiment should return false', async () => {
configureSettings(true, [], ['All']);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isFalse(result);
sinon.assert.notCalled(sendTelemetryEventStub);
sinon.assert.notCalled(getTreatmentVariableAsyncStub);
});

test('If the opt-out setting contains "All" and the experiment setting is enabled, inExperiment should return false', async () => {
configureSettings(true, [], ['All']);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isFalse(result);
sinon.assert.notCalled(sendTelemetryEventStub);
sinon.assert.notCalled(getTreatmentVariableAsyncStub);
});

test('If the opt-out setting contains the experiment name, inExperiment should return false', async () => {
configureSettings(true, [], [experiment]);

const experimentService = new ExperimentService(
instance(workspaceService),
instance(appEnvironment),
globalMemento,
outputChannel,
);
const result = await experimentService.inExperiment(experiment);

assert.isFalse(result);
assert.strictEqual(telemetryEvents.length, 0);
sinon.assert.notCalled(getTreatmentVariableAsyncStub);
});
});

suite('In-experiment-sync check', () => {
const experiment = 'Test Experiment - experiment';
let telemetryEvents: { eventName: string; properties: Record<string, unknown> }[] = [];
Expand Down