Uh oh!
There was an error while loading. Please reload this page.
feat: add telemetry audit mode with FileSystemSink - #1014
Conversation
agentcore-cli-automation
commented
Apr 29, 2026
Reviewed the sink implementation, public export change, and tests. The implementation matches the existing LGTM — no blocking issues from me. 🚀 |
a450075 to
ee14f54Compareee14f54 to
df58164Comparedf58164 to
4a59361Compare4a59361 to
2416e93Compare2416e93 to
3817b33Compare3817b33 to
be67dceComparebe67dce to
85582a1Compare6d61a36 to
5f4f27cCompare0a05c8f to
c24b9cfComparec24b9cf to
fa54cb7Comparefa54cb7 to
fa2076fComparefa2076f to
56a429bCompare56a429b to
9db18adCompareHweinstock
commented
Apr 29, 2026
/strands review |
Review SummaryI've reviewed the telemetry audit mode implementation. The code quality is solid and follows existing patterns well. No blocking issues found, but I have a few suggestions and questions for clarification. ✅ What's Working Well
💡 Suggestions1. TelemetryClientAccessor.init() timing (src/cli/cli.ts:226)Observation: Context: This is actually fine for the current scope since:
Suggestion: Consider documenting this limitation or adding a TODO comment for future TUI telemetry if planned. 2. FileSystemSink race condition in shutdown (src/cli/telemetry/sinks/filesystem-sink.ts:34-39)Observation: Impact: Minimal in practice since commands finish before shutdown, but the interface technically allows it. Suggestion: Consider setting asyncshutdown(): Promise<void>{consthadRecords=this.hasRecords;awaitthis.pendingWrite;if(hadRecords){this.log(`[audit mode] Telemetry written to ${this.filePath}`);}}❓ Questions1. Entrypoint derivation from args[0] (src/cli/cli.ts:226)Current behavior: TelemetryClientAccessor.init(args[0]??'unknown');This uses the command name (e.g., "help") as the entrypoint, which becomes the filename prefix: Question: For subcommands like
Is this intentional? Should it use:
2. Config file setting: telemetry.audit (src/cli/global-config.ts:21)Observation: The global config schema includes Questions:
3. File naming strategy (related to #1)Current behavior: Multiple commands in a session (e.g., running both Inside the file, they're distinguished: {"value": 42, "attrs": {"command": "help", ...}}
{"value": 55, "attrs": {"command": "help.modes", ...}}Question: Is this the intended UX for audit mode? Or should each command invocation get its own file? 🔍 Nits
Overall AssessmentStatus: ✅ Approve with suggestions The implementation is solid and production-ready. The suggestions above are for polish and clarification, not correctness issues. The questions are mainly to ensure the design choices align with the intended use case (internal audit mode for development/debugging). Great work on maintaining consistency with the existing codebase! 🚀 |
Hweinstock
commented
Apr 29, 2026
|
Uh oh!
There was an error while loading. Please reload this page.
Description
Adds telemetry audit mode: when enabled, CLI commands write telemetry data to
~/.agentcore/telemetry/as JSONL files for local inspection.MetricSinkthat appends{ value, attrs }as JSONL. Eachrecord()appends a line to the file.init()creates eagerly,get()creates lazily if needed,shutdown()proxies to the client. Called frommain()at startup/exit.AGENTCORE_TELEMETRY_AUDIT=1env var ortelemetry.audit: truein config. Theagentcore telemetry auditcommand is intentionally not introduced since we are not introducing this functionality to customers yet.helpandhelp.modeswrapped withwithCommandRun()as proof of concept. Same/similar logic will follow for rest of commands.Related Issue
Closes #
Documentation PR
N/A — hidden flag, no user-facing docs yet.
Type of Change
Testing
In addition to tests added, also tested e2e with packaged tarball.
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.