Skip to content

Commit 2391391

Browse files
DonJayamanneDonJayamanne
authored
Minimize data sent as part of error telemetry (#5858)
* Minimize data sent as part of error telemetry * Added comments * Fixed tests
1 parent 6781c61 commit 2391391

3 files changed

Lines changed: 137 additions & 122 deletions

File tree

‎news/3 Code Health/4602.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Minimize data sent as part of the `ERROR` telemetry event.

‎src/client/telemetry/index.ts‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,17 @@ export function sendTelemetryEvent<P extends IEventNamePropertyMapping, E extend
9191
const reporter = getTelemetryReporter();
9292
const measures = typeof durationMs === 'number' ? { duration: durationMs } : durationMs ? durationMs : undefined;
9393

94-
// tslint:disable-next-line:no-any
94+
if (ex && (eventName as any) !== 'ERROR') {
95+
// When sending `ERROR` telemetry event no need to send custom properties.
96+
// Else we have to review all properties everytime as part of GDPR.
97+
// Assume we have 10 events all with their own properties.
98+
// As we have errors for each event, those properties are treated as new data items.
99+
// Hence they need to be classified as part of the GDPR process, and thats unnecessary and onerous.
100+
const props: Record<string, string> = {};
101+
props.stackTrace = getStackTrace(ex);
102+
props.originalEventName = eventName as any as string;
103+
reporter.sendTelemetryEvent('ERROR', props, measures);
104+
}
95105
const customProperties: Record<string, string> = {};
96106
if (properties) {
97107
// tslint:disable-next-line:prefer-type-cast no-any
@@ -104,21 +114,7 @@ export function sendTelemetryEvent<P extends IEventNamePropertyMapping, E extend
104114
(customProperties as any)[prop] = typeof data[prop] === 'string' ? data[prop] : data[prop].toString();
105115
});
106116
}
107-
if (ex) {
108-
customProperties.stackTrace = getStackTrace(ex);
109-
}
110-
if (ex && (eventName as any) !== 'ERROR') {
111-
customProperties.originalEventName = eventName as any as string;
112-
reporter.sendTelemetryEvent('ERROR', customProperties, measures);
113-
}
114117
reporter.sendTelemetryEvent((eventName as any) as string, customProperties, measures);
115-
116-
// Enable this to debug telemetry. To be discussed whether or not we want this all of the time.
117-
// try {
118-
// traceInfo(`Telemetry: ${eventName} : ${JSON.stringify(customProperties)}`);
119-
// } catch {
120-
// noop();
121-
// }
122118
}
123119

124120
// tslint:disable-next-line:no-any function-name

‎src/test/telemetry/index.unit.test.ts‎

