Skip to content

Log experiments run via ExP in the output panel #12742

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
kimadeline merged 9 commits into from
Jul 7, 2020
Merged

Log experiments run via ExP in the output panel #12742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4c668dd
Log experiments run via ExP in the output panel
Jul 3, 2020
ba3cfa4
Fix tests
Jul 3, 2020
6887d64
It worked on my machine ;_;
Jul 3, 2020
5473887
Filter out non-PVSC experiments
Jul 6, 2020
dd4d202
Merge branch 'main' into 12656-exp-log
Jul 6, 2020
dc89e80
Fix tests
Jul 7, 2020
6b7104a
Clarification.
Jul 7, 2020
42f6512
Clarification 2.
Jul 7, 2020
093e8a3
Case-insentitive filtering
Jul 7, 2020
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/3 Code Health/12656.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Log ExP experiments the user belongs to in the output panel.
32 changes: 28 additions & 4 deletions src/client/common/experiments/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,20 @@ import { getExperimentationService, IExperimentationService, TargetPopulation }
import { sendTelemetryEvent } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { IApplicationEnvironment } from '../application/types';
import { PVSC_EXTENSION_ID } from '../constants';
import { GLOBAL_MEMENTO, IConfigurationService, IExperimentService, IMemento, IPythonSettings } from '../types';
import { PVSC_EXTENSION_ID, STANDARD_OUTPUT_CHANNEL } from '../constants';
import {
GLOBAL_MEMENTO,
IConfigurationService,
IExperimentService,
IMemento,
IOutputChannel,
IPythonSettings
} from '../types';
import { Experiments } from '../utils/localize';
import { ExperimentationTelemetry } from './telemetry';

const EXP_MEMENTO_KEY = 'VSCode.ABExp.FeatureData';

