bug(telemetry): sessionId not unique

nkomonen-amazon · nkomonen-amazon · commit 800b6a572549 · 2024-10-24T10:20:40.000-04:00
Problem:

There seems to be a bug with vscode.env.sessionId since we observed in telemetry
multiple clientIds who has the same sessionId.

Solution:

Generate our own sessionId and share it between extensions through globalThis

Signed-off-by: nkomonen-amazon &lt;nkomonen@amazon.com&gt;
diff --git 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<DevFunction, MenuOption> = {
-    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<DevFunction, MenuOption> = () => {
+    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<void> {
             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
@@ -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
@@ -24,11 +24,11 @@ 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'
-import { v5 as uuidV5 } from 'uuid'
+import { v4 as uuidV4, v5 as uuidV5 } from 'uuid'
 
 const legacySettingsTelemetryValueDisable = 'Disable'
 const legacySettingsTelemetryValueEnable = 'Enable'
@@ -98,33 +98,60 @@ 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 = uuidV4()
+        }
+        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.
+     * - This is exported only for testing.
+     */
+    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
@@ -12,7 +12,7 @@ import {
     getSessionId,
     getUserAgent,
     platformPair,
-    sessionIdNonce,
+    SessionId,
     telemetryClientIdEnvKey,
     TelemetryConfig,
 } from '../../../shared/telemetry/util'
@@ -112,14 +112,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')
     })
 })
 