Lines changed: 125 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -13,41 +13,49 @@ import { correctPathForOsType } from '../common';
1313
suite('Telemetry', () => {
1414
const oldValueOfVSC_PYTHON_UNIT_TEST = process.env.VSC_PYTHON_UNIT_TEST;
1515
const oldValueOfVSC_PYTHON_CI_TEST = process.env.VSC_PYTHON_CI_TEST;
16+
17+
class Reporter {
18+
public static eventName: string[] = [];
19+
public static properties: Record<string, string>[] = [];
20+
public static measures: {}[] = [];
21+
public static clear() {
22+
Reporter.eventName = [];
23+
Reporter.properties = [];
24+
Reporter.measures = [];
25+
}
26+
public sendTelemetryEvent(eventName: string, properties?: {}, measures?: {}) {
27+
Reporter.eventName.push(eventName);
28+
Reporter.properties.push(properties!);
29+
Reporter.measures.push(measures!);
30+
}
31+
}
32+
1633
setup(() => {
1734
process.env.VSC_PYTHON_UNIT_TEST = undefined;
1835
process.env.VSC_PYTHON_CI_TEST = undefined;
1936
clearTelemetryReporter();
37+
Reporter.clear();
2038
});
2139
teardown(() => {
2240
process.env.VSC_PYTHON_UNIT_TEST = oldValueOfVSC_PYTHON_UNIT_TEST;
2341
process.env.VSC_PYTHON_CI_TEST = oldValueOfVSC_PYTHON_CI_TEST;
2442
rewiremock.disable();
2543
});
2644

27-
class Reporter {
28-
public static eventName: string;
29-
public static properties: Record<string, string>;
30-
public static measures: {};
31-
public sendTelemetryEvent(eventName: string, properties?: {}, measures?: {}) {
32-
Reporter.eventName = eventName;
33-
Reporter.properties = properties!;
34-
Reporter.measures = measures!;
35-
}
36-
}
3745
test('Send Telemetry', () => {
3846
rewiremock.enable();
3947
rewiremock('vscode-extension-telemetry').with({ default: Reporter });
4048

4149
const eventName = 'Testing';
4250
const properties = { hello: 'world', foo: 'bar' };
43-
const measuers = { start: 123, end: 987 };
51+
const measures = { start: 123, end: 987 };
4452

4553
// tslint:disable-next-line:no-any
46-
sendTelemetryEvent(eventName as any, measuers, properties as any);
54+
sendTelemetryEvent(eventName as any, measures, properties as any);
4755

48-
expect(Reporter.eventName).to.equal(eventName);
49-
expect(Reporter.measures).to.deep.equal(measuers);
50-
expect(Reporter.properties).to.deep.equal(properties);
56+
expect(Reporter.eventName).to.deep.equal([eventName]);
57+
expect(Reporter.measures).to.deep.equal([measures]);
58+
expect(Reporter.properties).to.deep.equal([properties]);
5159
});
5260
test('Send Telemetry', () => {
5361
rewiremock.enable();
@@ -57,9 +65,9 @@ suite('Telemetry', () => {
5765

5866
sendTelemetryEvent(eventName as any);
5967

60-
expect(Reporter.eventName).to.equal(eventName);
61-
expect(Reporter.measures).to.equal(undefined, 'Measures should be empty');
62-
expect(Reporter.properties).to.deep.equal({}, 'Properties should be empty');
68+
expect(Reporter.eventName).to.deep.equal([eventName]);
69+
expect(Reporter.measures).to.deep.equal([undefined], 'Measures should be empty');
70+
expect(Reporter.properties).to.deep.equal([{}], 'Properties should be empty');
6371
});
6472
test('Send Error Telemetry', () => {
6573
rewiremock.enable();
@@ -68,145 +76,155 @@ suite('Telemetry', () => {
6876

6977
const eventName = 'Testing';
7078
const properties = { hello: 'world', foo: 'bar' };
71-
const measuers = { start: 123, end: 987 };
79+
const measures = { start: 123, end: 987 };
7280

7381
// tslint:disable-next-line:no-any
74-
sendTelemetryEvent(eventName as any, measuers, properties as any, error);
82+
sendTelemetryEvent(eventName as any, measures, properties as any, error);
7583

76-
const stackTrace = Reporter.properties.stackTrace;
77-
delete Reporter.properties.stackTrace;
84+
const expectedErrorProperties = {
85+
originalEventName: eventName
86+
};
7887

79-
expect(Reporter.eventName).to.equal(eventName);
80-
expect(Reporter.measures).to.deep.equal(measuers);
81-
expect(Reporter.properties).to.deep.equal({ ...properties, originalEventName: eventName });
82-
expect(stackTrace).to.be.length.greaterThan(1);
88+
expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]);
89+
expect(Reporter.measures).to.deep.equal([measures, measures]);
90+
expect(Reporter.properties[0].stackTrace).to.be.length.greaterThan(1);
91+
delete Reporter.properties[0].stackTrace;
92+
expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]);
8393
});
8494
test('Send Error Telemetry', () => {
8595
rewiremock.enable();
8696
const error = new Error('Boo');
87-
error.stack = correctPathForOsType(`Error: Boo
88-
at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)
89-
at callFn (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:372:21)
90-
at Test.Runnable.run (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7)
91-
at Runner.runTest (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:455:10)
92-
at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:573:12
93-
at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:369:14)
94-
at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:379:7
95-
at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:303:14)
96-
at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:342:7
97-
at done (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:319:5)
98-
at callFn (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:395:7)
99-
at Hook.Runnable.run (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7)
100-
at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:317:10)
101-
at Immediate.<anonymous> (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)
102-
at runCallback (timers.js:789:20)
103-
at tryOnImmediate (timers.js:751:5)
104-
at processImmediate [as _immediateCallback] (timers.js:722:5)`);
97+
error.stack = correctPathForOsType(['Error: Boo',
98+
`at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`,
99+
`at callFn (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:372:21)`,
100+
`at Test.Runnable.run (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7)`,
101+
`at Runner.runTest (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:455:10)`,
102+
`at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:573:12`,
103+
`at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:369:14)`,
104+
`at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:379:7`,
105+
`at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:303:14)`,
106+
`at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:342:7`,
107+
`at done (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:319:5)`,
108+
`at callFn (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:395:7)`,
109+
`at Hook.Runnable.run (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7)`,
110+
`at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:317:10)`,
111+
`at Immediate.<anonymous> (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)`,
112+
'at runCallback (timers.js:789:20)',
113+
'at tryOnImmediate (timers.js:751:5)',
114+
'at processImmediate [as _immediateCallback] (timers.js:722:5)'].join('\n\t'));
105115
rewiremock('vscode-extension-telemetry').with({ default: Reporter });
106116

107117
const eventName = 'Testing';
108118
const properties = { hello: 'world', foo: 'bar' };
109-
const measuers = { start: 123, end: 987 };
119+
const measures = { start: 123, end: 987 };
110120

111121
// tslint:disable-next-line:no-any
112-
sendTelemetryEvent(eventName as any, measuers, properties as any, error);
122+
sendTelemetryEvent(eventName as any, measures, properties as any, error);
113123

114-
const stackTrace = Reporter.properties.stackTrace;
115-
delete Reporter.properties.stackTrace;
124+
const expectedErrorProperties = {
125+
originalEventName: eventName
126+
};
116127

117-
expect(Reporter.eventName).to.equal(eventName);
118-
expect(Reporter.measures).to.deep.equal(measuers);
119-
expect(Reporter.properties).to.deep.equal({ ...properties, originalEventName: eventName });
128+
const stackTrace = Reporter.properties[0].stackTrace;
129+
delete Reporter.properties[0].stackTrace;
130+
131+
expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]);
132+
expect(Reporter.measures).to.deep.equal([measures, measures]);
133+
expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]);
120134
expect(stackTrace).to.be.length.greaterThan(1);
121135

