Skip to content

feat: wire telemetry into all remove.* commands #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Hweinstock wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: wire telemetry into all remove.* commands #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions integ-tests/add-remove-ab-test.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import {
readProjectConfig,
runCLI,
} from '../src/test-utils/index.js';
import { createTelemetryHelper } from '../src/test-utils/telemetry-helper.js';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';

const telemetry = createTelemetryHelper();

async function runSuccess(args: string[], cwd: string) {
const result = await runCLI(args, cwd);
expect(result.exitCode, `stdout: ${result.stdout}, stderr: ${result.stderr}`).toBe(0);
Expand Down Expand Up @@ -38,6 +41,7 @@ describe('integration: add and remove ab-test', () => {

afterAll(async () => {
await project.cleanup();
telemetry.destroy();
});

it('requires --name for JSON mode', async () => {
Expand Down Expand Up @@ -154,17 +158,26 @@ describe('integration: add and remove ab-test', () => {
});

it('removes ab-test', async () => {
const json = await runSuccess(['remove', 'ab-test', '--name', 'MyIntegTest', '--json'], project.projectPath);
const result = await runCLI(['remove', 'ab-test', '--name', 'MyIntegTest', '--json'], project.projectPath, {
env: telemetry.env,
});
expect(result.exitCode).toBe(0);
const json = JSON.parse(result.stdout);
expect(json.success).toBe(true);

// Verify removal from agentcore.json
const spec = await readProjectConfig(project.projectPath);
const abTest = spec.abTests?.find((t: { name: string }) => t.name === 'MyIntegTest');
expect(abTest).toBeUndefined();
telemetry.assertMetricEmitted({ command: 'remove.ab-test', exit_reason: 'success' });
});

it('remove returns error for non-existent test', async () => {
const json = await runFailure(['remove', 'ab-test', '--name', 'DoesNotExist', '--json'], project.projectPath);
const result = await runCLI(['remove', 'ab-test', '--name', 'DoesNotExist', '--json'], project.projectPath, {
env: telemetry.env,
});
expect(result.exitCode).toBe(1);
const json = JSON.parse(result.stdout);
expect(json.error).toContain('not found');
telemetry.assertMetricEmitted({ command: 'remove.ab-test', exit_reason: 'failure' });
});
});
46 changes: 29 additions & 17 deletions integ-tests/add-remove-config-bundle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ import {
runFailure,
runSuccess,
} from '../src/test-utils/index.js';
import { createTelemetryHelper } from '../src/test-utils/telemetry-helper.js';
import { writeFile } from 'node:fs/promises';
import { join } from 'node:path';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';

const telemetry = createTelemetryHelper();

describe('integration: add and remove config-bundle', () => {
let project: TestProject;

Expand All @@ -20,6 +23,7 @@ describe('integration: add and remove config-bundle', () => {

afterAll(async () => {
await project.cleanup();
telemetry.destroy();
});

// ── Add lifecycle ─────────────────────────────────────────────────────
Expand All @@ -40,7 +44,7 @@ describe('integration: add and remove config-bundle', () => {
expect(json.bundleName).toBe('InlineBundle');

const config = await readProjectConfig(project.projectPath);
const bundle = config.configBundles!.find(b => b.name === 'InlineBundle');
const bundle = config.configBundles.find(b => b.name === 'InlineBundle');
expect(bundle).toBeDefined();
expect(bundle!.type).toBe('ConfigurationBundle');
expect(bundle!.branchName).toBe('mainline');
Expand Down Expand Up @@ -68,7 +72,7 @@ describe('integration: add and remove config-bundle', () => {
expect(json.bundleName).toBe('FileBundle');

const config = await readProjectConfig(project.projectPath);
const bundle = config.configBundles!.find(b => b.name === 'FileBundle');
const bundle = config.configBundles.find(b => b.name === 'FileBundle');
expect(bundle).toBeDefined();
expect(Object.keys(bundle!.components)).toHaveLength(2);
});
Expand Down Expand Up @@ -102,7 +106,7 @@ describe('integration: add and remove config-bundle', () => {
expect(json.bundleName).toBe('FullOptsBundle');

const config = await readProjectConfig(project.projectPath);
const bundle = config.configBundles!.find(b => b.name === 'FullOptsBundle');
const bundle = config.configBundles.find(b => b.name === 'FullOptsBundle');
expect(bundle).toBeDefined();
expect(bundle!.description).toBe('A bundle with all optional fields');
expect(bundle!.branchName).toBe('feature-branch');
Expand All @@ -127,7 +131,7 @@ describe('integration: add and remove config-bundle', () => {
expect(json.bundleName).toBe('PlaceholderBundle');

const config = await readProjectConfig(project.projectPath);
const bundle = config.configBundles!.find(b => b.name === 'PlaceholderBundle');
const bundle = config.configBundles.find(b => b.name === 'PlaceholderBundle');
expect(bundle).toBeDefined();
const keys = Object.keys(bundle!.components);
expect(keys).toContain('{{runtime:AgentA}}');
Expand Down Expand Up @@ -228,37 +232,45 @@ describe('integration: add and remove config-bundle', () => {

describe('remove config-bundle', () => {
it('removes an existing config bundle', async () => {
const json = await runSuccess(
const result = await runCLI(
['remove', 'config-bundle', '--name', 'InlineBundle', '--json'],
project.projectPath
project.projectPath,
{ env: telemetry.env }
);

expect(result.exitCode).toBe(0);
const json = JSON.parse(result.stdout);
expect(json.success).toBe(true);

const config = await readProjectConfig(project.projectPath);
const bundle = config.configBundles!.find(b => b.name === 'InlineBundle');
const bundle = config.configBundles.find(b => b.name === 'InlineBundle');
expect(bundle).toBeUndefined();
telemetry.assertMetricEmitted({ command: 'remove.config-bundle', exit_reason: 'success' });
});

it('returns error for non-existent bundle', async () => {
const json = await runFailure(
const result = await runCLI(
['remove', 'config-bundle', '--name', 'DoesNotExist', '--json'],
project.projectPath
project.projectPath,
{ env: telemetry.env }
);

expect(result.exitCode).toBe(1);
const json = JSON.parse(result.stdout);
expect(json.error).toContain('not found');
telemetry.assertMetricEmitted({ command: 'remove.config-bundle', exit_reason: 'failure' });
});

it('removes all remaining config bundles one by one', async () => {
const configBefore = await readProjectConfig(project.projectPath);
const remaining = configBefore.configBundles!.map(b => b.name);
const remaining = configBefore.configBundles.map(b => b.name);

for (const name of remaining) {
await runSuccess(['remove', 'config-bundle', '--name', name, '--json'], project.projectPath);
}

const configAfter = await readProjectConfig(project.projectPath);
expect(configAfter.configBundles!).toHaveLength(0);
expect(configAfter.configBundles).toHaveLength(0);
});
});

Expand All @@ -282,21 +294,21 @@ describe('integration: add and remove config-bundle', () => {
}

const config = await readProjectConfig(project.projectPath);
expect(config.configBundles!).toHaveLength(bundleNames.length);
expect(config.configBundles).toHaveLength(bundleNames.length);

for (const name of bundleNames) {
expect(config.configBundles!.find(b => b.name === name)).toBeDefined();
expect(config.configBundles.find(b => b.name === name)).toBeDefined();
}
});

it('removing one bundle does not affect others', async () => {
await runSuccess(['remove', 'config-bundle', '--name', 'BundleBeta', '--json'], project.projectPath);

const config = await readProjectConfig(project.projectPath);
expect(config.configBundles!).toHaveLength(2);
expect(config.configBundles!.find(b => b.name === 'BundleAlpha')).toBeDefined();
expect(config.configBundles!.find(b => b.name === 'BundleGamma')).toBeDefined();
expect(config.configBundles!.find(b => b.name === 'BundleBeta')).toBeUndefined();
expect(config.configBundles).toHaveLength(2);
expect(config.configBundles.find(b => b.name === 'BundleAlpha')).toBeDefined();
expect(config.configBundles.find(b => b.name === 'BundleGamma')).toBeDefined();
expect(config.configBundles.find(b => b.name === 'BundleBeta')).toBeUndefined();
});

afterAll(async () => {
Expand Down
12 changes: 10 additions & 2 deletions integ-tests/add-remove-evaluator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { createTestProject, parseJsonOutput, readProjectConfig, runCLI } from '../src/test-utils/index.js';
import type { TestProject } from '../src/test-utils/index.js';
import { createTelemetryHelper } from '../src/test-utils/telemetry-helper.js';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';

const telemetry = createTelemetryHelper();

/** Run a CLI command and assert it succeeds, returning parsed JSON output. */
async function runSuccess(args: string[], cwd: string) {
const result = await runCLI(args, cwd);
const result = await runCLI(args, cwd, { env: telemetry.env });
expect(result.exitCode, `stdout: ${result.stdout}, stderr: ${result.stderr}`).toBe(0);
const json: unknown = parseJsonOutput(result.stdout);
expect(json).toHaveProperty('success', true);
Expand All @@ -13,7 +16,7 @@ async function runSuccess(args: string[], cwd: string) {

/** Run a CLI command and assert it fails, returning parsed JSON output. */
async function runFailure(args: string[], cwd: string) {
const result = await runCLI(args, cwd);
const result = await runCLI(args, cwd, { env: telemetry.env });
expect(result.exitCode).toBe(1);
const json: unknown = parseJsonOutput(result.stdout);
expect(json).toHaveProperty('success', false);
Expand All @@ -35,6 +38,7 @@ describe('integration: add and remove evaluators and online eval configs', () =>

afterAll(async () => {
await project.cleanup();
telemetry.destroy();
});

describe('evaluator and online eval lifecycle', () => {
Expand Down Expand Up @@ -123,25 +127,29 @@ describe('integration: add and remove evaluators and online eval configs', () =>

const config = await readProjectConfig(project.projectPath);
expect(config.onlineEvalConfigs.find(c => c.name === configName)).toBeUndefined();
telemetry.assertMetricEmitted({ command: 'remove.online-eval', exit_reason: 'success' });
});

it('removes the evaluator after online eval is gone', async () => {
await runSuccess(['remove', 'evaluator', '--name', evalName, '--json'], project.projectPath);

const config = await readProjectConfig(project.projectPath);
expect(config.evaluators.find(e => e.name === evalName)).toBeUndefined();
telemetry.assertMetricEmitted({ command: 'remove.evaluator', exit_reason: 'success' });
});
});

describe('error cases', () => {
it('fails to remove non-existent evaluator', async () => {
const json = await runFailure(['remove', 'evaluator', '--name', 'NonExistent', '--json'], project.projectPath);
expect(json.error).toContain('not found');
telemetry.assertMetricEmitted({ command: 'remove.evaluator', exit_reason: 'failure' });
});

it('fails to remove non-existent online eval config', async () => {
const json = await runFailure(['remove', 'online-eval', '--name', 'NonExistent', '--json'], project.projectPath);
expect(json.error).toContain('not found');
telemetry.assertMetricEmitted({ command: 'remove.online-eval', exit_reason: 'failure' });
});

it('rejects evaluator with missing --level', async () => {
Expand Down
32 changes: 30 additions & 2 deletions integ-tests/add-remove-gateway.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { createTestProject, runCLI } from '../src/test-utils/index.js';
import type { TestProject } from '../src/test-utils/index.js';
import { createTelemetryHelper } from '../src/test-utils/telemetry-helper.js';
import { mkdir, readFile, writeFile } from 'node:fs/promises';
import { join } from 'node:path';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';

const telemetry = createTelemetryHelper();

async function readProjectConfig(projectPath: string) {
return JSON.parse(await readFile(join(projectPath, 'agentcore/agentcore.json'), 'utf-8'));
}
Expand All @@ -19,6 +22,7 @@ describe('integration: add and remove gateway with external MCP server', () => {

afterAll(async () => {
await project.cleanup();
telemetry.destroy();
});

describe('gateway lifecycle', () => {
Expand Down Expand Up @@ -64,7 +68,9 @@ describe('integration: add and remove gateway with external MCP server', () => {
});

it('removes the gateway target', async () => {
const result = await runCLI(['remove', 'gateway-target', '--name', targetName, '--json'], project.projectPath);
const result = await runCLI(['remove', 'gateway-target', '--name', targetName, '--json'], project.projectPath, {
env: telemetry.env,
});

expect(result.exitCode, `stdout: ${result.stdout}, stderr: ${result.stderr}`).toBe(0);
const json = JSON.parse(result.stdout);
Expand All @@ -75,10 +81,13 @@ describe('integration: add and remove gateway with external MCP server', () => {
const targets = gateway?.targets ?? [];
const found = targets.find((t: { name: string }) => t.name === targetName);
expect(found, `Target "${targetName}" should be removed`).toBeFalsy();
telemetry.assertMetricEmitted({ command: 'remove.gateway-target', exit_reason: 'success' });
});

it('removes the gateway', async () => {
const result = await runCLI(['remove', 'gateway', '--name', gatewayName, '--json'], project.projectPath);
const result = await runCLI(['remove', 'gateway', '--name', gatewayName, '--json'], project.projectPath, {
env: telemetry.env,
});

expect(result.exitCode, `stdout: ${result.stdout}, stderr: ${result.stderr}`).toBe(0);
const json = JSON.parse(result.stdout);
Expand All @@ -88,6 +97,25 @@ describe('integration: add and remove gateway with external MCP server', () => {
const gateways = mcpSpec.agentCoreGateways ?? [];
const found = gateways.find((g: { name: string }) => g.name === gatewayName);
expect(found, `Gateway "${gatewayName}" should be removed`).toBeFalsy();
telemetry.assertMetricEmitted({ command: 'remove.gateway', exit_reason: 'success' });
});

it('fails to remove non-existent gateway', async () => {
const result = await runCLI(['remove', 'gateway', '--name', 'NonExistent', '--json'], project.projectPath, {
env: telemetry.env,
});
expect(result.exitCode).toBe(1);
telemetry.assertMetricEmitted({ command: 'remove.gateway', exit_reason: 'failure' });
});

it('fails to remove non-existent gateway target', async () => {
const result = await runCLI(
['remove', 'gateway-target', '--name', 'NonExistent', '--json'],
project.projectPath,
{ env: telemetry.env }
);
expect(result.exitCode).toBe(1);
telemetry.assertMetricEmitted({ command: 'remove.gateway-target', exit_reason: 'failure' });
});
});
});
Expand Down
17 changes: 14 additions & 3 deletions integ-tests/add-remove-policy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { createTestProject, readProjectConfig, runCLI } from '../src/test-utils/index.js';
import type { TestProject } from '../src/test-utils/index.js';
import { createTelemetryHelper } from '../src/test-utils/telemetry-helper.js';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';

const telemetry = createTelemetryHelper();

describe('integration: add and remove policy engines and policies', () => {
let project: TestProject;

Expand All @@ -16,6 +19,7 @@ describe('integration: add and remove policy engines and policies', () => {

afterAll(async () => {
await project.cleanup();
telemetry.destroy();
});

describe('policy engine lifecycle', () => {
Expand Down Expand Up @@ -111,7 +115,8 @@ describe('integration: add and remove policy engines and policies', () => {
it('removes a policy with --engine flag', async () => {
const result = await runCLI(
['remove', 'policy', '--name', policyName, '--engine', engineName, '--json'],
project.projectPath
project.projectPath,
{ env: telemetry.env }
);

expect(result.exitCode, `stdout: ${result.stdout}, stderr: ${result.stderr}`).toBe(0);
Expand All @@ -125,6 +130,7 @@ describe('integration: add and remove policy engines and policies', () => {
expect(engine, `Engine "${engineName}" should still exist`).toBeDefined();
const policy = engine!.policies.find(p => p.name === policyName);
expect(policy, `Policy "${policyName}" should be removed`).toBeUndefined();
telemetry.assertMetricEmitted({ command: 'remove.policy', exit_reason: 'success' });
});
});

Expand Down Expand Up @@ -272,24 +278,29 @@ describe('integration: add and remove policy engines and policies', () => {
});

it('fails to remove non-existent policy', async () => {
const result = await runCLI(['remove', 'policy', '--name', 'NonExistentPolicy', '--json'], project.projectPath);
const result = await runCLI(['remove', 'policy', '--name', 'NonExistentPolicy', '--json'], project.projectPath, {
env: telemetry.env,
});

expect(result.exitCode).toBe(1);
const json = JSON.parse(result.stdout);
expect(json.success).toBe(false);
expect(json.error).toContain('not found');
telemetry.assertMetricEmitted({ command: 'remove.policy', exit_reason: 'failure' });
});

it('fails to remove non-existent policy engine', async () => {
const result = await runCLI(
['remove', 'policy-engine', '--name', 'NonExistentEngine', '--json'],
project.projectPath
project.projectPath,
{ env: telemetry.env }
);

expect(result.exitCode).toBe(1);
const json = JSON.parse(result.stdout);
expect(json.success).toBe(false);
expect(json.error).toContain('not found');
telemetry.assertMetricEmitted({ command: 'remove.policy-engine', exit_reason: 'failure' });
});

it('requires --engine when adding a policy', async () => {
Expand Down
Loading
Loading