From 9f24127bae95e5a6f36f12dfd00d3e849b966bac Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 11:09:12 -0500 Subject: [PATCH 01/44] add dummy command --- packages/amazonq/package.json | 5 +++++ packages/core/package.nls.json | 1 + packages/core/src/extension.ts | 5 +++++ packages/toolkit/package.json | 5 +++++ 4 files changed, 16 insertions(+) diff --git a/packages/amazonq/package.json b/packages/amazonq/package.json index 9e144c171a3..b9f09c19688 100644 --- a/packages/amazonq/package.json +++ b/packages/amazonq/package.json @@ -595,6 +595,11 @@ "title": "%AWS.command.viewLogs%", "category": "%AWS.amazonq.title%" }, + { + "command": "aws.amazonq.viewActiveProcesses", + "title": "%AWS.command.viewActiveProcesses%", + "category": "%AWS.title%" + }, { "command": "aws.amazonq.github", "title": "%AWS.command.github%", diff --git a/packages/core/package.nls.json b/packages/core/package.nls.json index cab4f52dcee..63c703949b7 100644 --- a/packages/core/package.nls.json +++ b/packages/core/package.nls.json @@ -176,6 +176,7 @@ "AWS.command.submitFeedback": "Send Feedback...", "AWS.command.downloadSchemaItemCode": "Download Code Bindings", "AWS.command.viewLogs": "View Logs", + "AWS.command.viewActiveProcesses": "View Active Subprocesses", "AWS.command.cloudWatchLogs.searchLogGroup": "Search Log Group", "AWS.command.cloudWatchLogs.tailLogGroup": "Tail Log Group", "AWS.command.sam.newTemplate": "Create new SAM Template", diff --git a/packages/core/src/extension.ts b/packages/core/src/extension.ts index 00fd730b490..bc8bcf7681c 100644 --- a/packages/core/src/extension.ts +++ b/packages/core/src/extension.ts @@ -171,6 +171,11 @@ export async function activateCommon( await activateViewsShared(extContext.extensionContext) + context.subscriptions.push( + Commands.register(`aws.${contextPrefix}.viewActiveProcesses`, () => + console.log('running viewActiveProcesses command') + ) + ) return extContext } diff --git a/packages/toolkit/package.json b/packages/toolkit/package.json index b93cd063334..ded124972f2 100644 --- a/packages/toolkit/package.json +++ b/packages/toolkit/package.json @@ -3146,6 +3146,11 @@ "title": "%AWS.command.viewLogs%", "category": "%AWS.title%" }, + { + "command": "aws.toolkit.viewActiveProcesses", + "title": "%AWS.command.viewActiveProcesses%", + "category": "%AWS.title%" + }, { "command": "aws.toolkit.help", "title": "%AWS.command.help%", From 2d316751ca55779853fe9e8ca50c55d4f3376523 Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 11:23:42 -0500 Subject: [PATCH 02/44] convert tracker to singleton class --- packages/core/src/shared/utilities/processUtils.ts | 10 ++++++++-- .../src/test/shared/utilities/processUtils.test.ts | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index de6d5875c79..b7ad97fc822 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -74,10 +74,16 @@ export class ChildProcessTracker { cpu: 50, } static readonly logger = getLogger('childProcess') + static #instance: ChildProcessTracker + + static get instance(): ChildProcessTracker { + return (this.#instance ??= new ChildProcessTracker()) + } + #processByPid: Map = new Map() #pids: PollingSet - public constructor() { + private constructor() { this.#pids = new PollingSet(ChildProcessTracker.pollingInterval, () => this.monitor()) } @@ -197,7 +203,7 @@ export class ChildProcessTracker { * - call and await run to get the results (pass or fail) */ export class ChildProcess { - static #runningProcesses = new ChildProcessTracker() + static #runningProcesses = ChildProcessTracker.instance static stopTimeout = 3000 #childProcess: proc.ChildProcess | undefined #processErrors: Error[] = [] diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index 3a12ae01df5..d68b697ca33 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -389,7 +389,7 @@ describe('ChildProcessTracker', function () { before(function () { clock = installFakeClock() - tracker = new ChildProcessTracker() + tracker = ChildProcessTracker.instance usageMock = sinon.stub(ChildProcessTracker.prototype, 'getUsage') }) From ee81478d7475ec61c3d2cf7590e08c0b7193a81d Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 12:07:45 -0500 Subject: [PATCH 03/44] log all usage at info level --- packages/core/src/extension.ts | 6 +- .../core/src/shared/utilities/processUtils.ts | 59 +++++++++++++------ 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/packages/core/src/extension.ts b/packages/core/src/extension.ts index bc8bcf7681c..ca2fad9be32 100644 --- a/packages/core/src/extension.ts +++ b/packages/core/src/extension.ts @@ -52,6 +52,7 @@ import { registerCommands } from './commands' import endpoints from '../resources/endpoints.json' import { getLogger, maybeShowMinVscodeWarning, setupUninstallHandler } from './shared' import { showViewLogsMessage } from './shared/utilities/messages' +import { ChildProcessTracker } from './shared/utilities/processUtils' disableAwsSdkWarning() @@ -172,8 +173,9 @@ export async function activateCommon( await activateViewsShared(extContext.extensionContext) context.subscriptions.push( - Commands.register(`aws.${contextPrefix}.viewActiveProcesses`, () => - console.log('running viewActiveProcesses command') + Commands.register( + `aws.${contextPrefix}.viewActiveProcesses`, + async () => await ChildProcessTracker.instance.logAllUsage() ) ) return extContext diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index b7ad97fc822..d9db0180241 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -2,7 +2,6 @@ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. * SPDX-License-Identifier: Apache-2.0 */ - import * as proc from 'child_process' // eslint-disable-line no-restricted-imports import * as crossSpawn from 'cross-spawn' import * as logger from '../logger' @@ -62,7 +61,7 @@ export interface ChildProcessResult { } export const eof = Symbol('EOF') - +type pid = number export interface ProcessStats { memory: number cpu: number @@ -70,26 +69,27 @@ export interface ProcessStats { export class ChildProcessTracker { static readonly pollingInterval: number = 10000 // Check usage every 10 seconds static readonly thresholds: ProcessStats = { - memory: 100 * 1024 * 1024, // 100 MB + memory: 100, // 100 MB cpu: 50, } - static readonly logger = getLogger('childProcess') + readonly logger: logger.Logger static #instance: ChildProcessTracker static get instance(): ChildProcessTracker { return (this.#instance ??= new ChildProcessTracker()) } - #processByPid: Map = new Map() - #pids: PollingSet + #processByPid: Map = new Map() + #pids: PollingSet private constructor() { + this.logger = getLogger('childProcess') this.#pids = new PollingSet(ChildProcessTracker.pollingInterval, () => this.monitor()) } private cleanUp() { const terminatedProcesses = Array.from(this.#pids.values()).filter( - (pid: number) => this.#processByPid.get(pid)?.stopped + (pid: pid) => this.#processByPid.get(pid)?.stopped ) for (const pid of terminatedProcesses) { this.delete(pid) @@ -98,26 +98,26 @@ export class ChildProcessTracker { private async monitor() { this.cleanUp() - ChildProcessTracker.logger.debug(`Active running processes size: ${this.#pids.size}`) + this.logger.debug(`Active running processes size: ${this.#pids.size}`) for (const pid of this.#pids.values()) { await this.checkProcessUsage(pid) } } - private async checkProcessUsage(pid: number): Promise { + private async checkProcessUsage(pid: pid): Promise { if (!this.#pids.has(pid)) { - ChildProcessTracker.logger.warn(`Missing process with id ${pid}`) + this.logger.warn(`Missing process with id ${pid}`) return } const stats = await this.getUsage(pid) if (stats) { - ChildProcessTracker.logger.debug(`Process ${pid} usage: %O`, stats) + this.logger.debug(`Process ${pid} usage: %O`, stats) if (stats.memory > ChildProcessTracker.thresholds.memory) { - ChildProcessTracker.logger.warn(`Process ${pid} exceeded memory threshold: ${stats.memory}`) + this.logger.warn(`Process ${pid} exceeded memory threshold: ${stats.memory}`) } if (stats.cpu > ChildProcessTracker.thresholds.cpu) { - ChildProcessTracker.logger.warn(`Process ${pid} exceeded cpu threshold: ${stats.cpu}`) + this.logger.warn(`Process ${pid} exceeded cpu threshold: ${stats.cpu}`) } } } @@ -128,7 +128,7 @@ export class ChildProcessTracker { this.#pids.start(pid) } - public delete(childProcessId: number) { + public delete(childProcessId: pid) { this.#processByPid.delete(childProcessId) this.#pids.delete(childProcessId) } @@ -149,7 +149,30 @@ export class ChildProcessTracker { this.#processByPid.clear() } - public async getUsage(pid: number): Promise { + public async getAllUsage(): Promise> { + return new Map( + await Promise.all( + Array.from(this.#pids).map(async (pid: pid) => { + const stats = await this.getUsage(pid) + return [pid, stats] as const + }) + ) + ) + } + + public async logAllUsage(): Promise { + if (this.size === 0) { + this.logger.info('No active subprocesses') + return + } + const usage = await this.getAllUsage() + const logMsg = Array.from(usage.entries()).reduce((acc, [pid, stats]) => { + return acc + `Process ${pid}: ${stats.cpu}% cpu, ${stats.memory} MB of memory\n` + }, '') + this.logger.info(`Active Subprocesses (${this.size} active)\n${logMsg}`) + } + + public async getUsage(pid: pid): Promise { const getWindowsUsage = () => { const cpuOutput = proc .execFileSync('wmic', [ @@ -170,7 +193,7 @@ export class ChildProcessTracker { return { cpu: isNaN(cpuPercentage) ? 0 : cpuPercentage, - memory: memoryBytes, + memory: memoryBytes / (1024 * 1024), } } const getUnixUsage = () => { @@ -183,14 +206,14 @@ export class ChildProcessTracker { return { cpu: isNaN(cpuPercentage) ? 0 : cpuPercentage, - memory: memoryBytes, + memory: memoryBytes / (1024 * 1024), } } try { // isWin() leads to circular dependency. return process.platform === 'win32' ? getWindowsUsage() : getUnixUsage() } catch (e) { - ChildProcessTracker.logger.warn(`Failed to get process stats for ${pid}: ${e}`) + this.logger.warn(`Failed to get process stats for ${pid}: ${e}`) return { cpu: 0, memory: 0 } } } From 63855940559970df6ab8ba3d577df8bbe1e6e99b Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 12:19:13 -0500 Subject: [PATCH 04/44] switch command name --- packages/amazonq/package.json | 4 ++-- packages/core/package.nls.json | 2 +- packages/core/src/extension.ts | 2 +- packages/toolkit/package.json | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/amazonq/package.json b/packages/amazonq/package.json index b9f09c19688..3dc7cf4cff5 100644 --- a/packages/amazonq/package.json +++ b/packages/amazonq/package.json @@ -596,8 +596,8 @@ "category": "%AWS.amazonq.title%" }, { - "command": "aws.amazonq.viewActiveProcesses", - "title": "%AWS.command.viewActiveProcesses%", + "command": "aws.amazonq.logActiveProcceses", + "title": "%AWS.command.logActiveProcceses%", "category": "%AWS.title%" }, { diff --git a/packages/core/package.nls.json b/packages/core/package.nls.json index 63c703949b7..ffd1c9b8d00 100644 --- a/packages/core/package.nls.json +++ b/packages/core/package.nls.json @@ -176,7 +176,7 @@ "AWS.command.submitFeedback": "Send Feedback...", "AWS.command.downloadSchemaItemCode": "Download Code Bindings", "AWS.command.viewLogs": "View Logs", - "AWS.command.viewActiveProcesses": "View Active Subprocesses", + "AWS.command.logActiveProcceses": "Log All Active Subprocesses", "AWS.command.cloudWatchLogs.searchLogGroup": "Search Log Group", "AWS.command.cloudWatchLogs.tailLogGroup": "Tail Log Group", "AWS.command.sam.newTemplate": "Create new SAM Template", diff --git a/packages/core/src/extension.ts b/packages/core/src/extension.ts index ca2fad9be32..49398797495 100644 --- a/packages/core/src/extension.ts +++ b/packages/core/src/extension.ts @@ -174,7 +174,7 @@ export async function activateCommon( context.subscriptions.push( Commands.register( - `aws.${contextPrefix}.viewActiveProcesses`, + `aws.${contextPrefix}.logActiveProcceses`, async () => await ChildProcessTracker.instance.logAllUsage() ) ) diff --git a/packages/toolkit/package.json b/packages/toolkit/package.json index ded124972f2..1bfeb3b6ce6 100644 --- a/packages/toolkit/package.json +++ b/packages/toolkit/package.json @@ -3147,8 +3147,8 @@ "category": "%AWS.title%" }, { - "command": "aws.toolkit.viewActiveProcesses", - "title": "%AWS.command.viewActiveProcesses%", + "command": "aws.toolkit.logActiveProcceses", + "title": "%AWS.command.logActiveProcceses%", "category": "%AWS.title%" }, { From b3d1fd4b8c4477d930d63234790adbe6edac6ae4 Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 12:55:12 -0500 Subject: [PATCH 05/44] add testing --- .../core/src/shared/utilities/processUtils.ts | 2 +- .../shared/utilities/processUtils.test.ts | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index d9db0180241..90e8c8d5cb4 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -162,7 +162,7 @@ export class ChildProcessTracker { public async logAllUsage(): Promise { if (this.size === 0) { - this.logger.info('No active subprocesses') + this.logger.info('No Active Subprocesses') return } const usage = await this.getAllUsage() diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index d68b697ca33..7198e13a1e2 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -400,6 +400,7 @@ describe('ChildProcessTracker', function () { after(function () { clock.uninstall() + usageMock.restore() }) it(`removes stopped processes every ${ChildProcessTracker.pollingInterval / 1000} seconds`, async function () { @@ -500,3 +501,38 @@ describe('ChildProcessTracker', function () { await stopAndWait(runningProcess) }) }) + +describe('ChildProcessTracker.logAllUsage', function () { + let tracker: ChildProcessTracker + + before(function () { + tracker = ChildProcessTracker.instance + }) + + afterEach(function () { + tracker.clear() + }) + + it('logAllUsage includes only active processes', async function () { + const runningProcess1 = startSleepProcess() + const runningProcess2 = startSleepProcess() + + tracker.add(runningProcess1.childProcess) + tracker.add(runningProcess2.childProcess) + + assert.ok(tracker.has(runningProcess1.childProcess), 'Missing first process') + assert.ok(tracker.has(runningProcess2.childProcess), 'Missing seconds process') + + await stopAndWait(runningProcess1) + + await tracker.logAllUsage() + + assert.throws(() => assertLogsContain(runningProcess1.childProcess.pid().toString(), false, 'info')) + assertLogsContain(runningProcess2.childProcess.pid().toString(), false, 'info') + }) + + it('logAllUsage defaults to empty message when empty', async function () { + await tracker.logAllUsage() + assertLogsContain('No Active Subprocesses', false, 'info') + }) +}) From be8fffbf51d0b3526f14959af57394b5b9f08d9c Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 13:49:07 -0500 Subject: [PATCH 06/44] refactor tests to avoid mocking when not necessary --- .../shared/utilities/processUtils.test.ts | 194 +++++++++--------- 1 file changed, 98 insertions(+), 96 deletions(-) diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index 7198e13a1e2..d27b378d5e5 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -384,133 +384,135 @@ function startSleepProcess(timeout: number = 90): RunningProcess { describe('ChildProcessTracker', function () { let tracker: ChildProcessTracker - let clock: FakeTimers.InstalledClock - let usageMock: sinon.SinonStub before(function () { - clock = installFakeClock() tracker = ChildProcessTracker.instance - usageMock = sinon.stub(ChildProcessTracker.prototype, 'getUsage') }) afterEach(function () { tracker.clear() - usageMock.reset() }) - after(function () { - clock.uninstall() - usageMock.restore() - }) - - it(`removes stopped processes every ${ChildProcessTracker.pollingInterval / 1000} seconds`, async function () { - // Start a 'sleep' command, check it only removes after we stop it. - const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) - assert.strictEqual(tracker.has(runningProcess.childProcess), true, 'failed to add sleep command') - - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assert.strictEqual(tracker.has(runningProcess.childProcess), true, 'process was mistakenly removed') - await stopAndWait(runningProcess) - - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assert.strictEqual(tracker.has(runningProcess.childProcess), false, 'process was not removed after stopping') - }) + describe('tracking functionality', function () { + let clock: FakeTimers.InstalledClock + let usageMock: sinon.SinonStub - it('multiple processes from same command are tracked seperately', async function () { - const runningProcess1 = startSleepProcess() - const runningProcess2 = startSleepProcess() - tracker.add(runningProcess1.childProcess) - tracker.add(runningProcess2.childProcess) + before(function () { + clock = installFakeClock() + tracker = ChildProcessTracker.instance + usageMock = sinon.stub(ChildProcessTracker.prototype, 'getUsage') + }) - assert.strictEqual(tracker.has(runningProcess1.childProcess), true, 'Missing first process') - assert.strictEqual(tracker.has(runningProcess2.childProcess), true, 'Missing second process') + afterEach(function () { + usageMock.reset() + }) - await stopAndWait(runningProcess1) - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assert.strictEqual(tracker.has(runningProcess2.childProcess), true, 'second process was mistakenly removed') - assert.strictEqual( - tracker.has(runningProcess1.childProcess), - false, - 'first process was not removed after stopping it' - ) - - await stopAndWait(runningProcess2) - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assert.strictEqual( - tracker.has(runningProcess2.childProcess), - false, - 'second process was not removed after stopping it' - ) - - assert.strictEqual(tracker.size, 0, 'expected tracker to be empty') - }) + after(function () { + clock.uninstall() + usageMock.restore() + }) - it('logs a warning message when system usage exceeds threshold', async function () { - const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) + it(`removes stopped processes every ${ChildProcessTracker.pollingInterval / 1000} seconds`, async function () { + // Start a 'sleep' command, check it only removes after we stop it. + const runningProcess = startSleepProcess() + tracker.add(runningProcess.childProcess) + assert.strictEqual(tracker.has(runningProcess.childProcess), true, 'failed to add sleep command') + + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assert.strictEqual(tracker.has(runningProcess.childProcess), true, 'process was mistakenly removed') + await stopAndWait(runningProcess) + + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assert.strictEqual( + tracker.has(runningProcess.childProcess), + false, + 'process was not removed after stopping' + ) + }) - const highCpu: ProcessStats = { - cpu: ChildProcessTracker.thresholds.cpu + 1, - memory: 0, - } - const highMemory: ProcessStats = { - cpu: 0, - memory: ChildProcessTracker.thresholds.memory + 1, - } + it('multiple processes from same command are tracked seperately', async function () { + const runningProcess1 = startSleepProcess() + const runningProcess2 = startSleepProcess() + tracker.add(runningProcess1.childProcess) + tracker.add(runningProcess2.childProcess) + + assert.strictEqual(tracker.has(runningProcess1.childProcess), true, 'Missing first process') + assert.strictEqual(tracker.has(runningProcess2.childProcess), true, 'Missing second process') + + await stopAndWait(runningProcess1) + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assert.strictEqual(tracker.has(runningProcess2.childProcess), true, 'second process was mistakenly removed') + assert.strictEqual( + tracker.has(runningProcess1.childProcess), + false, + 'first process was not removed after stopping it' + ) + + await stopAndWait(runningProcess2) + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assert.strictEqual( + tracker.has(runningProcess2.childProcess), + false, + 'second process was not removed after stopping it' + ) + + assert.strictEqual(tracker.size, 0, 'expected tracker to be empty') + }) - usageMock.resolves(highCpu) + it('logs a warning message when system usage exceeds threshold', async function () { + const runningProcess = startSleepProcess() + tracker.add(runningProcess.childProcess) - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assertLogsContain('exceeded cpu threshold', false, 'warn') + const highCpu: ProcessStats = { + cpu: ChildProcessTracker.thresholds.cpu + 1, + memory: 0, + } + const highMemory: ProcessStats = { + cpu: 0, + memory: ChildProcessTracker.thresholds.memory + 1, + } - usageMock.resolves(highMemory) - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assertLogsContain('exceeded memory threshold', false, 'warn') + usageMock.resolves(highCpu) - await stopAndWait(runningProcess) - }) + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assertLogsContain('exceeded cpu threshold', false, 'warn') - it('includes pid in logs', async function () { - const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) + usageMock.resolves(highMemory) + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assertLogsContain('exceeded memory threshold', false, 'warn') - usageMock.resolves({ - cpu: ChildProcessTracker.thresholds.cpu + 1, - memory: 0, + await stopAndWait(runningProcess) }) - await clock.tickAsync(ChildProcessTracker.pollingInterval) - assertLogsContain(runningProcess.childProcess.pid().toString(), false, 'warn') + it('includes pid in logs', async function () { + const runningProcess = startSleepProcess() + tracker.add(runningProcess.childProcess) - await stopAndWait(runningProcess) - }) + usageMock.resolves({ + cpu: ChildProcessTracker.thresholds.cpu + 1, + memory: 0, + }) - it('does not log for processes within threshold', async function () { - const runningProcess = startSleepProcess() + await clock.tickAsync(ChildProcessTracker.pollingInterval) + assertLogsContain(runningProcess.childProcess.pid().toString(), false, 'warn') - usageMock.resolves({ - cpu: ChildProcessTracker.thresholds.cpu - 1, - memory: ChildProcessTracker.thresholds.memory - 1, + await stopAndWait(runningProcess) }) - await clock.tickAsync(ChildProcessTracker.pollingInterval) - - assert.throws(() => assertLogsContain(runningProcess.childProcess.pid().toString(), false, 'warn')) + it('does not log for processes within threshold', async function () { + const runningProcess = startSleepProcess() - await stopAndWait(runningProcess) - }) -}) + usageMock.resolves({ + cpu: ChildProcessTracker.thresholds.cpu - 1, + memory: ChildProcessTracker.thresholds.memory - 1, + }) -describe('ChildProcessTracker.logAllUsage', function () { - let tracker: ChildProcessTracker + await clock.tickAsync(ChildProcessTracker.pollingInterval) - before(function () { - tracker = ChildProcessTracker.instance - }) + assert.throws(() => assertLogsContain(runningProcess.childProcess.pid().toString(), false, 'warn')) - afterEach(function () { - tracker.clear() + await stopAndWait(runningProcess) + }) }) it('logAllUsage includes only active processes', async function () { From 6572e6b64d77fabc3107f8a3f7b1562b3deaa744 Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 14:03:47 -0500 Subject: [PATCH 07/44] add changelog --- .../Feature-db34038d-d2bf-4b6e-b4dc-ac84c0156ab5.json | 4 ++++ .../Feature-82e8146f-5dec-4af6-8f55-482471fdb4ad.json | 4 ++++ 2 files changed, 8 insertions(+) create mode 100644 packages/amazonq/.changes/next-release/Feature-db34038d-d2bf-4b6e-b4dc-ac84c0156ab5.json create mode 100644 packages/toolkit/.changes/next-release/Feature-82e8146f-5dec-4af6-8f55-482471fdb4ad.json diff --git a/packages/amazonq/.changes/next-release/Feature-db34038d-d2bf-4b6e-b4dc-ac84c0156ab5.json b/packages/amazonq/.changes/next-release/Feature-db34038d-d2bf-4b6e-b4dc-ac84c0156ab5.json new file mode 100644 index 00000000000..75a5b1179e0 --- /dev/null +++ b/packages/amazonq/.changes/next-release/Feature-db34038d-d2bf-4b6e-b4dc-ac84c0156ab5.json @@ -0,0 +1,4 @@ +{ + "type": "Feature", + "description": "Subprocesses are now tracked and visible by \"Log All Active Subprocesses\" in command palette" +} diff --git a/packages/toolkit/.changes/next-release/Feature-82e8146f-5dec-4af6-8f55-482471fdb4ad.json b/packages/toolkit/.changes/next-release/Feature-82e8146f-5dec-4af6-8f55-482471fdb4ad.json new file mode 100644 index 00000000000..75a5b1179e0 --- /dev/null +++ b/packages/toolkit/.changes/next-release/Feature-82e8146f-5dec-4af6-8f55-482471fdb4ad.json @@ -0,0 +1,4 @@ +{ + "type": "Feature", + "description": "Subprocesses are now tracked and visible by \"Log All Active Subprocesses\" in command palette" +} From 31fd2b942b7aec61172507bf3fd40c7a15e79efb Mon Sep 17 00:00:00 2001 From: hkobew Date: Tue, 7 Jan 2025 14:32:51 -0500 Subject: [PATCH 08/44] add tests for telemetry --- .../src/shared/telemetry/vscodeTelemetry.json | 15 +++++++++++++ .../core/src/shared/utilities/processUtils.ts | 22 +++++++++++-------- .../shared/utilities/processUtils.test.ts | 18 ++++++++++++++- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/packages/core/src/shared/telemetry/vscodeTelemetry.json b/packages/core/src/shared/telemetry/vscodeTelemetry.json index 5e32b249d4f..5fb07e912fc 100644 --- a/packages/core/src/shared/telemetry/vscodeTelemetry.json +++ b/packages/core/src/shared/telemetry/vscodeTelemetry.json @@ -1,5 +1,10 @@ { "types": [ + { + "name": "size", + "type": "int", + "description": "A generic size for when units are clear" + }, { "name": "amazonGenerateApproachLatency", "type": "double", @@ -372,6 +377,16 @@ } ] }, + { + "name": "ide_logActiveProcesses", + "description": "Emitted when user invokes logActiveProcesses command", + "metadata": [ + { + "type": "size", + "required": true + } + ] + }, { "name": "vscode_executeCommand", "description": "Emitted whenever a registered Toolkit command is executed", diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index 90e8c8d5cb4..e87fc648b48 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -8,6 +8,7 @@ import * as logger from '../logger' import { Timeout, CancellationError, waitUntil } from './timeoutUtils' import { PollingSet } from './pollingSet' import { getLogger } from '../logger/logger' +import { telemetry } from '../telemetry' export interface RunParameterContext { /** Reports an error parsed from the stdin/stdout streams. */ @@ -161,15 +162,18 @@ export class ChildProcessTracker { } public async logAllUsage(): Promise { - if (this.size === 0) { - this.logger.info('No Active Subprocesses') - return - } - const usage = await this.getAllUsage() - const logMsg = Array.from(usage.entries()).reduce((acc, [pid, stats]) => { - return acc + `Process ${pid}: ${stats.cpu}% cpu, ${stats.memory} MB of memory\n` - }, '') - this.logger.info(`Active Subprocesses (${this.size} active)\n${logMsg}`) + telemetry.ide_logActiveProcesses.run(async (span) => { + span.record({ size: this.size }) + if (this.size === 0) { + this.logger.info('No Active Subprocesses') + return + } + const usage = await this.getAllUsage() + const logMsg = Array.from(usage.entries()).reduce((acc, [pid, stats]) => { + return acc + `Process ${pid}: ${stats.cpu}% cpu, ${stats.memory} MB of memory\n` + }, '') + this.logger.info(`Active Subprocesses (${this.size} active)\n${logMsg}`) + }) } public async getUsage(pid: pid): Promise { diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index d27b378d5e5..87c6f8a5587 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -19,7 +19,7 @@ import { sleep } from '../../../shared/utilities/timeoutUtils' import { Timeout, waitUntil } from '../../../shared/utilities/timeoutUtils' import { fs } from '../../../shared' import * as FakeTimers from '@sinonjs/fake-timers' -import { installFakeClock } from '../../testUtil' +import { assertTelemetry, installFakeClock } from '../../testUtil' import { isWin } from '../../../shared/vscode/env' import { assertLogsContain } from '../../globalSetup.test' @@ -537,4 +537,20 @@ describe('ChildProcessTracker', function () { await tracker.logAllUsage() assertLogsContain('No Active Subprocesses', false, 'info') }) + + it('logAllUsage emits telemetry with size equal to number of processes (empty)', async function () { + await tracker.logAllUsage() + assertTelemetry('ide_logActiveProcesses', { size: 0 }) + }) + + it('logsAllUsage emits telemetry to number of processes (nonempty)', async function () { + const size = 10 + for (const _ of Array.from({ length: size })) { + const runningProcess = startSleepProcess() + tracker.add(runningProcess.childProcess) + } + + await tracker.logAllUsage() + assertTelemetry('ide_logActiveProcesses', { size: size }) + }) }) From c0bc44e537fd46e79c8c3d5d44883ef87cf36739 Mon Sep 17 00:00:00 2001 From: hkobew Date: Wed, 8 Jan 2025 11:20:02 -0500 Subject: [PATCH 09/44] implement telemetry for exceeding thresholds --- .../src/shared/telemetry/vscodeTelemetry.json | 16 ++++++++++++ .../core/src/shared/utilities/processUtils.ts | 26 ++++++++++++++----- .../shared/utilities/processUtils.test.ts | 9 ++++--- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/packages/core/src/shared/telemetry/vscodeTelemetry.json b/packages/core/src/shared/telemetry/vscodeTelemetry.json index 5e32b249d4f..c5990d0a2ee 100644 --- a/packages/core/src/shared/telemetry/vscodeTelemetry.json +++ b/packages/core/src/shared/telemetry/vscodeTelemetry.json @@ -1,5 +1,11 @@ { "types": [ + { + "name": "systemResource", + "type": "string", + "allowedValues": ["cpu", "memory"], + "description": "The type of system resource being measured" + }, { "name": "amazonGenerateApproachLatency", "type": "double", @@ -372,6 +378,16 @@ } ] }, + { + "name": "ide_childProcessWarning", + "description": "Child Process warning due to high system usage", + "metadata": [ + { + "type": "systemResource", + "required": true + } + ] + }, { "name": "vscode_executeCommand", "description": "Emitted whenever a registered Toolkit command is executed", diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index de6d5875c79..2d5714bbe33 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -9,6 +9,7 @@ import * as logger from '../logger' import { Timeout, CancellationError, waitUntil } from './timeoutUtils' import { PollingSet } from './pollingSet' import { getLogger } from '../logger/logger' +import { telemetry } from '../telemetry' export interface RunParameterContext { /** Reports an error parsed from the stdin/stdout streams. */ @@ -100,18 +101,29 @@ export class ChildProcessTracker { } private async checkProcessUsage(pid: number): Promise { + const doesExceedThreshold = (resource: keyof ProcessStats, value: number) => { + const threshold = ChildProcessTracker.thresholds[resource] + return value > threshold + } + const warn = (resource: keyof ProcessStats, value: number) => { + telemetry.ide_childProcessWarning.run((span) => { + ChildProcessTracker.logger.warn(`Process ${pid} exceeded ${resource} threshold: ${value}`) + span.record({ systemResource: resource }) + }) + } + if (!this.#pids.has(pid)) { ChildProcessTracker.logger.warn(`Missing process with id ${pid}`) return } const stats = await this.getUsage(pid) - if (stats) { - ChildProcessTracker.logger.debug(`Process ${pid} usage: %O`, stats) - if (stats.memory > ChildProcessTracker.thresholds.memory) { - ChildProcessTracker.logger.warn(`Process ${pid} exceeded memory threshold: ${stats.memory}`) - } - if (stats.cpu > ChildProcessTracker.thresholds.cpu) { - ChildProcessTracker.logger.warn(`Process ${pid} exceeded cpu threshold: ${stats.cpu}`) + if (!stats) { + ChildProcessTracker.logger.warn(`Failed to get process stats for ${pid}`) + return + } + for (const resource of Object.keys(ChildProcessTracker.thresholds) as (keyof ProcessStats)[]) { + if (doesExceedThreshold(resource, stats[resource])) { + warn(resource as keyof ProcessStats, stats[resource]) } } } diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index 3a12ae01df5..de513914775 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -19,7 +19,7 @@ import { sleep } from '../../../shared/utilities/timeoutUtils' import { Timeout, waitUntil } from '../../../shared/utilities/timeoutUtils' import { fs } from '../../../shared' import * as FakeTimers from '@sinonjs/fake-timers' -import { installFakeClock } from '../../testUtil' +import { assertTelemetry, installFakeClock } from '../../testUtil' import { isWin } from '../../../shared/vscode/env' import { assertLogsContain } from '../../globalSetup.test' @@ -445,7 +445,7 @@ describe('ChildProcessTracker', function () { assert.strictEqual(tracker.size, 0, 'expected tracker to be empty') }) - it('logs a warning message when system usage exceeds threshold', async function () { + it('logs a warning message and emits metric when system usage exceeds threshold', async function () { const runningProcess = startSleepProcess() tracker.add(runningProcess.childProcess) @@ -462,10 +462,12 @@ describe('ChildProcessTracker', function () { await clock.tickAsync(ChildProcessTracker.pollingInterval) assertLogsContain('exceeded cpu threshold', false, 'warn') + assertTelemetry('ide_childProcessWarning', { systemResource: 'cpu' }) usageMock.resolves(highMemory) await clock.tickAsync(ChildProcessTracker.pollingInterval) assertLogsContain('exceeded memory threshold', false, 'warn') + assertTelemetry('ide_childProcessWarning', { systemResource: 'memory' }) await stopAndWait(runningProcess) }) @@ -485,7 +487,7 @@ describe('ChildProcessTracker', function () { await stopAndWait(runningProcess) }) - it('does not log for processes within threshold', async function () { + it('does not log or emit telemetry for processes within threshold', async function () { const runningProcess = startSleepProcess() usageMock.resolves({ @@ -496,6 +498,7 @@ describe('ChildProcessTracker', function () { await clock.tickAsync(ChildProcessTracker.pollingInterval) assert.throws(() => assertLogsContain(runningProcess.childProcess.pid().toString(), false, 'warn')) + assert.throws(() => assertTelemetry('ide_childProcessWarning', {})) await stopAndWait(runningProcess) }) From 1286ee2980ec64e81892280c3d69bb6d727f7bd3 Mon Sep 17 00:00:00 2001 From: hkobew Date: Wed, 8 Jan 2025 13:22:08 -0500 Subject: [PATCH 10/44] add process as str to telemetry --- .../src/shared/telemetry/vscodeTelemetry.json | 9 +++++++++ .../core/src/shared/utilities/processUtils.ts | 17 +++++++++++++---- .../shared/utilities/processUtils.test.ts | 19 +++++++++++++++---- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/packages/core/src/shared/telemetry/vscodeTelemetry.json b/packages/core/src/shared/telemetry/vscodeTelemetry.json index 5dd9b3ed0f7..d5086315b62 100644 --- a/packages/core/src/shared/telemetry/vscodeTelemetry.json +++ b/packages/core/src/shared/telemetry/vscodeTelemetry.json @@ -5,6 +5,11 @@ "type": "int", "description": "A generic size for when units are clear" }, + { + "name": "childProcess", + "type": "string", + "description": "A string representation of a ChildProcess" + }, { "name": "systemResource", "type": "string", @@ -400,6 +405,10 @@ { "type": "systemResource", "required": true + }, + { + "type": "childProcess", + "required": true } ] }, diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index a52c8880fcf..b8ccd8b4c4a 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -113,18 +113,18 @@ export class ChildProcessTracker { } const warn = (resource: keyof ProcessStats, value: number) => { telemetry.ide_childProcessWarning.run((span) => { - this.logger.warn(`Process ${pid} exceeded ${resource} threshold: ${value}`) - span.record({ systemResource: resource }) + this.logger.warn(`Process ${this.getProcessAsStr(pid)} exceeded ${resource} threshold: ${value}`) + span.record({ systemResource: resource, childProcess: this.getProcessAsStr(pid) }) }) } if (!this.#pids.has(pid)) { - this.logger.warn(`Missing process with id ${pid}`) + this.logger.warn(`Missing process with pid ${pid}`) return } const stats = await this.getUsage(pid) if (!stats) { - this.logger.warn(`Failed to get process stats for ${pid}`) + this.logger.warn(`Failed to get process stats for process with pid ${pid}`) return } for (const resource of Object.keys(ChildProcessTracker.thresholds) as (keyof ProcessStats)[]) { @@ -134,6 +134,15 @@ export class ChildProcessTracker { } } + public getProcessAsStr(pid: pid): string { + try { + return this.#processByPid.get(pid)!.toString() + } catch (e) { + this.logger.warn(`Missing process with pid ${pid}`) + return `pid: ${pid}` + } + } + public add(childProcess: ChildProcess) { const pid = childProcess.pid() this.#processByPid.set(pid, childProcess) diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index 60ed9facfde..07dc28f661a 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -472,7 +472,10 @@ describe('ChildProcessTracker', function () { await clock.tickAsync(ChildProcessTracker.pollingInterval) assertLogsContain('exceeded cpu threshold', false, 'warn') - assertTelemetry('ide_childProcessWarning', { systemResource: 'cpu' }) + assertTelemetry('ide_childProcessWarning', { + systemResource: 'cpu', + childProcess: runningProcess.childProcess.toString(), + }) await stopAndWait(runningProcess) }) @@ -488,7 +491,10 @@ describe('ChildProcessTracker', function () { usageMock.resolves(highMemory) await clock.tickAsync(ChildProcessTracker.pollingInterval) assertLogsContain('exceeded memory threshold', false, 'warn') - assertTelemetry('ide_childProcessWarning', { systemResource: 'memory' }) + assertTelemetry('ide_childProcessWarning', { + systemResource: 'memory', + childProcess: runningProcess.childProcess.toString(), + }) await stopAndWait(runningProcess) }) @@ -525,7 +531,6 @@ describe('ChildProcessTracker', function () { }) it('logAllUsage includes only active processes', async function () { - console.log('start') const runningProcess1 = startSleepProcess() const runningProcess2 = startSleepProcess() @@ -541,7 +546,6 @@ describe('ChildProcessTracker', function () { console.log('logAllUsage called') assert.throws(() => assertLogsContain(runningProcess1.childProcess.pid().toString(), false, 'info')) assertLogsContain(runningProcess2.childProcess.pid().toString(), false, 'info') - console.log('end') }) it('logAllUsage defaults to empty message when empty', async function () { @@ -564,4 +568,11 @@ describe('ChildProcessTracker', function () { await tracker.logAllUsage() assertTelemetry('ide_logActiveProcesses', { size: size }) }) + + it('getProcessAsStr logs warning when its missing', async function () { + const runningProcess1 = startSleepProcess() + tracker.clear() + tracker.getProcessAsStr(runningProcess1.childProcess.pid()) + assertLogsContain(runningProcess1.childProcess.pid().toString(), false, 'warn') + }) }) From e25df1be397f9eaae4a219986311d07109eb3656 Mon Sep 17 00:00:00 2001 From: hkobew Date: Wed, 8 Jan 2025 13:25:25 -0500 Subject: [PATCH 11/44] remove explicit adding now that tracker is singleton --- .../src/test/shared/utilities/processUtils.test.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index 07dc28f661a..d673dbc72e1 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -399,7 +399,6 @@ describe('ChildProcessTracker', function () { before(function () { clock = installFakeClock() - tracker = ChildProcessTracker.instance usageMock = sinon.stub(ChildProcessTracker.prototype, 'getUsage') }) @@ -415,7 +414,6 @@ describe('ChildProcessTracker', function () { it(`removes stopped processes every ${ChildProcessTracker.pollingInterval / 1000} seconds`, async function () { // Start a 'sleep' command, check it only removes after we stop it. const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) assert.strictEqual(tracker.has(runningProcess.childProcess), true, 'failed to add sleep command') await clock.tickAsync(ChildProcessTracker.pollingInterval) @@ -433,8 +431,6 @@ describe('ChildProcessTracker', function () { it('multiple processes from same command are tracked seperately', async function () { const runningProcess1 = startSleepProcess() const runningProcess2 = startSleepProcess() - tracker.add(runningProcess1.childProcess) - tracker.add(runningProcess2.childProcess) assert.strictEqual(tracker.has(runningProcess1.childProcess), true, 'Missing first process') assert.strictEqual(tracker.has(runningProcess2.childProcess), true, 'Missing second process') @@ -461,7 +457,6 @@ describe('ChildProcessTracker', function () { it('logs a warning message and emits metric when cpu exceeds threshold', async function () { const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) const highCpu: ProcessStats = { cpu: ChildProcessTracker.thresholds.cpu + 1, @@ -482,7 +477,6 @@ describe('ChildProcessTracker', function () { it('logs a warning message and emits metric when memory exceeds threshold', async function () { const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) const highMemory: ProcessStats = { cpu: 0, memory: ChildProcessTracker.thresholds.memory + 1, @@ -501,7 +495,6 @@ describe('ChildProcessTracker', function () { it('includes pid in logs', async function () { const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) usageMock.resolves({ cpu: ChildProcessTracker.thresholds.cpu + 1, @@ -534,9 +527,6 @@ describe('ChildProcessTracker', function () { const runningProcess1 = startSleepProcess() const runningProcess2 = startSleepProcess() - tracker.add(runningProcess1.childProcess) - tracker.add(runningProcess2.childProcess) - assert.ok(tracker.has(runningProcess1.childProcess), 'Missing first process') assert.ok(tracker.has(runningProcess2.childProcess), 'Missing seconds process') @@ -561,8 +551,7 @@ describe('ChildProcessTracker', function () { it('logsAllUsage emits telemetry to number of processes (nonempty)', async function () { const size = 10 for (const _ of Array.from({ length: size })) { - const runningProcess = startSleepProcess() - tracker.add(runningProcess.childProcess) + startSleepProcess() } await tracker.logAllUsage() From 5090323e4f87789c5b22c9b44dda18fd3eea1534 Mon Sep 17 00:00:00 2001 From: hkobew Date: Wed, 8 Jan 2025 13:31:20 -0500 Subject: [PATCH 12/44] add a safety clear before tests run --- packages/core/src/test/shared/utilities/processUtils.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index d673dbc72e1..37dd26e954b 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -387,6 +387,7 @@ describe('ChildProcessTracker', function () { before(function () { tracker = ChildProcessTracker.instance + tracker.clear() // Avoid bleed-through of other tests running child processes. }) afterEach(function () { From 7685707e2685e4834b878bfc8c7db583ce8844b0 Mon Sep 17 00:00:00 2001 From: hkobew Date: Wed, 8 Jan 2025 13:59:57 -0500 Subject: [PATCH 13/44] regroup tests --- .../shared/utilities/processUtils.test.ts | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/packages/core/src/test/shared/utilities/processUtils.test.ts b/packages/core/src/test/shared/utilities/processUtils.test.ts index 37dd26e954b..5c2b24b347b 100644 --- a/packages/core/src/test/shared/utilities/processUtils.test.ts +++ b/packages/core/src/test/shared/utilities/processUtils.test.ts @@ -524,39 +524,40 @@ describe('ChildProcessTracker', function () { }) }) - it('logAllUsage includes only active processes', async function () { - const runningProcess1 = startSleepProcess() - const runningProcess2 = startSleepProcess() + describe('logAllUsage', function () { + it('includes only active processes', async function () { + const runningProcess1 = startSleepProcess() + const runningProcess2 = startSleepProcess() - assert.ok(tracker.has(runningProcess1.childProcess), 'Missing first process') - assert.ok(tracker.has(runningProcess2.childProcess), 'Missing seconds process') + assert.ok(tracker.has(runningProcess1.childProcess), 'Missing first process') + assert.ok(tracker.has(runningProcess2.childProcess), 'Missing seconds process') - await stopAndWait(runningProcess1) + await stopAndWait(runningProcess1) - await tracker.logAllUsage() - console.log('logAllUsage called') - assert.throws(() => assertLogsContain(runningProcess1.childProcess.pid().toString(), false, 'info')) - assertLogsContain(runningProcess2.childProcess.pid().toString(), false, 'info') - }) + await tracker.logAllUsage() + assert.throws(() => assertLogsContain(runningProcess1.childProcess.pid().toString(), false, 'info')) + assertLogsContain(runningProcess2.childProcess.pid().toString(), false, 'info') + }) - it('logAllUsage defaults to empty message when empty', async function () { - await tracker.logAllUsage() - assertLogsContain('No Active Subprocesses', false, 'info') - }) + it('defaults to empty message when empty', async function () { + await tracker.logAllUsage() + assertLogsContain('No Active Subprocesses', false, 'info') + }) - it('logAllUsage emits telemetry with size equal to number of processes (empty)', async function () { - await tracker.logAllUsage() - assertTelemetry('ide_logActiveProcesses', { size: 0 }) - }) + it('emits telemetry with size equal to number of processes (empty)', async function () { + await tracker.logAllUsage() + assertTelemetry('ide_logActiveProcesses', { size: 0 }) + }) - it('logsAllUsage emits telemetry to number of processes (nonempty)', async function () { - const size = 10 - for (const _ of Array.from({ length: size })) { - startSleepProcess() - } + it('emits telemetry to number of processes (nonempty)', async function () { + const size = 10 + for (const _ of Array.from({ length: size })) { + startSleepProcess() + } - await tracker.logAllUsage() - assertTelemetry('ide_logActiveProcesses', { size: size }) + await tracker.logAllUsage() + assertTelemetry('ide_logActiveProcesses', { size: size }) + }) }) it('getProcessAsStr logs warning when its missing', async function () { From b66b01a932a2a5b5d4551105b9c09bab91bc0d3c Mon Sep 17 00:00:00 2001 From: Jacob Chung Date: Mon, 13 Jan 2025 15:40:51 -0800 Subject: [PATCH 14/44] add message after accept/reject action --- .../Bug Fix-b6d52b75-69e6-47bb-939b-5ddede03f977.json | 4 ++++ packages/core/src/amazonqTest/chat/controller/controller.ts | 5 +++++ 2 files changed, 9 insertions(+) create mode 100644 packages/amazonq/.changes/next-release/Bug Fix-b6d52b75-69e6-47bb-939b-5ddede03f977.json diff --git a/packages/amazonq/.changes/next-release/Bug Fix-b6d52b75-69e6-47bb-939b-5ddede03f977.json b/packages/amazonq/.changes/next-release/Bug Fix-b6d52b75-69e6-47bb-939b-5ddede03f977.json new file mode 100644 index 00000000000..8772b9bf10d --- /dev/null +++ b/packages/amazonq/.changes/next-release/Bug Fix-b6d52b75-69e6-47bb-939b-5ddede03f977.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "Amazon Q /test: Unit test generation completed message shows after accept/reject action" +} diff --git a/packages/core/src/amazonqTest/chat/controller/controller.ts b/packages/core/src/amazonqTest/chat/controller/controller.ts index 35e234cc01a..9e6dcfaff0b 100644 --- a/packages/core/src/amazonqTest/chat/controller/controller.ts +++ b/packages/core/src/amazonqTest/chat/controller/controller.ts @@ -717,6 +717,9 @@ export class TestController { await vscode.window.showTextDocument(document) // TODO: send the message once again once build is enabled // this.messenger.sendMessage('Accepted', message.tabID, 'prompt') + + this.messenger.sendMessage('Unit test generation completed', message.tabID, 'answer') + telemetry.ui_click.emit({ elementId: 'unitTestGeneration_acceptDiff' }) TelemetryHelper.instance.sendTestGenerationToolkitEvent( @@ -840,6 +843,8 @@ export class TestController { private async endSession(data: any, step: FollowUpTypes) { const session = this.sessionStorage.getSession() if (step === FollowUpTypes.RejectCode) { + this.messenger.sendMessage('Unit test generation completed.', data.tabID, 'answer') + TelemetryHelper.instance.sendTestGenerationToolkitEvent( session, true, From e5d4538b4d98152f5291d1cb2136abd6e4a33a03 Mon Sep 17 00:00:00 2001 From: Jacob Chung Date: Tue, 14 Jan 2025 14:33:35 -0800 Subject: [PATCH 15/44] message now only in endSession --- .../core/src/amazonqTest/chat/controller/controller.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/core/src/amazonqTest/chat/controller/controller.ts b/packages/core/src/amazonqTest/chat/controller/controller.ts index 9e6dcfaff0b..57d4d231d07 100644 --- a/packages/core/src/amazonqTest/chat/controller/controller.ts +++ b/packages/core/src/amazonqTest/chat/controller/controller.ts @@ -717,9 +717,6 @@ export class TestController { await vscode.window.showTextDocument(document) // TODO: send the message once again once build is enabled // this.messenger.sendMessage('Accepted', message.tabID, 'prompt') - - this.messenger.sendMessage('Unit test generation completed', message.tabID, 'answer') - telemetry.ui_click.emit({ elementId: 'unitTestGeneration_acceptDiff' }) TelemetryHelper.instance.sendTestGenerationToolkitEvent( @@ -841,10 +838,10 @@ export class TestController { // TODO: Check if there are more cases to endSession if yes create a enum or type for step private async endSession(data: any, step: FollowUpTypes) { + this.messenger.sendMessage('Unit test generation completed.', data.tabID, 'answer') + const session = this.sessionStorage.getSession() if (step === FollowUpTypes.RejectCode) { - this.messenger.sendMessage('Unit test generation completed.', data.tabID, 'answer') - TelemetryHelper.instance.sendTestGenerationToolkitEvent( session, true, From 2e424b3c7cdc71334d0651d26486690f1b66b2f0 Mon Sep 17 00:00:00 2001 From: Josh Pinkney <103940141+jpinkney-aws@users.noreply.github.com> Date: Wed, 15 Jan 2025 13:37:51 -0500 Subject: [PATCH 16/44] test(amazonq): Add e2e tests for general amazon q chat panel (#6279) ## Problem - We want tests for the general amazon q panel - We want other teams to quickly be able to write tests for their agents ## Solution - Add general tests for the amazon q chat panel - Add a template that other teams can use to write their tests - Fix an issue that occurred only in tests where tab id's weren't defined for the help message. The problem was that in the framework everything is instant which meant a tab id was defined before that code ran and was never passed through to processQuickActionCommand - By default don't show the welcome page for now tests --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --- packages/amazonq/test/e2e/amazonq/assert.ts | 11 +++ .../amazonq/test/e2e/amazonq/chat.test.ts | 85 +++++++++++++++++++ .../test/e2e/amazonq/framework/framework.ts | 2 +- .../amazonq/test/e2e/amazonq/template.test.ts | 67 +++++++++++++++ .../amazonq/test/e2e/amazonq/welcome.test.ts | 10 +-- packages/core/src/amazonq/index.ts | 1 + .../controllers/chat/controller.ts | 8 +- 7 files changed, 172 insertions(+), 12 deletions(-) create mode 100644 packages/amazonq/test/e2e/amazonq/chat.test.ts create mode 100644 packages/amazonq/test/e2e/amazonq/template.test.ts diff --git a/packages/amazonq/test/e2e/amazonq/assert.ts b/packages/amazonq/test/e2e/amazonq/assert.ts index 7bc7bb2c22e..5bcec3fc0b4 100644 --- a/packages/amazonq/test/e2e/amazonq/assert.ts +++ b/packages/amazonq/test/e2e/amazonq/assert.ts @@ -28,3 +28,14 @@ export function assertQuickActions(tab: Messenger, commands: string[]) { assert.fail(`Could not find commands: ${missingCommands.join(', ')} for ${tab.tabID}`) } } + +export function assertContextCommands(tab: Messenger, contextCommands: string[]) { + assert.deepStrictEqual( + tab + .getStore() + .contextCommands?.map((x) => x.commands) + .flat() + .map((x) => x.command), + contextCommands + ) +} diff --git a/packages/amazonq/test/e2e/amazonq/chat.test.ts b/packages/amazonq/test/e2e/amazonq/chat.test.ts new file mode 100644 index 00000000000..3021be28782 --- /dev/null +++ b/packages/amazonq/test/e2e/amazonq/chat.test.ts @@ -0,0 +1,85 @@ +/*! + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +import assert from 'assert' +import { qTestingFramework } from './framework/framework' +import sinon from 'sinon' +import { Messenger } from './framework/messenger' +import { MynahUIDataModel } from '@aws/mynah-ui' +import { assertContextCommands, assertQuickActions } from './assert' +import { registerAuthHook, using } from 'aws-core-vscode/test' +import { loginToIdC } from './utils/setup' +import { webviewConstants } from 'aws-core-vscode/amazonq' + +describe('Amazon Q Chat', function () { + let framework: qTestingFramework + let tab: Messenger + let store: MynahUIDataModel + + const availableCommands: string[] = ['/dev', '/test', '/review', '/doc', '/transform'] + + before(async function () { + /** + * Login to the amazonq-test-account. When running in CI this has unlimited + * calls to the backend api + */ + await using(registerAuthHook('amazonq-test-account'), async () => { + await loginToIdC() + }) + }) + + // jscpd:ignore-start + beforeEach(() => { + // Make sure you're logged in before every test + registerAuthHook('amazonq-test-account') + framework = new qTestingFramework('cwc', true, []) + tab = framework.createTab() + store = tab.getStore() + }) + + afterEach(() => { + framework.removeTab(tab.tabID) + framework.dispose() + sinon.restore() + }) + + it(`Shows quick actions: ${availableCommands.join(', ')}`, async () => { + assertQuickActions(tab, availableCommands) + }) + + it('Shows @workspace', () => { + assertContextCommands(tab, ['@workspace']) + }) + + // jscpd:ignore-end + + it('Shows title', () => { + assert.deepStrictEqual(store.tabTitle, 'Chat') + }) + + it('Shows placeholder', () => { + assert.deepStrictEqual(store.promptInputPlaceholder, 'Ask a question or enter "/" for quick actions') + }) + + it('Sends message', async () => { + tab.addChatMessage({ + prompt: 'What is a lambda', + }) + await tab.waitForChatFinishesLoading() + const chatItems = tab.getChatItems() + // the last item should be an answer + assert.deepStrictEqual(chatItems[4].type, 'answer') + }) + + describe('Clicks examples', () => { + it('Click help', async () => { + tab.clickButton('help') + await tab.waitForText(webviewConstants.helpMessage) + const chatItems = tab.getChatItems() + assert.deepStrictEqual(chatItems[4].type, 'answer') + assert.deepStrictEqual(chatItems[4].body, webviewConstants.helpMessage) + }) + }) +}) diff --git a/packages/amazonq/test/e2e/amazonq/framework/framework.ts b/packages/amazonq/test/e2e/amazonq/framework/framework.ts index b39dbe4314b..6a29015c06f 100644 --- a/packages/amazonq/test/e2e/amazonq/framework/framework.ts +++ b/packages/amazonq/test/e2e/amazonq/framework/framework.ts @@ -29,7 +29,7 @@ export class qTestingFramework { featureName: TabType, amazonQEnabled: boolean, featureConfigsSerialized: [string, FeatureContext][], - welcomeCount = 0 + welcomeCount = Number.MAX_VALUE // by default don't show the welcome page ) { /** * Instantiate the UI and override the postMessage to publish using the app message diff --git a/packages/amazonq/test/e2e/amazonq/template.test.ts b/packages/amazonq/test/e2e/amazonq/template.test.ts new file mode 100644 index 00000000000..42857575583 --- /dev/null +++ b/packages/amazonq/test/e2e/amazonq/template.test.ts @@ -0,0 +1,67 @@ +/*! + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +// jscpd:ignore-start +import assert from 'assert' +import { qTestingFramework } from './framework/framework' +import sinon from 'sinon' +import { Messenger } from './framework/messenger' +import { MynahUIDataModel } from '@aws/mynah-ui' +import { assertQuickActions } from './assert' +import { registerAuthHook, using } from 'aws-core-vscode/test' +import { loginToIdC } from './utils/setup' + +describe.skip('Amazon Q Test Template', function () { + let framework: qTestingFramework + let tab: Messenger + let store: MynahUIDataModel + + const availableCommands: string[] = [] + + before(async function () { + /** + * Login to the amazonq-test-account. When running in CI this has unlimited + * calls to the backend api + */ + await using(registerAuthHook('amazonq-test-account'), async () => { + await loginToIdC() + }) + }) + + beforeEach(() => { + // Make sure you're logged in before every test + registerAuthHook('amazonq-test-account') + + // TODO change unknown to the tab type you want to test + framework = new qTestingFramework('unknown', true, []) + tab = framework.getTabs()[0] // use the default tab that gets created + framework.createTab() // alternatively you can create a new tab + store = tab.getStore() + }) + + afterEach(() => { + framework.removeTab(tab.tabID) + framework.dispose() + sinon.restore() + }) + + it(`Shows quick actions: ${availableCommands.join(', ')}`, async () => { + assertQuickActions(tab, availableCommands) + }) + + it('Shows title', () => { + assert.deepStrictEqual(store.tabTitle, '') + }) + + it('Shows placeholder', () => { + assert.deepStrictEqual(store.promptInputPlaceholder, '') + }) + + describe('clicks examples', () => {}) + + describe('sends message', async () => {}) +}) + +// jscpd:ignore-end diff --git a/packages/amazonq/test/e2e/amazonq/welcome.test.ts b/packages/amazonq/test/e2e/amazonq/welcome.test.ts index 3f9929cf062..d9f0ccd66bf 100644 --- a/packages/amazonq/test/e2e/amazonq/welcome.test.ts +++ b/packages/amazonq/test/e2e/amazonq/welcome.test.ts @@ -8,8 +8,8 @@ import { qTestingFramework } from './framework/framework' import sinon from 'sinon' import { Messenger } from './framework/messenger' import { MynahUIDataModel } from '@aws/mynah-ui' -import { assertQuickActions } from './assert' import { FeatureContext } from 'aws-core-vscode/shared' +import { assertContextCommands, assertQuickActions } from './assert' describe('Amazon Q Welcome page', function () { let framework: qTestingFramework @@ -42,13 +42,7 @@ describe('Amazon Q Welcome page', function () { }) it('Shows context commands', async () => { - assert.deepStrictEqual( - store.contextCommands - ?.map((x) => x.commands) - .flat() - .map((x) => x.command), - ['@workspace', '@highlight'] - ) + assertContextCommands(tab, ['@workspace', '@highlight']) }) describe('shows 3 times', async () => { diff --git a/packages/core/src/amazonq/index.ts b/packages/core/src/amazonq/index.ts index cd4ec424365..9ca9af7687c 100644 --- a/packages/core/src/amazonq/index.ts +++ b/packages/core/src/amazonq/index.ts @@ -25,6 +25,7 @@ export { init as gumbyChatAppInit } from '../amazonqGumby/app' export { init as testChatAppInit } from '../amazonqTest/app' export { init as docChatAppInit } from '../amazonqDoc/app' export { amazonQHelpUrl } from '../shared/constants' +export * as webviewConstants from './webview/ui/texts/constants' export { listCodeWhispererCommandsWalkthrough } from '../codewhisperer/ui/statusBarMenu' export { focusAmazonQPanel, focusAmazonQPanelKeybinding } from '../codewhispererChat/commands/registerCommands' export { TryChatCodeLensProvider, tryChatCodeLensCommand } from '../codewhispererChat/editor/codelens' diff --git a/packages/core/src/codewhispererChat/controllers/chat/controller.ts b/packages/core/src/codewhispererChat/controllers/chat/controller.ts index 57b45d414c1..a5205be78ca 100644 --- a/packages/core/src/codewhispererChat/controllers/chat/controller.ts +++ b/packages/core/src/codewhispererChat/controllers/chat/controller.ts @@ -231,17 +231,19 @@ export class ChatController { this.openLinkInExternalBrowser(click) } - private processQuickActionCommand(quickActionCommand: ChatPromptCommandType) { + private processQuickActionCommand(message: PromptMessage) { this.editorContextExtractor .extractContextForTrigger('QuickAction') .then((context) => { const triggerID = randomUUID() + const quickActionCommand = message.command as ChatPromptCommandType + this.messenger.sendQuickActionMessage(quickActionCommand, triggerID) this.triggerEventsStorage.addTriggerEvent({ id: triggerID, - tabID: undefined, + tabID: message.tabID, message: undefined, type: 'quick_action', quickAction: quickActionCommand, @@ -484,7 +486,7 @@ export class ChatController { recordTelemetryChatRunCommand('clear') return default: - this.processQuickActionCommand(message.command) + this.processQuickActionCommand(message) } } From d862a212bc6f7d94324e52feb01ff1433e06b140 Mon Sep 17 00:00:00 2001 From: Nikolas Komonen <118216176+nkomonen-amazon@users.noreply.github.com> Date: Wed, 15 Jan 2025 13:56:16 -0500 Subject: [PATCH 17/44] fix(sso): login with custom startUrl not allowed (#6368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem: A user reported that a non-standard start url is technically valid. This is because it can redirect to the underlying valid start url that matches the pattern: https://xxxxxxxx.awsapps.com/start ## Solution: Allow any URL, but warn users if they are using a non-standard one. We will show a yellow warning message in this case. The red error message is still shown when the input does not match a URL in general. ## Examples ### Invalid URL Screenshot 2025-01-14 at 4 33 58 PM ### Possibly valid since it may redirect to a valid URL Screenshot 2025-01-14 at 4 34 13 PM ### Missing the trailing `/start` Screenshot 2025-01-14 at 4 34 29 PM ### URL that also matches expected pattern Screenshot 2025-01-14 at 4 34 35 PM --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: nkomonen-amazon --- ...-71c6bbc1-67ae-4318-a7f0-c594e097ebc4.json | 4 ++ packages/core/src/auth/sso/constants.ts | 11 +++- .../core/src/login/webview/vue/backend.ts | 5 ++ packages/core/src/login/webview/vue/login.vue | 56 +++++++++++++++---- .../core/src/shared/utilities/uriUtils.ts | 12 ++++ .../test/shared/utilities/uriUtils.test.ts | 14 ++++- ...-29e6ef4c-536b-47bb-ae27-26b802ccdb65.json | 4 ++ 7 files changed, 93 insertions(+), 13 deletions(-) create mode 100644 packages/amazonq/.changes/next-release/Bug Fix-71c6bbc1-67ae-4318-a7f0-c594e097ebc4.json create mode 100644 packages/toolkit/.changes/next-release/Bug Fix-29e6ef4c-536b-47bb-ae27-26b802ccdb65.json diff --git a/packages/amazonq/.changes/next-release/Bug Fix-71c6bbc1-67ae-4318-a7f0-c594e097ebc4.json b/packages/amazonq/.changes/next-release/Bug Fix-71c6bbc1-67ae-4318-a7f0-c594e097ebc4.json new file mode 100644 index 00000000000..e0c15b7f2dc --- /dev/null +++ b/packages/amazonq/.changes/next-release/Bug Fix-71c6bbc1-67ae-4318-a7f0-c594e097ebc4.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "Auth: Valid StartURL not accepted at login" +} diff --git a/packages/core/src/auth/sso/constants.ts b/packages/core/src/auth/sso/constants.ts index 4b0e781ceaa..0e6bb082d7e 100644 --- a/packages/core/src/auth/sso/constants.ts +++ b/packages/core/src/auth/sso/constants.ts @@ -11,8 +11,15 @@ export const builderIdStartUrl = 'https://view.awsapps.com/start' export const internalStartUrl = 'https://amzn.awsapps.com/start' +/** + * Doc: https://docs.aws.amazon.com/singlesignon/latest/userguide/howtochangeURL.html + */ export const ssoUrlFormatRegex = /^(https?:\/\/(.+)\.awsapps\.com\/start|https?:\/\/identitycenter\.amazonaws\.com\/ssoins-[\da-zA-Z]{16})\/?$/ -export const ssoUrlFormatMessage = - 'URLs must start with http:// or https://. Example: https://d-xxxxxxxxxx.awsapps.com/start' +/** + * It is possible for a start url to be a completely custom url that redirects to something that matches the format + * below, so this message is only a warning. + */ +export const ssoUrlFormatMessage = 'URL possibly invalid. Expected format: https://xxxxxxxxxx.awsapps.com/start' +export const urlInvalidFormatMessage = 'URL format invalid. Expected format: https://xxxxxxxxxx.com/yyyy' diff --git a/packages/core/src/login/webview/vue/backend.ts b/packages/core/src/login/webview/vue/backend.ts index 0c1cbdaebc7..ed467175334 100644 --- a/packages/core/src/login/webview/vue/backend.ts +++ b/packages/core/src/login/webview/vue/backend.ts @@ -31,6 +31,7 @@ import { AuthEnabledFeatures, AuthError, AuthFlowState, AuthUiClick, userCancell import { DevSettings } from '../../../shared/settings' import { AuthSSOServer } from '../../../auth/sso/server' import { getLogger } from '../../../shared/logger/logger' +import { isValidUrl } from '../../../shared/utilities/uriUtils' export abstract class CommonAuthWebview extends VueWebview { private readonly className = 'CommonAuthWebview' @@ -276,4 +277,8 @@ export abstract class CommonAuthWebview extends VueWebview { cancelAuthFlow() { AuthSSOServer.lastInstance?.cancelCurrentFlow() } + + validateUrl(url: string) { + return isValidUrl(url) + } } diff --git a/packages/core/src/login/webview/vue/login.vue b/packages/core/src/login/webview/vue/login.vue index f15848a9069..4c9f65a2f6a 100644 --- a/packages/core/src/login/webview/vue/login.vue +++ b/packages/core/src/login/webview/vue/login.vue @@ -193,6 +193,7 @@ @keydown.enter="handleContinueClick()" />

{{ startUrlError }}

+

{{ startUrlWarning }}

Region
AWS Region that hosts identity directory