122-
// tslint:disable-next-line:no-multiline-string
123-
const expectedStack = correctPathForOsType(`at Context.test <pvsc>/src/test/telemetry/index.unit.test.ts:50:23
124-
\tat callFn <pvsc>/node_modules/mocha/lib/runnable.js:372:21
125-
\tat Test.Runnable.run <pvsc>/node_modules/mocha/lib/runnable.js:364:7
126-
\tat Runner.runTest <pvsc>/node_modules/mocha/lib/runner.js:455:10
127-
\tat <pvsc>/node_modules/mocha/lib/runner.js:573:12
128-
\tat next <pvsc>/node_modules/mocha/lib/runner.js:369:14
129-
\tat <pvsc>/node_modules/mocha/lib/runner.js:379:7
130-
\tat next <pvsc>/node_modules/mocha/lib/runner.js:303:14
131-
\tat <pvsc>/node_modules/mocha/lib/runner.js:342:7
132-
\tat done <pvsc>/node_modules/mocha/lib/runnable.js:319:5
133-
\tat callFn <pvsc>/node_modules/mocha/lib/runnable.js:395:7
134-
\tat Hook.Runnable.run <pvsc>/node_modules/mocha/lib/runnable.js:364:7
135-
\tat next <pvsc>/node_modules/mocha/lib/runner.js:317:10
136-
\tat Immediate <pvsc>/node_modules/mocha/lib/runner.js:347:5
137-
\tat runCallback <hidden>/timers.js:789:20
138-
\tat tryOnImmediate <hidden>/timers.js:751:5
139-
\tat processImmediate [as _immediateCallback] <hidden>/timers.js:722:5`);
136+
const expectedStack = correctPathForOsType(['at Context.test <pvsc>/src/test/telemetry/index.unit.test.ts:50:23\n\tat callFn <pvsc>/node_modules/mocha/lib/runnable.js:372:21',
137+
'at Test.Runnable.run <pvsc>/node_modules/mocha/lib/runnable.js:364:7',
138+
'at Runner.runTest <pvsc>/node_modules/mocha/lib/runner.js:455:10',
139+
'at <pvsc>/node_modules/mocha/lib/runner.js:573:12',
140+
'at next <pvsc>/node_modules/mocha/lib/runner.js:369:14',
141+
'at <pvsc>/node_modules/mocha/lib/runner.js:379:7',
142+
'at next <pvsc>/node_modules/mocha/lib/runner.js:303:14',
143+
'at <pvsc>/node_modules/mocha/lib/runner.js:342:7',
144+
'at done <pvsc>/node_modules/mocha/lib/runnable.js:319:5',
145+
'at callFn <pvsc>/node_modules/mocha/lib/runnable.js:395:7',
146+
'at Hook.Runnable.run <pvsc>/node_modules/mocha/lib/runnable.js:364:7',
147+
'at next <pvsc>/node_modules/mocha/lib/runner.js:317:10',
148+
'at Immediate <pvsc>/node_modules/mocha/lib/runner.js:347:5',
149+
'at runCallback <hidden>/timers.js:789:20',
150+
'at tryOnImmediate <hidden>/timers.js:751:5',
151+
'at processImmediate [as _immediateCallback] <hidden>/timers.js:722:5'].join('\n\t'));
140152

