Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 17 additions & 8 deletions packages/core/src/amazonq/webview/messages/messageDispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import globals from '../../../shared/extensionGlobals'
import { openUrl } from '../../../shared/utilities/vsCodeUtils'
import { DefaultAmazonQAppInitContext } from '../../apps/initContext'

const qChatModuleName = 'amazonqChat'

export function dispatchWebViewMessagesToApps(
webview: Webview,
webViewToAppsMessagePublishers: Map<TabType, MessagePublisher<any>>
Expand All @@ -29,8 +31,8 @@ export function dispatchWebViewMessagesToApps(
* This would be equivalent of the duration between "user clicked open q" and "ui has become available"
* NOTE: Amazon Q UI is only loaded ONCE. The state is saved between each hide/show of the webview.
*/
telemetry.webview_load.emit({
Copy link
Contributor

Choose a reason for hiding this comment

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

Once this is done for all the webviews, will webview_load be deprecated in favor of the more granular metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, webview_x will be deprecated for something like toolkit_moduleX. So we'll also drop webview_error as well

webviewName: 'amazonq',
telemetry.toolkit_didLoadModule.emit({
module: qChatModuleName,
duration: performance.measure(amazonqMark.uiReady, amazonqMark.open).duration,
result: 'Succeeded',
})
Expand Down Expand Up @@ -86,12 +88,19 @@ export function dispatchWebViewMessagesToApps(
}

if (msg.type === 'error') {
const event = msg.event === 'webview_load' ? telemetry.webview_load : telemetry.webview_error
event.emit({
webviewName: 'amazonqChat',
result: 'Failed',
reasonDesc: msg.errorMessage,
})
if (msg.event === 'toolkit_didLoadModule') {
telemetry.toolkit_didLoadModule.emit({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the webview_error metric exist? Shouldn't it in theory mirror the failures of toolkit_didLoadModule or are there non-error reasons a webview fails to load?

If we have a separate metric for error, shouldn't we emit it in this case since we still received an error from the webview?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking of dropping webview_error and creating something like toolkit_moduleError. This will capture any errors that happen after loading has happened, anything before would be captured in toolkit_didLoadModule.

I have it as a TODO to deprecate webview_error, this will also allow us to deal with different field names (module vs webviewName)

Copy link
Contributor

Choose a reason for hiding this comment

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

toolkit_moduleError. This will capture any errors that happen after loading has happened, anything before would be captured in toolkit_didLoadModule.

? errors should be part of all metrics. There should not be a separate "foo_error" metric.

module: qChatModuleName,
result: 'Failed',
reasonDesc: msg.errorMessage,
})
} else {
telemetry.webview_error.emit({
webviewName: qChatModuleName,
result: 'Failed',
reasonDesc: msg.errorMessage,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there a separate webview_error metiric? The error should be part of the toolkit_didLoadModule metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didLoadModule is mainly intended for the initial load, but if there is an error post-load then we will want a separate metric for that. Example is after clicking the "submit" button after putting in the startUrl+Region for signin

})
}
return
}

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/amazonq/webview/ui/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export const createMynahUI = (
const { error, message } = e
ideApi.postMessage({
type: 'error',
event: connector.isUIReady ? 'webview_error' : 'webview_load',
event: connector.isUIReady ? 'webview_error' : 'toolkit_didLoadModule',
errorMessage: error ? error.toString() : message,
})
})
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/login/webview/commonAuthViewProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,10 @@ export class CommonAuthViewProvider implements WebviewViewProvider {
enableCommandUris: true,
localResourceRoots: [dist, resources],
}
webviewView.webview.html = this._getHtmlForWebview(this.extensionContext.extensionUri, webviewView.webview)
// register the webview server
await this.webView?.setup(webviewView.webview)

webviewView.webview.html = this._getHtmlForWebview(this.extensionContext.extensionUri, webviewView.webview)
}

private _getHtmlForWebview(extensionURI: Uri, webview: vscode.Webview) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const className = 'AmazonQLoginWebview'
export class AmazonQLoginWebview extends CommonAuthWebview {
public override id: string = 'aws.amazonq.AmazonCommonAuth'
public static sourcePath: string = 'vue/src/login/webview/vue/amazonq/index.js'
public override supportsLoadTelemetry: boolean = true

override onActiveConnectionModified = new vscode.EventEmitter<void>()

Expand Down
23 changes: 15 additions & 8 deletions packages/core/src/login/webview/vue/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,25 @@ export abstract class CommonAuthWebview extends VueWebview {
}

private didCall: { login: boolean; reauth: boolean } = { login: false, reauth: false }
public setUiReady(state: 'login' | 'reauth') {
// Prevent telemetry spam, since showing/hiding chat triggers this each time.
// So only emit once.
/**
* Called when the UI load process is completed, regardless of success or failure
*
* @param errorMessage IF an error is caught on the frontend, this is the message. It will result in a failure metric.
* Otherwise we assume success.
*/
public setUiReady(state: 'login' | 'reauth', errorMessage?: string) {
// Only emit once to prevent telemetry spam, since showing/hiding chat triggers this each time.
// TODO: Research how to not trigger this on every show/hide
if (this.didCall[state]) {
return
}

telemetry.webview_load.emit({
passive: true,
webviewName: state,
result: 'Succeeded',
})
if (errorMessage) {
this.setLoadFailure(state, errorMessage)
} else {
this.setDidLoad(state)
}

this.didCall[state] = true
}

Expand Down
15 changes: 13 additions & 2 deletions packages/core/src/login/webview/vue/login.vue
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@
></SelectableItem>
<button
class="continue-button"
id="connection-selection-continue-button"
:disabled="selectedLoginOption === 0"
v-on:click="handleContinueClick()"
>
Expand Down Expand Up @@ -368,8 +369,6 @@ export default defineComponent({
// Pre-select the first available login option
await this.preselectLoginOption()
await this.handleUrlInput() // validate the default startUrl

await client.setUiReady('login')
},
methods: {
toggleItemSelection(itemId: number) {
Expand Down Expand Up @@ -590,6 +589,18 @@ export default defineComponent({
},
},
})

/**
* The ID of the element we will use to determine that the UI has completed its initial load.
*
* This makes assumptions that we will be in a certain state of the UI (eg showing a form vs. a loading bar).
* So if the UI flow changes, this may need to be updated.
*/
export function getReadyElementId() {
// On every initial load, we ASSUME that the user will always be in the connection selection state,
// which is why we specifically look for this button.
return 'connection-selection-continue-button'
}
</script>

<style>
Expand Down
19 changes: 13 additions & 6 deletions packages/core/src/login/webview/vue/reauthenticate.vue
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ and the final results are retrieved by the frontend. For this Component to updat
</div>

<div>
<button id="reauthenticate" v-on:click="reauthenticate">Re-authenticate</button>
<button id="reauthenticate-button" v-on:click="reauthenticate">Re-authenticate</button>
<div v-if="errorMessage" id="error-message" style="color: red">{{ errorMessage }}</div>
</div>

Expand Down Expand Up @@ -127,9 +127,6 @@ export default defineComponent({

this.doShow = true
},
async mounted() {
await client.setUiReady('reauth')
},
methods: {
async reauthenticate() {
client.emitUiClick('auth_reauthenticate')
Expand All @@ -148,6 +145,16 @@ export default defineComponent({
},
},
})

/**
* The ID of the element we will use to determine that the UI has completed its initial load.
*
* This makes assumptions that we will be in a certain state of the UI (eg showing a form vs. a loading bar).
* So if the UI flow changes, this may need to be updated.
*/
export function getReadyElementId() {
return 'reauthenticate-button'
}
</script>
<style>
@import './base.css';
Expand Down Expand Up @@ -199,7 +206,7 @@ export default defineComponent({
flex-direction: column;
}

button#reauthenticate {
button#reauthenticate-button {
cursor: pointer;
background-color: var(--vscode-button-background);
color: white;
Expand Down Expand Up @@ -231,7 +238,7 @@ button#cancel {
cursor: pointer;
}

body.vscode-high-contrast:not(body.vscode-high-contrast-light) button#reauthenticate {
body.vscode-high-contrast:not(body.vscode-high-contrast-light) button#reauthenticate-button {
background-color: white;
color: black;
}
Expand Down
73 changes: 71 additions & 2 deletions packages/core/src/login/webview/vue/root.vue
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ configure app to AMAZONQ if for Amazon Q login
</template>
<script lang="ts">
import { PropType, defineComponent } from 'vue'
import Login from './login.vue'
import Reauthenticate from './reauthenticate.vue'
import Login, { getReadyElementId as getLoginReadyElementId } from './login.vue'
import Reauthenticate, { getReadyElementId as getReauthReadyElementId } from './reauthenticate.vue'
import { AuthFlowState, FeatureId } from './types'
import { WebviewClientFactory } from '../../../webviews/client'
import { CommonAuthWebview } from './backend'
Expand Down Expand Up @@ -51,15 +51,84 @@ export default defineComponent({
})
await this.refreshAuthState()
// We were recieving the 'load' event before refreshAuthState() resolved (I'm assuming behavior w/ Vue + browser loading not blocking),
// so post refreshAuhState() if we detect we already loaded, then execute immediately since the event already happened.
if (didLoad) {
handleLoaded()
} else {
window.addEventListener('load', () => {
handleLoaded()
})
}
},
methods: {
async refreshAuthState() {
await client.refreshAuthState()
this.authFlowState = await client.getAuthState()
// Used for telemetry purposes
if (this.authFlowState === 'LOGIN') {
;(window as any).uiState = 'login'
;(window as any).uiReadyElementId = getLoginReadyElementId()
} else if (this.authFlowState && this.authFlowState !== undefined) {
;(window as any).uiState = 'reauth'
;(window as any).uiReadyElementId = getReauthReadyElementId()
}
this.refreshKey += 1
},
},
})
// ---- START ---- The following handles the process of indicating the UI has loaded successfully.
// TODO: Move this in to a reusable class for other webviews, it feels a bit messy here
let didSetReady = false
// Setup error handlers to report. This may not actually be able to catch certain errors that we'd expect,
// so this may have to be revisited.
window.onerror = function (message) {
if (didSetReady) {
return
}
setUiReady((window as any).uiState, message.toString())
}
document.addEventListener(
'error',
(e) => {
if (didSetReady) {
return
}
setUiReady((window as any).uiState, e.message)
},
true
)
let didLoad = false
window.addEventListener('load', () => {
didLoad = true
})
const handleLoaded = () => {
// in case some unexpected behavior triggers this flow again, skip since we already emitted for this instance
if (didSetReady) {
return
}
const foundElement = !!document.getElementById((window as any).uiReadyElementId)
if (!foundElement) {
setUiReady((window as any).uiState, `Could not find element: ${(window as any).uiReadyElementId}`)
} else {
// Successful load!
setUiReady((window as any).uiState)
}
}
const setUiReady = (state: 'login' | 'reauth', errorMessage?: string) => {
client.setUiReady(state, errorMessage)
didSetReady = true
}
// ---- END ----
</script>
<style>
body {
Expand Down
10 changes: 0 additions & 10 deletions packages/core/src/shared/telemetry/vscodeTelemetry.json
Original file line number Diff line number Diff line change
Expand Up @@ -985,16 +985,6 @@
],
"passive": true
},
{
"name": "webview_load",
"description": "Represents a webview load event",
"metadata": [
{
"type": "webviewName",
"required": true
}
]
},
{
"name": "webview_error",
"description": "Represents an error that occurs in a webview",
Expand Down
Loading
Loading