Skip to content
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
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 {
/**
* 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);

// 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;
}

const telemetryReporter = new ExperimentationTelemetry();

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

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 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);
}
}
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 {
public setSharedProperty(name: string, value: string): void {
// Add the shared property to all telemetry being sent, not just events being sent by the experimentation package.
setSharedProperty(name, value);
}

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);
}
}
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