141153
expect(stackTrace).to.be.equal(expectedStack);
142154
});
143155
test('Ensure non extension file paths are stripped from stack trace', () => {
144156
rewiremock.enable();
145157
const error = new Error('Boo');
146-
error.stack = correctPathForOsType(`Error: Boo
147-
at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)
148-
at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)
149-
at Test.Runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)
150-
at Runner.runTest (\\wow\wee/node_modules/mocha/lib/runner.js:455:10)
151-
at Immediate.<anonymous> (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)`);
158+
error.stack = correctPathForOsType(['Error: Boo',
159+
`at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`,
160+
'at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)',
161+
'at Test.Runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)',
162+
'at Runner.runTest (\\wow\wee/node_modules/mocha/lib/runner.js:455:10)',
163+
`at Immediate.<anonymous> (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)`].join('\n\t'));
152164
rewiremock('vscode-extension-telemetry').with({ default: Reporter });
153165

154166
const eventName = 'Testing';
155167
const properties = { hello: 'world', foo: 'bar' };
156-
const measuers = { start: 123, end: 987 };
168+
const measures = { start: 123, end: 987 };
157169

158170
// tslint:disable-next-line:no-any
159-
sendTelemetryEvent(eventName as any, measuers, properties as any, error);
171+
sendTelemetryEvent(eventName as any, measures, properties as any, error);
172+
173+
const expectedErrorProperties = {
174+
originalEventName: eventName
175+
};
160176

161-
const stackTrace = Reporter.properties.stackTrace;
162-
delete Reporter.properties.stackTrace;
177+
const stackTrace = Reporter.properties[0].stackTrace;
178+
delete Reporter.properties[0].stackTrace;
163179

164-
expect(Reporter.eventName).to.equal(eventName);
165-
expect(Reporter.measures).to.deep.equal(measuers);
166-
expect(Reporter.properties).to.deep.equal({ ...properties, originalEventName: eventName });
180+
expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]);
181+
expect(Reporter.measures).to.deep.equal([measures, measures]);
182+
expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]);
167183
expect(stackTrace).to.be.length.greaterThan(1);
168184

