-
Notifications
You must be signed in to change notification settings - Fork 180
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
Bump @vscode/extension-telemetry
to 0.8.0
#1483
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable - just one more question aside from the in-line one:
Have you tested this via custom vsix and compared the events sent have the same shape?
@@ -48,7 +48,7 @@ let crashCount = 0; | |||
|
|||
export async function activate(context: vscode.ExtensionContext): Promise<void> { | |||
const manifest = context.extension.packageJSON; | |||
reporter = new TelemetryReporter(context.extension.id, manifest.version, manifest.appInsightsKey); | |||
reporter = new TelemetryReporter(manifest.appInsightsKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does it get the extension ID & version from now? Is that automagically read at runtime from package.json
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With 0.7.5 they removed the need to provide these parameters. I assume the new VS Code telemetry API handles this internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you confirmed the VS Code telemetry API handles that and the events land in AppInsights in the expected shape (i.e. under the expected ID & version), then 👍🏻 LGTM
This PR updates the telemetry library to the latest version. Due to some breaking changes, I had to make some minor changes.
Error tracking now requires a custom event name. Please have a look at the strings I came up with.