Skip to content
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.

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

Add support for VS Code's experiment service #11979

Merged
merged 28 commits into from
Jun 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
6f19e0b
Add npm package
kimadeline May 25, 2020
bf7bd56
News entry
kimadeline May 25, 2020
b5bef69
experiments.ts -> experiments/manager.ts
kimadeline May 25, 2020
9502146
Wrong issue number
kimadeline May 25, 2020
b0683b9
eexperimentGroups -> experiments/experimentGroups
kimadeline May 25, 2020
3271dc7
Move experiments.unit.tests.ts -> experiments/
kimadeline May 25, 2020
e500572
Commit new files
kimadeline May 26, 2020
89e3935
Merge branch 'master' into 10008-exp-service
kimadeline May 26, 2020
3def3dd
Merge gone sideways
kimadeline May 26, 2020
821bada
Add types
kimadeline May 26, 2020
168c028
Add opt in/out handling
kimadeline May 27, 2020
cd37cba
Activation (service registry)
kimadeline May 27, 2020
7a37473
Don't pin tas-client to one version
kimadeline May 27, 2020
79f5a24
Unit tests + fixes
kimadeline May 27, 2020
622bc96
Merge branch 'master' into 10008-exp-service
kimadeline May 27, 2020
d9ca2ca
Lol forgot to remove a comment + add headers
kimadeline May 27, 2020
e73f68e
Forgot 'use strict' in service.ts
kimadeline May 27, 2020
c303506
Use IApplicationEnvironment instead of IExtensions
kimadeline May 28, 2020
9e75931
Merge branch 'master' into 10008-exp-service
kimadeline May 28, 2020
fbe5ff3
Apply suggestions from code review
kimadeline May 28, 2020
c0a2337
Remove unnecessary formatted props
kimadeline May 28, 2020
a9e1267
n e v e r m i n d
kimadeline May 28, 2020
cc1f165
Aight fixed it
kimadeline May 28, 2020
060c062
flight -> experiment
kimadeline May 28, 2020
fc29418
Check stub calls instead of ctor impl
kimadeline May 29, 2020
fd28d4b
removed getExperimentService stub check
kimadeline May 29, 2020
ccb8f67
Set shared properties for all telemetry events
kimadeline May 29, 2020
4b2d931
Add test for shared properties
kimadeline Jun 2, 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/1 Enhancements/10790.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Integrate VS Code experiment framework in the extension.
47 changes: 47 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3055,6 +3055,7 @@
"vscode-languageclient": "^6.2.0-next.2",
"vscode-languageserver": "^6.2.0-next.2",
"vscode-languageserver-protocol": "^3.16.0-next.2",
"vscode-tas-client": "^0.0.757",
"vsls": "^0.3.1291",
"winreg": "^1.2.4",
"winston": "^3.2.1",
Expand Down
91 changes: 91 additions & 0 deletions src/client/common/experiments/service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

'use strict';

import { inject, named } from 'inversify';
import { Memento } from 'vscode';
import { getExperimentationService, IExperimentationService, TargetPopulation } from 'vscode-tas-client';
import { sendTelemetryEvent } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { IApplicationEnvironment } from '../application/types';
import { GLOBAL_MEMENTO, IConfigurationService, IExperimentService, IMemento, IPythonSettings } from '../types';
import { ExperimentationTelemetry } from './telemetry';

export class ExperimentService implements IExperimentService {
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
/**
* Experiments the user requested to opt into manually.
*/
public _optInto: string[] = [];
/**
* Experiments the user requested to opt out from manually.
*/
public _optOutFrom: string[] = [];

private readonly experimentationService?: IExperimentationService;
private readonly settings: IPythonSettings;

constructor(
@inject(IConfigurationService) readonly configurationService: IConfigurationService,
@inject(IApplicationEnvironment) private readonly appEnvironment: IApplicationEnvironment,
@inject(IMemento) @named(GLOBAL_MEMENTO) readonly globalState: Memento
) {
this.settings = configurationService.getSettings(undefined);
kimadeline marked this conversation as resolved.
Show resolved Hide resolved

// Users can only opt in or out of experiment groups, not control groups.
const optInto = this.settings.experiments.optInto;
const optOutFrom = this.settings.experiments.optOutFrom;
this._optInto = optInto.filter((exp) => !exp.endsWith('control'));
this._optOutFrom = optOutFrom.filter((exp) => !exp.endsWith('control'));

// Don't initialize the experiment service if the extension's experiments setting is disabled.
const enabled = this.settings.experiments.enabled;
if (!enabled) {
return;
}

let targetPopulation: TargetPopulation;

if (this.appEnvironment.channel === 'insiders') {
targetPopulation = TargetPopulation.Insiders;
} else {
targetPopulation = TargetPopulation.Public;
}

kimadeline marked this conversation as resolved.
Show resolved Hide resolved
const telemetryReporter = new ExperimentationTelemetry();

this.experimentationService = getExperimentationService(
this.appEnvironment.extensionName,
this.appEnvironment.packageJson.version!,
targetPopulation,
telemetryReporter,
globalState
);
}

public async inExperiment(experiment: string): Promise<boolean> {
karrtikr marked this conversation as resolved.
Show resolved Hide resolved
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
if (!this.experimentationService) {
return false;
}

// Currently the service doesn't support opting in and out of experiments,
// so we need to perform these checks and send the corresponding telemetry manually.
if (this._optOutFrom.includes('All') || this._optOutFrom.includes(experiment)) {
sendTelemetryEvent(EventName.PYTHON_EXPERIMENTS_OPT_IN_OUT, undefined, {
expNameOptedOutOf: experiment
});

return false;
}

if (this._optInto.includes('All') || this._optInto.includes(experiment)) {
sendTelemetryEvent(EventName.PYTHON_EXPERIMENTS_OPT_IN_OUT, undefined, {
expNameOptedInto: experiment
});

return true;
}

return this.experimentationService.isCachedFlightEnabled(experiment);
karrtikr marked this conversation as resolved.
Show resolved Hide resolved
}
}
24 changes: 24 additions & 0 deletions src/client/common/experiments/telemetry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

