-
Notifications
You must be signed in to change notification settings - Fork 45
feat: launch telemetry #1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+770
−235
Merged
feat: launch telemetry #1430
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
9aa6670
feat: instrument config command
Hweinstock a977e51
fix: simplify set path by validing the partial instead of the merged
Hweinstock 24de6c4
refactor: unify parsing logic with telemetry for common abstraction
Hweinstock 76e6890
fix: adjust tests to include quotes for strings
Hweinstock 0f33f91
docs: update display text that telemetry is now active
Hweinstock a23b8b5
fix(config): avoid overwriting config when not parsable
Hweinstock 7b861f3
feat(telemetry): wire endpoint resolution with constant default and o…
Hweinstock ef5daa1
test(config): exit non-zero on corrupt config
Hweinstock b919c75
fix(config): show friendly error with path on config read/write failures
Hweinstock 2df5774
refactor(telemetry): remove enable/disable subcommands in favor of ag…
Hweinstock e112e68
fix(config): regenerate installationId when persisted value is not a …
Hweinstock fd9606e
fix(telemetry): suppress first-run notice when telemetry is disabled
Hweinstock 073fedc
test(integ): isolate spawned CLI from host telemetry env
Hweinstock a2f92f5
fix(telemetry): use .jsonl extension for audit files
Hweinstock b11dff7
docs(telemetry): correct stale comment on OtelMetricSink try/catch
Hweinstock 01ea06c
test(config): remove redundant integ tests and fix type error
Hweinstock 16042d4
docs: strip noisy comments from telemetry instrumentation
Hweinstock 75b9a58
fix(test): update telemetry helper to search the correct files
Hweinstock f60789d
chore(telemetry): remove dead commands from schema
Hweinstock 95ccc8f
docs: remove double quote in comment
Hweinstock a59d425
fix: swap telemetry tests to existing metric
Hweinstock File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| import { spawnAndCollect } from '../src/test-utils/cli-runner.js'; | ||
| import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; | ||
| import { rm } from 'node:fs/promises'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { afterAll, describe, expect, it } from 'vitest'; | ||
|
|
||
| const testConfigDir = mkdtempSync(join(tmpdir(), 'agentcore-config-integ-')); | ||
| const cliPath = join(__dirname, '..', 'dist', 'cli', 'index.mjs'); | ||
|
|
||
| function run(args: string[]) { | ||
| return spawnAndCollect('node', [cliPath, ...args], tmpdir(), { | ||
| AGENTCORE_SKIP_INSTALL: '1', | ||
| AGENTCORE_CONFIG_DIR: testConfigDir, | ||
| }); | ||
| } | ||
|
|
||
| function readConfig() { | ||
| return JSON.parse(readFileSync(join(testConfigDir, 'config.json'), 'utf-8')); | ||
| } | ||
|
|
||
| describe('config command', () => { | ||
| afterAll(() => rm(testConfigDir, { recursive: true, force: true })); | ||
|
|
||
| it('lists config with installationId when fresh', async () => { | ||
| const result = await run(['config']); | ||
| expect(result.exitCode).toBe(0); | ||
| const parsed = JSON.parse(result.stdout); | ||
| expect(parsed.installationId).toBeDefined(); | ||
| }); | ||
|
|
||
| it('sets a string value', async () => { | ||
| const result = await run(['config', 'uvIndex', 'https://example.com']); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout).toContain('Set uvIndex = https://example.com'); | ||
| expect(readConfig().uvIndex).toBe('https://example.com'); | ||
| }); | ||
|
|
||
| it('gets a value', async () => { | ||
| const result = await run(['config', 'uvIndex']); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout.trim()).toBe('"https://example.com"'); | ||
| }); | ||
|
|
||
| it('sets a nested value with dot notation', async () => { | ||
| const result = await run(['config', 'telemetry.endpoint', 'https://metrics.example.com']); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(readConfig().telemetry.endpoint).toBe('https://metrics.example.com'); | ||
| }); | ||
|
|
||
| it('gets a nested value with dot notation', async () => { | ||
| const result = await run(['config', 'telemetry.endpoint']); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout.trim()).toBe('"https://metrics.example.com"'); | ||
| }); | ||
|
|
||
| it('gets an object value as JSON', async () => { | ||
| const result = await run(['config', 'telemetry']); | ||
| expect(result.exitCode).toBe(0); | ||
| const parsed = JSON.parse(result.stdout); | ||
| expect(parsed.endpoint).toBe('https://metrics.example.com'); | ||
| }); | ||
|
|
||
| it('sets a boolean value via JSON parsing', async () => { | ||
| const result = await run(['config', 'telemetry.enabled', 'true']); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(readConfig().telemetry.enabled).toBe(true); | ||
| }); | ||
|
|
||
| it('sets a numeric value via JSON parsing', async () => { | ||
| const result = await run(['config', 'transactionSearchIndexPercentage', '50']); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(readConfig().transactionSearchIndexPercentage).toBe(50); | ||
| }); | ||
|
|
||
| it('rejects invalid value for a typed key', async () => { | ||
| const result = await run(['config', 'telemetry.enabled', 'notabool']); | ||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stderr).toContain('Invalid value'); | ||
| }); | ||
|
|
||
| it('rejects unknown keys', async () => { | ||
| const result = await run(['config', 'foo.bar.baz', 'hello']); | ||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stderr).toContain('Invalid value'); | ||
| }); | ||
|
|
||
| it('returns error for unset key', async () => { | ||
| const result = await run(['config', 'disableTransactionSearch']); | ||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stderr).toContain('is not set'); | ||
| }); | ||
|
|
||
| it('lists all config after mutations', async () => { | ||
| const result = await run(['config']); | ||
| expect(result.exitCode).toBe(0); | ||
| const parsed = JSON.parse(result.stdout); | ||
| expect(parsed.uvIndex).toBe('https://example.com'); | ||
| expect(parsed.telemetry.endpoint).toBe('https://metrics.example.com'); | ||
| }); | ||
|
|
||
| describe('corrupt config file', () => { | ||
| const corruptDir = mkdtempSync(join(tmpdir(), 'agentcore-config-corrupt-')); | ||
| const corruptFile = join(corruptDir, 'config.json'); | ||
|
|
||
| afterAll(() => rm(corruptDir, { recursive: true, force: true })); | ||
|
|
||
| function runCorrupt(args: string[]) { | ||
| return spawnAndCollect('node', [cliPath, ...args], tmpdir(), { | ||
| AGENTCORE_SKIP_INSTALL: '1', | ||
| AGENTCORE_CONFIG_DIR: corruptDir, | ||
| }); | ||
| } | ||
|
|
||
| it('exits non-zero with a clear error when listing a corrupt config', async () => { | ||
| writeFileSync(corruptFile, '{ this is not valid json'); | ||
|
|
||
| const result = await runCorrupt(['config']); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stderr).toContain(`Error: Unable to parse config file at ${corruptFile}`); | ||
| }); | ||
|
|
||
| it('exits non-zero with a clear error when getting a key from a non-object config', async () => { | ||
| writeFileSync(corruptFile, '"a string"'); | ||
|
|
||
| const result = await runCorrupt(['config', 'telemetry.enabled']); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stderr).toContain(`Error: Unable to parse config file at ${corruptFile}`); | ||
| }); | ||
| }); | ||
|
|
||
| describe('installationId validation', () => { | ||
| it('rejects setting installationId to a non-UUID value', async () => { | ||
| const result = await run(['config', 'installationId', 'my-custom-id']); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stderr).toContain('Invalid value'); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { registerABTestCommand } from './commands/abtest'; | |
| import { registerAdd } from './commands/add'; | ||
| import { registerAddTool } from './commands/add/tool-command'; | ||
| import { registerArchive } from './commands/archive'; | ||
| import { registerConfig } from './commands/config'; | ||
| import { registerConfigBundle } from './commands/config-bundle'; | ||
| import { registerCreate } from './commands/create'; | ||
| import { registerDataset } from './commands/dataset'; | ||
|
|
@@ -112,6 +113,7 @@ export function registerCommands(program: Command) { | |
| registerUpdate(program); | ||
| registerValidate(program); | ||
| registerConfigBundle(program); | ||
| registerConfig(program); | ||
| registerDataset(program); | ||
| registerArchive(program); | ||
|
|
||
|
|
@@ -134,8 +136,10 @@ export const main = async (argv: string[]) => { | |
| // Register global cleanup handlers once at startup | ||
| setupAltScreenCleanup(); | ||
|
|
||
| // Generate installationId on first run and show telemetry notice | ||
| const { created: isFirstRun } = await getOrCreateInstallationId(); | ||
| // Generate installationId on first run and show telemetry notice. If we | ||
| // could not persist the id, suppress the notice so it doesn't fire every run. | ||
| const installationIdResult = await getOrCreateInstallationId(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. potential optimization: we read the config file in a few places, we should allow it to be cached. The challenge is that if a user edits mid TUI, we want to pick up the changes so will require some thought there. |
||
| const isFirstRun = installationIdResult.success && installationIdResult.created; | ||
|
|
||
| const program = createProgram(); | ||
|
|
||
|
|
@@ -153,7 +157,7 @@ export const main = async (argv: string[]) => { | |
| } | ||
|
|
||
| if (isFirstRun) { | ||
| printTelemetryNotice(); | ||
| await printTelemetryNotice(); | ||
| } | ||
|
|
||
| await TelemetryClientAccessor.init(args[0] ?? 'unknown'); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we import and use
asserthere because it provides type narrowing and avoids verbosity in follow-up assertions on result types.