telemetry(messages): maybeShowMinVscodeWarning #5762
Conversation
|
This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required. |
| component: 'editor', | ||
| } | ||
| ): Thenable<string | undefined> { | ||
| return telemetry.toolkit_showNotification.run(async (span) => { |
There was a problem hiding this comment.
nice idea. Does it make sense to also take in the "then" callback as an argument so that we can wrap the entire flow of messge + descision and report a final result based on what they clicked? It could be the selected item from items in the reason field
There was a problem hiding this comment.
Yeah I need to think more about this, could you show a pseudocode example of what you have in mind?
Meanwhile, I will push a lower-risk change for today's release.
There was a problem hiding this comment.
function showMessage(..., onResponse: (choice: string) => Promise<void>) {
return telemetry.toolkit_showNotification.run(span => {
span.record({passive: true, ...metric})
const response = await _showMessage()
span.record({reason: reponse}) // now we also know what their selection was
await onResponse(reponse)
})
function _showMessage() {
switch (kind) {
case 'info':
return vscode.window.showInformationMessage(message, options, ...items)
case 'warn':
return vscode.window.showWarningMessage(message, options, ...items)
case 'error':
default:
return vscode.window.showErrorMessage(message, options, ...items)
}
}
}
// extensionStartup.ts, how the new function call would look
// but with how many args we have I wonder if the args should be an object now
void showMessage(
'warn',
`${productName()} will soon require VS Code ${minVscode} or newer. The currently running version ${vscode.version} will no longer receive updates.`,
[updateButton, localizedText.dontShow],
{},
{
id: 'versionNotification',
component: 'editor',
reason: 'unsupportedVersion',
},
async (resp) => {
if (resp === updateButton) {
await vscode.commands.executeCommand('update.checkForUpdate')
} else if (resp === localizedText.dontShow) {
void settings.disablePrompt('minIdeVersion')
}
})
There was a problem hiding this comment.
Ah, I think I can make that work in a followup PR.
| { modal: useModal }, | ||
| { | ||
| id: 'showMessageWithUrl', | ||
| reasonDesc: getTelemetryReasonDesc(message), |
There was a problem hiding this comment.
Setting reasonDesc based on message. Maybe in the future, showMessageWithUrl should allow the caller to set id.
## Problem no metrics for notifications. ## Solution add `toolkit_showNotification` to `showMessage` and use it in `maybeShowMinVscodeWarning`.
2 participants
Problem
no metrics for notifications.
Solution
add
toolkit_showNotificationtoshowMessageand use it inmaybeShowMinVscodeWarning.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.