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

debug: filter exceptions from DAP telemetry on web #97627

Merged
merged 3 commits into from May 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/vs/editor/standalone/browser/simpleServices.ts
Expand Up @@ -548,6 +548,7 @@ export class StandaloneTelemetryService implements ITelemetryService {
_serviceBrand: undefined;

public isOptedIn = false;
public sendErrorTelemetry = false;

public setEnabled(value: boolean): void {
}
Expand Down
5 changes: 5 additions & 0 deletions src/vs/platform/telemetry/common/telemetry.ts
Expand Up @@ -23,6 +23,11 @@ export interface ITelemetryData {

export interface ITelemetryService {

/**
* Whether error telemetry will get sent. If false, `publicLogError` will no-op.
*/
readonly sendErrorTelemetry: boolean;

_serviceBrand: undefined;

/**
Expand Down
6 changes: 3 additions & 3 deletions src/vs/platform/telemetry/common/telemetryService.ts
Expand Up @@ -35,7 +35,7 @@ export class TelemetryService implements ITelemetryService {
private _piiPaths: string[];
private _userOptIn: boolean;
private _enabled: boolean;
private _sendErrorTelemetry: boolean;
public readonly sendErrorTelemetry: boolean;

private readonly _disposables = new DisposableStore();
private _cleanupPatterns: RegExp[] = [];
Expand All @@ -49,7 +49,7 @@ export class TelemetryService implements ITelemetryService {
this._piiPaths = config.piiPaths || [];
this._userOptIn = true;
this._enabled = true;
this._sendErrorTelemetry = !!config.sendErrorTelemetry;
this.sendErrorTelemetry = !!config.sendErrorTelemetry;

// static cleanup pattern for: `file:///DANGEROUS/PATH/resources/app/Useful/Information`
this._cleanupPatterns = [/file:\/\/\/.*?\/resources\/app\//gi];
Expand Down Expand Up @@ -148,7 +148,7 @@ export class TelemetryService implements ITelemetryService {
}

publicLogError(errorEventName: string, data?: ITelemetryData): Promise<any> {
if (!this._sendErrorTelemetry) {
if (!this.sendErrorTelemetry) {
return Promise.resolve(undefined);
}

Expand Down
2 changes: 2 additions & 0 deletions src/vs/platform/telemetry/common/telemetryUtils.ts
Expand Up @@ -13,6 +13,8 @@ import { isObject } from 'vs/base/common/types';

export const NullTelemetryService = new class implements ITelemetryService {
_serviceBrand: undefined;
readonly sendErrorTelemetry = false;

publicLog(eventName: string, data?: ITelemetryData) {
return Promise.resolve(undefined);
}
Expand Down
8 changes: 7 additions & 1 deletion src/vs/workbench/contrib/debug/browser/debugSession.ts
Expand Up @@ -35,6 +35,7 @@ import { INotificationService } from 'vs/platform/notification/common/notificati
import { ILifecycleService } from 'vs/platform/lifecycle/common/lifecycle';
import { localize } from 'vs/nls';
import { canceled } from 'vs/base/common/errors';
import { filterExceptionsFromTelemetry } from 'vs/workbench/contrib/debug/common/debugUtils';

export class DebugSession implements IDebugSession {

Expand Down Expand Up @@ -848,7 +849,12 @@ export class DebugSession implements IDebugSession {
// and the user opted in telemetry
if (this.raw.customTelemetryService && this.telemetryService.isOptedIn) {
// __GDPR__TODO__ We're sending events in the name of the debug extension and we can not ensure that those are declared correctly.
this.raw.customTelemetryService.publicLog(event.body.output, event.body.data);
let data = event.body.data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to move this out to a helper method which would live in debugTelemetry.ts?

Copy link
Member Author

Choose a reason for hiding this comment

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

The DebugTelemetry class isn't very accessible here; that's used for the top-level of the session and sends to the global(?) telemetry service, and for this bit of telemetry we're sending to a separate instance of the service created from the aiKey inside the debug extension's package.json.

So perhaps not a method, but I did pop this to its own helper function in debugUtils.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I like that!

if (!this.raw.customTelemetryService.sendErrorTelemetry && event.body.data) {
data = filterExceptionsFromTelemetry(event.body.data);
}

this.raw.customTelemetryService.publicLog(event.body.output, data);
}

return;
Expand Down
16 changes: 16 additions & 0 deletions src/vs/workbench/contrib/debug/common/debugUtils.ts
Expand Up @@ -23,6 +23,22 @@ export function formatPII(value: string, excludePII: boolean, args: { [key: stri
});
}

/**
* Filters exceptions (keys marked with "!") from the given object. Used to
* ensure exception data is not sent on web remotes, see #97628.
*/
export function filterExceptionsFromTelemetry<T extends { [key: string]: unknown }>(data: T): Partial<T> {
const output: Partial<T> = {};
for (const key of Object.keys(data) as (keyof T & string)[]) {
if (!key.startsWith('!')) {
output[key] = data[key];
}
}

return output;
}


export function isSessionAttach(session: IDebugSession): boolean {
return session.configuration.request === 'attach' && !getExtensionHostDebugSession(session);
}
Expand Down
12 changes: 11 additions & 1 deletion src/vs/workbench/contrib/debug/node/debugHelperService.ts
Expand Up @@ -10,10 +10,17 @@ import { getPathFromAmdModule } from 'vs/base/common/amd';
import { TelemetryService } from 'vs/platform/telemetry/common/telemetryService';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { registerSingleton } from 'vs/platform/instantiation/common/extensions';
import { IWorkbenchEnvironmentService } from 'vs/workbench/services/environment/common/environmentService';
import { cleanRemoteAuthority } from 'vs/platform/telemetry/common/telemetryUtils';

export class NodeDebugHelperService implements IDebugHelperService {
_serviceBrand: undefined;

constructor(
@IWorkbenchEnvironmentService private readonly environmentService: IWorkbenchEnvironmentService,
) { }


createTelemetryService(configurationService: IConfigurationService, args: string[]): TelemetryService | undefined {

const client = new TelemetryClient(
Expand All @@ -33,7 +40,10 @@ export class NodeDebugHelperService implements IDebugHelperService {
const channel = client.getChannel('telemetryAppender');
const appender = new TelemetryAppenderClient(channel);

return new TelemetryService({ appender, sendErrorTelemetry: true }, configurationService);
return new TelemetryService({
appender,
sendErrorTelemetry: cleanRemoteAuthority(this.environmentService.configuration.remoteAuthority) !== 'other'
}, configurationService);
}
}

Expand Down
74 changes: 74 additions & 0 deletions src/vs/workbench/contrib/debug/test/browser/telemetry.test.ts
@@ -0,0 +1,74 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as assert from 'assert';
import { MockDebugAdapter, createMockDebugModel } from 'vs/workbench/contrib/debug/test/common/mockDebug';
import { DebugModel } from 'vs/workbench/contrib/debug/common/debugModel';
import { DebugSession } from 'vs/workbench/contrib/debug/browser/debugSession';
import { generateUuid } from 'vs/base/common/uuid';
import { NullOpenerService } from 'vs/platform/opener/common/opener';
import { RawDebugSession } from 'vs/workbench/contrib/debug/browser/rawDebugSession';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { stub, SinonStub } from 'sinon';
import { timeout } from 'vs/base/common/async';

suite('Debug - DebugSession telemetry', () => {
let model: DebugModel;
let session: DebugSession;
let adapter: MockDebugAdapter;
let telemetry: { isOptedIn: boolean; sendErrorTelemetry: boolean; publicLog: SinonStub };

setup(() => {
telemetry = { isOptedIn: true, sendErrorTelemetry: true, publicLog: stub() };
adapter = new MockDebugAdapter();
model = createMockDebugModel();

const telemetryService = telemetry as Partial<ITelemetryService> as ITelemetryService;
session = new DebugSession(generateUuid(), undefined!, undefined!, model, undefined, undefined!, telemetryService, undefined!, undefined!, undefined!, undefined!, undefined!, undefined!, NullOpenerService, undefined!, undefined!);
session.initializeForTest(new RawDebugSession(adapter, undefined!, undefined!, telemetryService, undefined!, undefined!, undefined!));
});

test('does not send telemetry when opted out', async () => {
telemetry.isOptedIn = false;
adapter.sendEventBody('output', {
category: 'telemetry',
output: 'someEvent',
data: { foo: 'bar', '!err': 'oh no!' }
});

await timeout(0);
assert.strictEqual(telemetry.publicLog.callCount, 0);
});

test('logs telemetry and exceptions when enabled', async () => {
adapter.sendEventBody('output', {
category: 'telemetry',
output: 'someEvent',
data: { foo: 'bar', '!err': 'oh no!' }
});

await timeout(0);
assert.deepStrictEqual(telemetry.publicLog.args[0], [
'someEvent',
{ foo: 'bar', '!err': 'oh no!' }
]);
});

test('filters exceptions when error reporting disabled', async () => {
telemetry.sendErrorTelemetry = false;

adapter.sendEventBody('output', {
category: 'telemetry',
output: 'someEvent',
data: { foo: 'bar', '!err': 'oh no!' }
});

await timeout(0);
assert.deepStrictEqual(telemetry.publicLog.args[0], [
'someEvent',
{ foo: 'bar' }
]);
});
});
Expand Up @@ -36,6 +36,7 @@ export class TelemetryService extends Disposable implements ITelemetryService {
_serviceBrand: undefined;

private impl: ITelemetryService;
public readonly sendErrorTelemetry = false;

constructor(
@IWorkbenchEnvironmentService environmentService: IWorkbenchEnvironmentService,
Expand Down
Expand Up @@ -24,6 +24,7 @@ export class TelemetryService extends Disposable implements ITelemetryService {
_serviceBrand: undefined;

private impl: ITelemetryService;
public readonly sendErrorTelemetry: boolean;

constructor(
@IWorkbenchEnvironmentService environmentService: INativeWorkbenchEnvironmentService,
Expand All @@ -48,6 +49,8 @@ export class TelemetryService extends Disposable implements ITelemetryService {
} else {
this.impl = NullTelemetryService;
}

this.sendErrorTelemetry = this.impl.sendErrorTelemetry;
}

setEnabled(value: boolean): void {
Expand Down
Expand Up @@ -163,6 +163,7 @@ suite.skip('TextSearch performance (integration)', () => {
class TestTelemetryService implements ITelemetryService {
public _serviceBrand: undefined;
public isOptedIn = true;
public sendErrorTelemetry = true;

public events: any[] = [];

Expand Down