@injectable()
export class ExperimentService implements IExperimentService {
/**
Expand All @@ -30,7 +40,8 @@ export class ExperimentService implements IExperimentService {
constructor(
@inject(IConfigurationService) readonly configurationService: IConfigurationService,
@inject(IApplicationEnvironment) private readonly appEnvironment: IApplicationEnvironment,
@inject(IMemento) @named(GLOBAL_MEMENTO) readonly globalState: Memento
@inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalState: Memento,
@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly output: IOutputChannel
) {
this.settings = configurationService.getSettings(undefined);

Expand Down Expand Up @@ -61,8 +72,10 @@ export class ExperimentService implements IExperimentService {
this.appEnvironment.packageJson.version!,
targetPopulation,
telemetryReporter,
globalState
this.globalState
);

this.logExperiments();
}

public async inExperiment(experiment: string): Promise<boolean> {
Expand Down Expand Up @@ -90,4 +103,15 @@ export class ExperimentService implements IExperimentService {

return this.experimentationService.isCachedFlightEnabled(experiment);
}

private logExperiments() {
const experiments = this.globalState.get<{ features: string[] }>(EXP_MEMENTO_KEY, { features: [] });
Copy link
Copy Markdown

@karrtikr karrtikr Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this return all experiments the user belongs to? Or all experiments running on behalf of the Python extension.

We should be logging the prior one, right? The news entry and naming is making it seem like we're logging all the experiments, and not just the one the user belongs to.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user doesn't belong to an experiment it won't be in storage, so by default it returns all experiments and control groups the user belongs to. Then because we don't want to log experiments that are not run by the Python extension (agreed on with Luciana) we filter them out.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the news entry nor the naming are ambiguous (we're logging experiments inside the Python extension, why would we log experiments that are not from the Python extension), but I updated the issue content provide an answer to your question.

Copy link
Copy Markdown

@DonJayamanne DonJayamanne Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a public API (accessing the stored experiments from the global storage), shouldn't we ask for a public API instead.
I.e. the storage key isn't public.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We asked, and they said they were working on it. If I remember correctly other teams also asked for this feature, so hopefully there will be an "official" way to do it soon. In the meantime, they are the ones who provided us with this workaround.

Copy link
Copy Markdown

@karrtikr karrtikr Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're logging experiments inside the Python extension, why would we log experiments that are not from the Python extension

What I was trying to say is, by looking at the news entry which says
Log experiments run via ExP in the output panel
It seems we're logging all experiments registered in the Python extension - whether the user is in it or not.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can amend it to be "Log ExP experiments the user belongs to in the output panel.".

Copy link
Copy Markdown

@karrtikr karrtikr Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I also recommend adding a doc comment in the method logExperiments(), which says it logs the experiment the user belongs to.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behaviour is similar as the existing experiment manager, which already only logs the experiments the user belongs to. Instead I'll clarify the existing comment to say that we filter out experiments that are not run by the Python extension.


experiments.features.forEach((exp) => {
// Filter out experiments groups that are not from the Python extension.
if (exp.toLowerCase().startsWith('python')) {
this.output.appendLine(Experiments.inGroup().format(exp));
}
});
}
}
65 changes: 54 additions & 11 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 @@ -5,16 +5,18 @@

import { assert } from 'chai';
import * as sinon from 'sinon';
import { instance, mock, when } from 'ts-mockito';
import { anything, instance, mock, when } from 'ts-mockito';
import * as tasClient from 'vscode-tas-client';
import { ApplicationEnvironment } from '../../../client/common/application/applicationEnvironment';
import { Channel, IApplicationEnvironment } from '../../../client/common/application/types';
import { ConfigurationService } from '../../../client/common/configuration/service';
import { ExperimentService } from '../../../client/common/experiments/service';
import { IConfigurationService } from '../../../client/common/types';
import { Experiments } from '../../../client/common/utils/localize';
import * as Telemetry from '../../../client/telemetry';
import { EventName } from '../../../client/telemetry/constants';
import { PVSC_EXTENSION_ID_FOR_TESTS } from '../../constants';
import { MockOutputChannel } from '../../mockClasses';
import { MockMemento } from '../../mocks/mementos';

suite('Experimentation service', () => {
Expand All @@ -23,15 +25,18 @@ suite('Experimentation service', () => {
let configurationService: IConfigurationService;
let appEnvironment: IApplicationEnvironment;
let globalMemento: MockMemento;
let outputChannel: MockOutputChannel;

setup(() => {
configurationService = mock(ConfigurationService);
appEnvironment = mock(ApplicationEnvironment);
globalMemento = new MockMemento();
outputChannel = new MockOutputChannel('');
});

teardown(() => {
sinon.restore();
Telemetry._resetSharedProperties();
});

function configureSettings(enabled: boolean, optInto: string[], optOutFrom: string[]) {
Expand Down Expand Up @@ -59,7 +64,12 @@ suite('Experimentation service', () => {
configureApplicationEnvironment('stable', extensionVersion);

// tslint:disable-next-line: no-unused-expression
new ExperimentService(instance(configurationService), instance(appEnvironment), globalMemento);
new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento,
outputChannel
);

sinon.assert.calledWithExactly(
getExperimentationServiceStub,
Expand All @@ -78,7 +88,12 @@ suite('Experimentation service', () => {
configureApplicationEnvironment('insiders', extensionVersion);

// tslint:disable-next-line: no-unused-expression
new ExperimentService(instance(configurationService), instance(appEnvironment), globalMemento);
new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento,
outputChannel
);

sinon.assert.calledWithExactly(
getExperimentationServiceStub,
Expand All @@ -99,7 +114,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);

assert.deepEqual(experimentService._optInto, ['Foo - experiment']);
Expand All @@ -113,11 +129,32 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);

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

test('Experiment data in Memento storage should be logged if it starts with "python"', () => {
const experiments = ['ExperimentOne', 'pythonExperiment'];
globalMemento = mock(MockMemento);
configureSettings(true, [], []);
configureApplicationEnvironment('stable', extensionVersion);
// tslint:disable-next-line: no-any
when(globalMemento.get(anything(), anything())).thenReturn({ features: experiments } as any);

// tslint:disable-next-line: no-unused-expression
new ExperimentService(
instance(configurationService),
instance(appEnvironment),
instance(globalMemento),
outputChannel
);
const output = `${Experiments.inGroup().format('pythonExperiment')}\n`;

assert.equal(outputChannel.output, output);
});
});

suite('In-experiment check', () => {
Expand Down Expand Up @@ -153,7 +190,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);
const result = await experimentService.inExperiment(experiment);

Expand All @@ -168,7 +206,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);
const result = await experimentService.inExperiment(experiment);

Expand All @@ -183,7 +222,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);
const result = await experimentService.inExperiment(experiment);

Expand All @@ -202,7 +242,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);
const result = await experimentService.inExperiment(experiment);

Expand All @@ -221,7 +262,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);
const result = await experimentService.inExperiment(experiment);

Expand All @@ -240,7 +282,8 @@ suite('Experimentation service', () => {
const experimentService = new ExperimentService(
instance(configurationService),
instance(appEnvironment),
globalMemento
globalMemento,
outputChannel
);
const result = await experimentService.inExperiment(experiment);

Expand Down