-
Notifications
You must be signed in to change notification settings - Fork 749
telemetry(auth): Update metrics to better debug Auth dropoff #6625
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
Changes from all commits
a5ab6c3
6f6fedb
0231f0f
969f1ec
022d858
b2b639a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ import { AuthSources } from './util' | |
| import { AuthFlowStates } from './vue/types' | ||
| import { getTelemetryMetadataForConn } from '../../auth/connection' | ||
| import { AuthUtil } from '../../codewhisperer/util/authUtil' | ||
| import { ExtensionUse } from '../../auth/utils' | ||
|
|
||
| export class CommonAuthViewProvider implements WebviewViewProvider { | ||
| public readonly viewType: string | ||
|
|
@@ -83,14 +84,22 @@ export class CommonAuthViewProvider implements WebviewViewProvider { | |
| ) { | ||
| // Our callback won't fire on the first view. | ||
| if (webviewView.visible) { | ||
| telemetry.auth_signInPageOpened.emit({ result: 'Succeeded', passive: true }) | ||
| telemetry.auth_signInPageOpened.emit({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we modify this metric and make it more general in the future?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, we need to better structure out telemetry since it is messy with what "opened" means. We can also collapse all webviews in to a single metric probably |
||
| result: 'Succeeded', | ||
| passive: true, | ||
| source: ExtensionUse.instance.sourceForTelemetry(), | ||
| }) | ||
| } | ||
|
|
||
| // This will fire whenever the user opens or closes the login page from 'somewhere else' | ||
| // i.e. NOT when switching from/to the chat window, which uses the same view area. | ||
| webviewView.onDidChangeVisibility(async () => { | ||
| if (webviewView.visible) { | ||
| telemetry.auth_signInPageOpened.emit({ result: 'Succeeded', passive: true }) | ||
| telemetry.auth_signInPageOpened.emit({ | ||
| result: 'Succeeded', | ||
| passive: true, | ||
| source: ExtensionUse.instance.sourceForTelemetry(), | ||
| }) | ||
| } else { | ||
| telemetry.auth_signInPageClosed.emit({ result: 'Succeeded', passive: true }) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,22 @@ export abstract class CommonAuthWebview extends VueWebview { | |
| return globals.regionProvider.getRegions().reverse() | ||
| } | ||
|
|
||
| private didCall: { login: boolean; reauth: boolean } = { login: false, reauth: false } | ||
| public setUiReady(state: 'login' | 'reauth') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this something we could just use the once() util for? That way you don't have to keep track of the extra state?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to emit once for |
||
| // Prevent telemetry spam, since showing/hiding chat triggers this each time. | ||
| // So only emit once. | ||
| if (this.didCall[state]) { | ||
| return | ||
| } | ||
|
|
||
| telemetry.webview_load.emit({ | ||
| passive: true, | ||
| webviewName: state, | ||
| result: 'Succeeded', | ||
| }) | ||
| this.didCall[state] = true | ||
| } | ||
|
|
||
| /** | ||
| * This wraps the execution of the given setupFunc() and handles common errors from the SSO setup process. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1131,6 +1131,17 @@ | |
| ], | ||
| "passive": true | ||
| }, | ||
| { | ||
| "name": "auth_signInPageOpened", | ||
| "description": "When the Amazon Q sign in page is opened and focused.", | ||
| "metadata": [ | ||
| { | ||
| "type": "source", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not add this to the existing metric? https://github.com/aws/aws-toolkit-common/blob/ccb16ca4f73b07c4e1cd79da159312c9ffe403a7/telemetry/definitions/commonDefinitions.json#L2915
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm being rushed to get the change out for this release, but I will port this over after |
||
| "required": true | ||
| } | ||
| ], | ||
| "passive": true | ||
| }, | ||
| { | ||
| "name": "function_call", | ||
| "description": "Represents a function call. In most cases this should wrap code with a run(), then you can add context.", | ||
|
|
@@ -1207,6 +1218,17 @@ | |
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "session_start", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? There is already a metric for starting the plugin. Adding similar, semantically-redundant metrics will make it harder to reason about the lifecycle and telemetry.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the metric?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this metric already exists. So different question is can the existing metric be updated instead , in the common repo
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I plan to do this. It was just due to time constraints. I'll follow up after this PR |
||
| "description": "Called when starting the plugin", | ||
| "metadata": [ | ||
| { | ||
| "type": "source", | ||
| "required": false | ||
| } | ||
| ], | ||
| "passive": true | ||
| }, | ||
| { | ||
| "name": "session_end", | ||
| "description": "Called when stopping the IDE on a best effort basis", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,23 +11,23 @@ import globals from '../../shared/extensionGlobals' | |
|
|
||
| describe('ExtensionUse.isFirstUse()', function () { | ||
| let instance: ExtensionUse | ||
| const notHasExistingConnections = () => false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, I'll follow up with the name change after. Don't want to have to run CI again unless theres something critical. |
||
|
|
||
| beforeEach(async function () { | ||
| instance = new ExtensionUse() | ||
| await globals.globalState.update(ExtensionUse.instance.isExtensionFirstUseKey, true) | ||
| await makeStateValueNotExist() | ||
| }) | ||
|
|
||
| it('is true only on first startup', function () { | ||
| assert.strictEqual(instance.isFirstUse(), true, 'Failed on first call.') | ||
| assert.strictEqual(instance.isFirstUse(), true, 'Failed on second call.') | ||
| assert.strictEqual(instance.isFirstUse(notHasExistingConnections), true, 'Failed on first call.') | ||
| assert.strictEqual(instance.isFirstUse(notHasExistingConnections), true, 'Failed on second call.') | ||
|
|
||
| const nextStartup = nextExtensionStartup() | ||
| assert.strictEqual(nextStartup.isFirstUse(), false, 'Failed on new startup.') | ||
| assert.strictEqual(nextStartup.isFirstUse(notHasExistingConnections), false, 'Failed on new startup.') | ||
| }) | ||
|
|
||
| it('true when: (state value not exists + NOT has existing connections)', async function () { | ||
| await makeStateValueNotExist() | ||
| const notHasExistingConnections = () => false | ||
| assert.strictEqual( | ||
| instance.isFirstUse(notHasExistingConnections), | ||
| true, | ||
|
|
||
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.
qq: why is this always true for amazonQ?
Uh oh!
There was an error while loading. Please reload this page.
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.
There was previous logic where if we detected an auth connection in Toolkit that we would not consider it a first time user. But this doesn't make sense for Q.
Also if they get this far in the function it is guaranteed they are a first time user, since they would have otherwise returned earlier