Add telemetry#6
Conversation
|
@brettcannon I can't see the telemetry on app insights, there could be a delay in the data coming through. Will keep monitoring. |
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
I think there's like one design question about an idea that may (or may not) make things easier to maintain, a spelling mistake in an attribute, and a change to the copyright headers.
| @@ -0,0 +1,30 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
| * Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
Less visibly jarring by dropping the --- lines, plus it doesn't mention the license file name since it's actually LICENSE in our case, but if we changed it I don't think we would want to update every single comment. 😉
There was a problem hiding this comment.
I can go back and fix it, better now
There was a problem hiding this comment.
I'll just fix the mentioning of the license file, easier than removing the lines
| // tslint:disable-next-line:prefer-type-cast | ||
| (result as Promise<void>) | ||
| .then(data => { | ||
| sendTelemetryEvent(eventName, stopWatch.elpsedTime); |
|
|
||
| export function sendTelemetryEvent(eventName: string, durationMs?: number, properties?: TelemetryProperties) { | ||
| const reporter = getTelemetryReporter(); | ||
| const measures = typeof durationMs === 'number' ? { duration: durationMs } : undefined; |
There was a problem hiding this comment.
Is there any reason we need to worry about the unit of measure changing? E.g. should as label the data as duration-ms?
There was a problem hiding this comment.
I don't think we'll every want another unit (we can always do the math at the moment of reporting)
| } | ||
|
|
||
| // tslint:disable-next-line:no-any function-name | ||
| export function captureTelemetry(eventName: string) { |
There was a problem hiding this comment.
I miss context managers from Python.
| // tslint:disable-next-line:no-invalid-this no-use-before-declare no-unsafe-any | ||
| const result = originalMethod.apply(this, args); | ||
|
|
||
| // If method being wrapped returns a promise then wait for it. |
There was a problem hiding this comment.
Is it worth having two separate functions, e.g. captureAsyncTelemetry() and captureTelemetry() to simplify this logic at all?
There was a problem hiding this comment.
I don't think so, this is a decorator function (a decorator that can be applied to any method (sync or async).
e.g.
class Somethind {
@captureTelemetry()
public doSomethingSynchronously(){ }
@captureTelemetry()
public async doSomethingAsynchronously(){ }
}| @@ -1,36 +1,30 @@ | |||
| declare module "vscode-extension-telemetry" { | |||
There was a problem hiding this comment.
Is there really no .d.ts file on npm for the package? If so, has that fact been reported upstream?
There was a problem hiding this comment.
nope, everyone seems to be creating their own dts for this damn thing... can't understand why. plan was to create a dts a create PR (separate piece of work)
| @@ -1,3 +1,7 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
This should only be added to new files, not pre-existing ones where external contributors may have made changes.
| if (stdout && stdout.length > 0) { | ||
| // Take the second part, see below example. | ||
| // pip 9.0.1 from /Users/donjayamanne/anaconda3/lib/python3.6/site-packages (python 3.6). | ||
| const parts = stdout.split(' '); |
There was a problem hiding this comment.
Is it worth using a regex to avoid potential format changes that could break this and lead to accidentally capturing PII?
| terminal.sendText(unix_code); | ||
| } | ||
| terminal.show(); | ||
| sendTelemetryEvent(EXECUTION_DJANGO, undefined, { scope: 'selection' }); |
There was a problem hiding this comment.
I take it there's no way to execute Django as a file since that option isn't listed here?
| const pyVersion = await interpreterVersion.getVersion('INVALID_INTERPRETER', 'DEFAULT_TEST_VALUE'); | ||
| assert.equal(pyVersion, 'DEFAULT_TEST_VALUE', 'Incorrect version'); | ||
| }); | ||
| test('Must return the Pip Version', async () => { |
There was a problem hiding this comment.
You definitely do not need to change this, but as an FYI, the official capitalization is "pip" (all lower-case).
There was a problem hiding this comment.
lol, i will deifinitely change this, as i'm making other chages.
…into Telemetry * 'Telemetry' of https://github.com/Microsoft/vscode-python: fixed unit tests incorrect telemetry scope for running unittests do not stop discovery when running tests
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
FYI I went through and removed the copyright header from all preexisting files. And since I was already doing that I just went ahead and tweaked the formatting for the new files.
2 participants
No description provided.