From 798a4fd808ed089a0d23585c0a888d97f547e7e7 Mon Sep 17 00:00:00 2001 From: Nikolas Komonen <118216176+nkomonen-amazon@users.noreply.github.com> Date: Thu, 24 Oct 2024 14:10:01 -0400 Subject: [PATCH] telemetry(bug): sessionId not unique (#5845) ## Problem: There seems to be a bug with vscode.env.sessionId since we observed in telemetry multiple clientIds who had the same sessionId. ## Solution: Generate our own sessionId and share it between extensions through globalThis --- License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: nkomonen-amazon Co-authored-by: Maxim Hayes <149123719+hayemaxi@users.noreply.github.com> --- packages/core/src/dev/activation.ts | 106 +++++++++--------- .../core/src/shared/telemetry/activation.ts | 4 +- packages/core/src/shared/telemetry/util.ts | 80 ++++++++----- .../src/test/shared/telemetry/util.test.ts | 15 ++- 4 files changed, 118 insertions(+), 87 deletions(-) diff --git a/packages/core/src/dev/activation.ts b/packages/core/src/dev/activation.ts index df69c018791..3c94e5d6d71 100644 --- a/packages/core/src/dev/activation.ts +++ b/packages/core/src/dev/activation.ts @@ -61,57 +61,59 @@ let targetAuth: Auth * on selection. There is no support for name-spacing. Just add the relevant * feature/module as a description so it can be moved around easier. */ -const menuOptions: Record = { - installVsix: { - label: 'Install VSIX on Remote Environment', - description: 'CodeCatalyst', - detail: 'Automatically upload/install a VSIX to a remote host', - executor: installVsixCommand, - }, - openTerminal: { - label: 'Open Remote Terminal', - description: 'CodeCatalyst', - detail: 'Opens a new terminal connected to the remote environment', - executor: openTerminalCommand, - }, - deleteDevEnv: { - label: 'Delete Workspace', - description: 'CodeCatalyst', - detail: 'Deletes the selected Dev Environment', - executor: deleteDevEnvCommand, - }, - editStorage: { - label: 'Show or Edit globalState', - description: 'VS Code', - detail: 'Shows all globalState values, or edit a globalState/secret item', - executor: openStorageFromInput, - }, - showEnvVars: { - label: 'Show Environment Variables', - description: 'AWS Toolkit', - detail: 'Shows all environment variable values', - executor: () => showState('envvars'), - }, - deleteSsoConnections: { - label: 'Auth: Delete SSO Connections', - detail: 'Deletes all SSO Connections the extension is using.', - executor: deleteSsoConnections, - }, - expireSsoConnections: { - label: 'Auth: Expire SSO Connections', - detail: 'Force expires all SSO Connections, in to a "needs reauthentication" state.', - executor: expireSsoConnections, - }, - editAuthConnections: { - label: 'Auth: Edit Connections', - detail: 'Opens editor to all Auth Connections the extension is using.', - executor: editSsoConnections, - }, - forceIdeCrash: { - label: 'Crash: Force IDE ExtHost Crash', - detail: `Will SIGKILL ExtHost, { pid: ${process.pid}, sessionId: '${getSessionId().slice(0, 8)}-...' }, but the IDE itself will not crash.`, - executor: forceQuitIde, - }, +const menuOptions: () => Record = () => { + return { + installVsix: { + label: 'Install VSIX on Remote Environment', + description: 'CodeCatalyst', + detail: 'Automatically upload/install a VSIX to a remote host', + executor: installVsixCommand, + }, + openTerminal: { + label: 'Open Remote Terminal', + description: 'CodeCatalyst', + detail: 'Opens a new terminal connected to the remote environment', + executor: openTerminalCommand, + }, + deleteDevEnv: { + label: 'Delete Workspace', + description: 'CodeCatalyst', + detail: 'Deletes the selected Dev Environment', + executor: deleteDevEnvCommand, + }, + editStorage: { + label: 'Show or Edit globalState', + description: 'VS Code', + detail: 'Shows all globalState values, or edit a globalState/secret item', + executor: openStorageFromInput, + }, + showEnvVars: { + label: 'Show Environment Variables', + description: 'AWS Toolkit', + detail: 'Shows all environment variable values', + executor: () => showState('envvars'), + }, + deleteSsoConnections: { + label: 'Auth: Delete SSO Connections', + detail: 'Deletes all SSO Connections the extension is using.', + executor: deleteSsoConnections, + }, + expireSsoConnections: { + label: 'Auth: Expire SSO Connections', + detail: 'Force expires all SSO Connections, in to a "needs reauthentication" state.', + executor: expireSsoConnections, + }, + editAuthConnections: { + label: 'Auth: Edit Connections', + detail: 'Opens editor to all Auth Connections the extension is using.', + executor: editSsoConnections, + }, + forceIdeCrash: { + label: 'Crash: Force IDE ExtHost Crash', + detail: `Will SIGKILL ExtHost, { pid: ${process.pid}, sessionId: '${getSessionId().slice(0, 8)}-...' }, but the IDE itself will not crash.`, + executor: forceQuitIde, + }, + } } /** @@ -167,7 +169,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { globalState = targetContext.globalState targetAuth = opts.auth void openMenu( - entries(menuOptions) + entries(menuOptions()) .filter((e) => (opts.menuOptions ?? Object.keys(menuOptions)).includes(e[0])) .map((e) => e[1]) ) diff --git a/packages/core/src/shared/telemetry/activation.ts b/packages/core/src/shared/telemetry/activation.ts index e1be7df6f5e..e9fa6b4c2b1 100644 --- a/packages/core/src/shared/telemetry/activation.ts +++ b/packages/core/src/shared/telemetry/activation.ts @@ -79,8 +79,8 @@ export async function activate( if (globals.telemetry.telemetryEnabled) { // Only log the IDs if telemetry is enabled, so that users who have it disabled do not think we are sending events. - getLogger().info(`Telemetry Client ID: ${globals.telemetry.clientId}`) - getLogger().info(`Telemetry Session ID: ${getSessionId()}`) + getLogger().info(`Telemetry clientId: ${globals.telemetry.clientId}`) + getLogger().info(`Telemetry sessionId: ${getSessionId()}`) } } catch (e) { // Only throw in a production build because: diff --git a/packages/core/src/shared/telemetry/util.ts b/packages/core/src/shared/telemetry/util.ts index bd6b5b206a6..4a9a2f1ff1f 100644 --- a/packages/core/src/shared/telemetry/util.ts +++ b/packages/core/src/shared/telemetry/util.ts @@ -24,7 +24,7 @@ import { Result } from './telemetry.gen' import { MetricDatum } from './clienttelemetry' import { isValidationExemptMetric } from './exemptMetrics' import { isAmazonQ, isCloud9, isSageMaker } from '../../shared/extensionUtilities' -import { randomUUID } from '../crypto' +import { isUuid, randomUUID } from '../crypto' import { ClassToInterfaceType } from '../utilities/tsUtils' import { FunctionEntry, type TelemetryTracer } from './spans' import { telemetry } from './telemetry' @@ -98,33 +98,59 @@ export function convertLegacy(value: unknown): boolean { * can be used in conjunction with the client ID to differntiate between * different VS Code windows on a users machine. * - * ### Rules: - * - * - On startup of a new application instance, a new session ID must be created. - * - This identifier must be a `UUID`, it is enforced by the telemetry service. - * - The session ID must be different from all other IDs on the machine - * - A session ID exists until the application instance is terminated. - * It should never be used again after termination. - * - All extensions on the same application instance MUST return the same session ID. - * - This will allow us to know which of our extensions (eg Q vs Toolkit) are in the - * same VS Code window. - * - * `vscode.env.sessionId` behaves as described aboved, EXCEPT its - * value looks close to a UUID by does not exactly match it (has additional characters). - * As a result we process it through uuidV5 which creates a proper UUID from it. - * uuidV5 is idempotent, so as long as `vscode.env.sessionId` returns the same value, - * we will get the same UUID. + * See spec: https://quip-amazon.com/9gqrAqwO5FCE */ -export const getSessionId = once(() => uuidV5(vscode.env.sessionId, sessionIdNonce)) -/** - * This is an arbitrary nonce that is used in creating a v5 UUID for Session ID. We only - * have this since the spec requires it. - * - This should ONLY be used by {@link getSessionId}. - * - This value MUST NOT change during runtime, otherwise {@link getSessionId} will lose its - * idempotency. But, if there was a reason to change the value in a PR, it would not be an issue. - * - This is exported only for testing. - */ -export const sessionIdNonce = '44cfdb20-b30b-4585-a66c-9f48f24f99b5' as const +export const getSessionId = once(() => SessionId.getSessionId()) + +/** IMPORTANT: Use {@link getSessionId()} only. This is exported just for testing. */ +export class SessionId { + public static getSessionId(): string { + // This implementation does not work in web + if (!isWeb()) { + return this._getSessionId() + } + // A best effort at a sessionId just for web mode + return this._getVscSessionId() + } + + /** + * This implementation assumes that the `globalThis` is shared between extensions in the same + * Extension Host, so we can share a global variable that way. + * + * This does not seem to work on web mode since the `globalThis` is not shared due to WebWorker design + */ + private static _getSessionId() { + const g = globalThis as any + if (g.amzn_sessionId === undefined || !isUuid(g.amzn_sessionId)) { + g.amzn_sessionId = randomUUID() + } + return g.amzn_sessionId + } + + /** + * `vscode.env.sessionId` looks close to a UUID by does not exactly match it (has additional characters). + * As a result we process it through uuidV5 which creates a proper UUID from it. + * uuidV5 is idempotent, so as long as `vscode.env.sessionId` returns the same value, + * we will get the same UUID. + * + * We were initially using this implementation for all session ids, but it has some caveats: + * - If the extension host crashes, sesionId stays the same since the parent VSC process defines it and that does not crash. + * We wanted it to generate a new sessionId on ext host crash. + * - This value may not be reliable, see the following sessionId in telemetry, it contains many events + * all from different client ids: `sessionId: cabea8e7-a8a1-5e51-a60e-07218f4a5937` + */ + private static _getVscSessionId() { + return uuidV5(vscode.env.sessionId, this.sessionIdNonce) + } + /** + * This is an arbitrary nonce that is used in creating a v5 UUID for Session ID. We only + * have this since the spec requires it. + * - This should ONLY be used by {@link getSessionId}. + * - This value MUST NOT change during runtime, otherwise {@link getSessionId} will lose its + * idempotency. But, if there was a reason to change the value in a PR, it would not be an issue. + */ + private static readonly sessionIdNonce = '44cfdb20-b30b-4585-a66c-9f48f24f99b5' +} /** * Calculates the clientId for the current profile. This calculation is performed once diff --git a/packages/core/src/test/shared/telemetry/util.test.ts b/packages/core/src/test/shared/telemetry/util.test.ts index 60d8ae6a4a8..627ad58f5e7 100644 --- a/packages/core/src/test/shared/telemetry/util.test.ts +++ b/packages/core/src/test/shared/telemetry/util.test.ts @@ -9,10 +9,9 @@ import { Settings } from '../../../shared/settings' import { convertLegacy, getClientId, - getSessionId, getUserAgent, platformPair, - sessionIdNonce, + SessionId, telemetryClientIdEnvKey, TelemetryConfig, } from '../../../shared/telemetry/util' @@ -112,14 +111,18 @@ describe('TelemetryConfig', function () { describe('getSessionId', function () { it('returns a stable UUID', function () { - const result = getSessionId() + const result = SessionId.getSessionId() assert.deepStrictEqual(isUuid(result), true) - assert.deepStrictEqual(getSessionId(), result, 'Subsequent call did not return the same UUID') + assert.deepStrictEqual(SessionId.getSessionId(), result, 'Subsequent call did not return the same UUID') }) - it('nonce is the same as always', function () { - assert.deepStrictEqual(sessionIdNonce, '44cfdb20-b30b-4585-a66c-9f48f24f99b5') + it('overwrites something that does not look like a UUID', function () { + ;(globalThis as any).amzn_sessionId = 'notAUUID' + const result = SessionId.getSessionId() + + assert.deepStrictEqual(isUuid(result), true) + assert.deepStrictEqual(SessionId.getSessionId(), result, 'Subsequent call did not return the same UUID') }) })