169-
// tslint:disable-next-line:no-multiline-string
170-
const expectedStack = correctPathForOsType(`at Context.test <pvsc>/src/test/telemetry/index.unit.test.ts:50:23
171-
\tat callFn <hidden>/runnable.js:372:21
172-
\tat Test.Runnable.run <hidden>/runnable.js:364:7
173-
\tat Runner.runTest <hidden>/runner.js:455:10
174-
\tat Immediate <pvsc>/node_modules/mocha/lib/runner.js:347:5`);
185+
const expectedStack = correctPathForOsType(['at Context.test <pvsc>/src/test/telemetry/index.unit.test.ts:50:23',
186+
'at callFn <hidden>/runnable.js:372:21',
187+
'at Test.Runnable.run <hidden>/runnable.js:364:7',
188+
'at Runner.runTest <hidden>/runner.js:455:10',
189+
'at Immediate <pvsc>/node_modules/mocha/lib/runner.js:347:5'].join('\n\t'));
175190

176191
expect(stackTrace).to.be.equal(expectedStack);
177192
});
178193
test('Ensure non function names containing file names (unlikely, but for sake of completeness) are stripped from stack trace', () => {
179194
rewiremock.enable();
180195
const error = new Error('Boo');
181-
error.stack = correctPathForOsType(`Error: Boo
182-
at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)
183-
at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)
184-
at Test./usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)
185-
at Runner.runTest (\\wow\wee/node_modules/mocha/lib/runner.js:455:10)
186-
at Immediate.<anonymous> (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)`);
196+
error.stack = correctPathForOsType(['Error: Boo',
197+
`at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`,
198+
'at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)',
199+
'at Test./usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)',
200+
'at Runner.runTest (\\wow\wee/node_modules/mocha/lib/runner.js:455:10)',
201+
`at Immediate.<anonymous> (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)`].join('\n\t'));
187202
rewiremock('vscode-extension-telemetry').with({ default: Reporter });
188203

189204
const eventName = 'Testing';
190205
const properties = { hello: 'world', foo: 'bar' };
191-
const measuers = { start: 123, end: 987 };
206+
const measures = { start: 123, end: 987 };
192207

193208
// tslint:disable-next-line:no-any
194-
sendTelemetryEvent(eventName as any, measuers, properties as any, error);
209+
sendTelemetryEvent(eventName as any, measures, properties as any, error);
210+
211+
const expectedErrorProperties = {
212+
originalEventName: eventName
213+
};
195214

196-
const stackTrace = Reporter.properties.stackTrace;
197-
delete Reporter.properties.stackTrace;
215+
const stackTrace = Reporter.properties[0].stackTrace;
216+
delete Reporter.properties[0].stackTrace;
198217

199-
expect(Reporter.eventName).to.equal(eventName);
200-
expect(Reporter.measures).to.deep.equal(measuers);
201-
expect(Reporter.properties).to.deep.equal({ ...properties, originalEventName: eventName });
218+
expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]);
219+
expect(Reporter.measures).to.deep.equal([measures, measures]);
220+
expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]);
202221
expect(stackTrace).to.be.length.greaterThan(1);
203222

204-
// tslint:disable-next-line:no-multiline-string
205-
const expectedStack = correctPathForOsType(`at Context.test <pvsc>/src/test/telemetry/index.unit.test.ts:50:23
206-
\tat callFn <hidden>/runnable.js:372:21
207-
\tat <hidden>.run <hidden>/runnable.js:364:7
208-
\tat Runner.runTest <hidden>/runner.js:455:10
209-
\tat Immediate <pvsc>/node_modules/mocha/lib/runner.js:347:5`);
223+
const expectedStack = correctPathForOsType(['at Context.test <pvsc>/src/test/telemetry/index.unit.test.ts:50:23',
224+
'at callFn <hidden>/runnable.js:372:21',
225+
'at <hidden>.run <hidden>/runnable.js:364:7',
226+
'at Runner.runTest <hidden>/runner.js:455:10',
227+
'at Immediate <pvsc>/node_modules/mocha/lib/runner.js:347:5'].join('\n\t'));
210228

211229
expect(stackTrace).to.be.equal(expectedStack);
212230
});

0 commit comments

Comments
 (0)