'use strict';

import { IExperimentationTelemetry } from 'vscode-tas-client';
import { sendTelemetryEvent, setSharedProperty } from '../../telemetry';

export class ExperimentationTelemetry implements IExperimentationTelemetry {
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
public setSharedProperty(name: string, value: string): void {
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
// Add the shared property to all telemetry being sent, not just events being sent by the experimentation package.
setSharedProperty(name, value);
}
kimadeline marked this conversation as resolved.
Show resolved Hide resolved

public postEvent(eventName: string, properties: Map<string, string>): void {
const formattedProperties: { [key: string]: string } = {};
properties.forEach((value, key) => {
formattedProperties[key] = value;
});

// tslint:disable-next-line: no-any
sendTelemetryEvent(eventName as any, undefined, formattedProperties);
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
}
}
4 changes: 3 additions & 1 deletion src/client/common/serviceRegistry.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import { IExtensionSingleActivationService } from '../activation/types';
import { IFileDownloader, IHttpClient, IInterpreterPathService } from '../common/types';
import { IExperimentService, IFileDownloader, IHttpClient, IInterpreterPathService } from '../common/types';
import { LiveShareApi } from '../datascience/liveshare/liveshare';
import { INotebookExecutionLogger } from '../datascience/types';
import { IServiceManager } from '../ioc/types';
Expand Down Expand Up @@ -42,6 +42,7 @@ import { ConfigurationService } from './configuration/service';
import { CryptoUtils } from './crypto';
import { EditorUtils } from './editor';
import { ExperimentsManager } from './experiments/manager';
import { ExperimentService } from './experiments/service';
import { FeatureDeprecationManager } from './featureDeprecationManager';
import {
ExtensionInsidersDailyChannelRule,
Expand Down Expand Up @@ -149,6 +150,7 @@ export function registerTypes(serviceManager: IServiceManager) {
serviceManager.addSingleton<ILiveShareApi>(ILiveShareApi, LiveShareApi);
serviceManager.addSingleton<ICryptoUtils>(ICryptoUtils, CryptoUtils);
serviceManager.addSingleton<IExperimentsManager>(IExperimentsManager, ExperimentsManager);
serviceManager.addSingleton<IExperimentService>(IExperimentService, ExperimentService);

serviceManager.addSingleton<ITerminalHelper>(ITerminalHelper, TerminalHelper);
serviceManager.addSingleton<ITerminalActivationCommandProvider>(
Expand Down
8 changes: 8 additions & 0 deletions src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,14 @@ export interface IExperimentsManager {
sendTelemetryIfInExperiment(experimentName: string): void;
}

/**
* Experiment service leveraging VS Code's experiment framework.
*/
export const IExperimentService = Symbol('IExperimentService');
export interface IExperimentService {
inExperiment(experimentName: string): Promise<boolean>;
}

export type InterpreterConfigurationScope = { uri: Resource; configTarget: ConfigurationTarget };
export type InspectInterpreterSettingType = {
globalValue?: string;
Expand Down
21 changes: 21 additions & 0 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,24 @@ export function isTelemetryDisabled(workspaceService: IWorkspaceService): boolea
return settings.globalValue === false ? true : false;
}

// Shared properties set by the IExperimentationTelemetry implementation.
const sharedProperties: Record<string, string> = {};
/**
* Set shared properties for all telemetry events.
*/
export function setSharedProperty(name: string, value: string): void {
sharedProperties[name] = value;
}

/**
* Reset shared properties for testing purposes.
*/
export function _resetSharedProperties(): void {
for (const key of Object.keys(sharedProperties)) {
delete sharedProperties[key];
}
}

let telemetryReporter: TelemetryReporter | undefined;
function getTelemetryReporter() {
if (!isTestExecution() && telemetryReporter) {
Expand Down Expand Up @@ -123,6 +141,9 @@ export function sendTelemetryEvent<P extends IEventNamePropertyMapping, E extend
});
}

// Add shared properties to telemetry props (we may overwrite existing ones).
Object.assign(customProperties, sharedProperties);
Copy link

Choose a reason for hiding this comment

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

Can you please clarify the purpose of sharedProperties?

Copy link
Author

Choose a reason for hiding this comment

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

It's required by the telemetry library:

The setSharedProperty method is intended to add the shared property to all telemetry being sent from your extension, not just events being sent by the experimentation package.

#11979 (comment)

Copy link

@karrtikr karrtikr Jun 2, 2020

Choose a reason for hiding this comment

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

I understand, but what I meant was if you have any idea why is it required, what kind of properties are those, etc.

Choose a reason for hiding this comment

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

The properties that get attached to your telemetry events just specify what flights the user is a part of - this allows you to compute A/B metrics based off of any of your telemetry events. Without this, you'd have to do some complicated joining or only compute metrics based off of events that have the flight info attached to them.

Copy link

Choose a reason for hiding this comment

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

I see, makes sense. Mind adding this to the property description @kimadeline ?

Copy link
Author

Choose a reason for hiding this comment

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

I think this would be more suited in the integration guide that I am currently writing.

Copy link

Choose a reason for hiding this comment

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

It's just that it's not clear when reading the code on what a "shared property" is.

Copy link

Choose a reason for hiding this comment

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

I would've expected this to be addressed before merging.


reporter.sendTelemetryEvent(eventNameSent, customProperties, measures);
}

Expand Down
Loading