Skip to content

telemetry(messages): maybeShowMinVscodeWarning #5762

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 10, 2024
Merged

telemetry(messages): maybeShowMinVscodeWarning #5762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 19 additions & 13 deletions packages/core/src/shared/extensionStartup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import { fs } from '../shared/fs/fs'
import { getIdeProperties, getIdeType, isAmazonQ, isCloud9, isCn, productName } from './extensionUtilities'
import * as localizedText from './localizedText'
import { AmazonQPromptSettings, ToolkitPromptSettings } from './settings'
import { showMessage } from './utilities/messages'
import { getTelemetryReasonDesc } from './errors'

const localize = nls.loadMessageBundle()

Expand All @@ -26,20 +28,24 @@ export async function maybeShowMinVscodeWarning(minVscode: string) {
return
}
const updateButton = `Update ${vscode.env.appName}`
const msg = `${productName()} will soon require VS Code ${minVscode} or newer. The currently running version ${vscode.version} will no longer receive updates.`
if (getIdeType() === 'vscode' && semver.lt(vscode.version, minVscode)) {
void vscode.window
.showWarningMessage(
`${productName()} will soon require VS Code ${minVscode} or newer. The currently running version ${vscode.version} will no longer receive updates.`,
updateButton,
localizedText.dontShow
)
.then(async (resp) => {
if (resp === updateButton) {
await vscode.commands.executeCommand('update.checkForUpdate')
} else if (resp === localizedText.dontShow) {
void settings.disablePrompt('minIdeVersion')
}
})
void showMessage(
'warn',
msg,
[updateButton, localizedText.dontShow],
{},
{
id: 'maybeShowMinVscodeWarning',
reasonDesc: getTelemetryReasonDesc(msg),
}
).then(async (resp) => {
if (resp === updateButton) {
await vscode.commands.executeCommand('update.checkForUpdate')
} else if (resp === localizedText.dontShow) {
void settings.disablePrompt('minIdeVersion')
}
})
}
}

Expand Down
59 changes: 44 additions & 15 deletions packages/core/src/shared/utilities/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import { getIcon, codicon } from '../icons'
import globals from '../extensionGlobals'
import { openUrl } from './vsCodeUtils'
import { AmazonQPromptSettings, ToolkitPromptSettings } from '../../shared/settings'
import { telemetry } from '../telemetry/telemetry'
import { telemetry, ToolkitShowNotification } from '../telemetry/telemetry'
import { vscodeComponent } from '../vscode/commands2'
import { getTelemetryReasonDesc } from '../errors'

export const messages = {
editCredentials(icon: boolean) {
Expand All @@ -35,21 +36,31 @@ export function makeFailedWriteMessage(filename: string): string {
return message
}

function showMessageWithItems(
message: string,
export function showMessage(
kind: 'info' | 'warn' | 'error' = 'error',
message: string,
items: string[] = [],
useModal: boolean = false
options: vscode.MessageOptions & { telemetry?: boolean } = {},
metric: Partial<ToolkitShowNotification> = {}
): Thenable<string | undefined> {
switch (kind) {
case 'info':
return vscode.window.showInformationMessage(message, { modal: useModal }, ...items)
case 'warn':
return vscode.window.showWarningMessage(message, { modal: useModal }, ...items)
case 'error':
default:
return vscode.window.showErrorMessage(message, { modal: useModal }, ...items)
}
return telemetry.toolkit_showNotification.run(async (span) => {
Copy link
Contributor

@nkomonen-amazon nkomonen-amazon Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@justinmk3 justinmk3 Oct 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nkomonen-amazon nkomonen-amazon Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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')
            }
        })

Copy link
Contributor Author

@justinmk3 justinmk3 Oct 10, 2024

Choose a reason for hiding this comment

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

Ah, I think I can make that work in a followup PR.

Copy link
Contributor Author

@justinmk3 justinmk3 Oct 11, 2024

Choose a reason for hiding this comment

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

#5776

span.record({
passive: true,
id: 'unknown',
component: 'editor',
...metric,
})

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)
}
})
}

/**
Expand All @@ -75,7 +86,16 @@ export async function showMessageWithUrl(
const uri = typeof url === 'string' ? vscode.Uri.parse(url) : url
const items = [...extraItems, urlItem]

const p = showMessageWithItems(message, kind, items, useModal)
const p = showMessage(
kind,
message,
items,
{ modal: useModal },
{
id: 'showMessageWithUrl',
reasonDesc: getTelemetryReasonDesc(message),
Copy link
Contributor Author

@justinmk3 justinmk3 Oct 10, 2024

Choose a reason for hiding this comment

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

Setting reasonDesc based on message. Maybe in the future, showMessageWithUrl should allow the caller to set id.

}
)
return p.then<string | undefined>((selection) => {
if (selection === urlItem) {
void openUrl(uri)
Expand All @@ -102,7 +122,16 @@ export async function showViewLogsMessage(
const logsItem = localize('AWS.generic.message.viewLogs', 'View Logs...')
const items = [...extraItems, logsItem]

const p = showMessageWithItems(message, kind, items)
const p = showMessage(
kind,
message,
items,
{},
{
id: 'showViewLogsMessage',
reasonDesc: getTelemetryReasonDesc(message),
}
)
return p.then<string | undefined>((selection) => {
if (selection === logsItem) {
globals.logOutputChannel.show(true)
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/test/shared/extensionUtilities.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import globals from '../../shared/extensionGlobals'
import { createQuickStartWebview, maybeShowMinVscodeWarning } from '../../shared/extensionStartup'
import { fs } from '../../shared'
import { getTestWindow } from './vscode/window'
import { assertTelemetry } from '../testUtil'

describe('extensionUtilities', function () {
it('maybeShowMinVscodeWarning', async () => {
Expand All @@ -30,6 +31,7 @@ describe('extensionUtilities', function () {
/will soon require .* 99\.0\.0 or newer. The currently running version .* will no longer receive updates./
const msg = await getTestWindow().waitForMessage(expectedMsg)
msg.close()
assertTelemetry('toolkit_showNotification', [])
})

describe('createQuickStartWebview', async function () {
Expand Down
Loading