feat: instrument telemetry for invoke command#1227
Conversation
Package TarballHow to installnpm install https://github.com/aws/agentcore-cli/releases/download/pr-1227-tarball/aws-agentcore-0.13.1.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
LGTM 👍
Reviewed the changes against the telemetry README and the deploy command's pattern. The PR:
- Correctly wraps both CLI and TUI paths with
withCommandRunTelemetry('invoke', ...), including validation/prompt-resolution failures (these now emitexit_reason: 'failure'instead of being skipped). - Properly refactors
handleInvokeCLIto returnInvokeResultso telemetry can be recorded before output/exit, and extractsprintInvokeResultto keep the I/O concerns in the action handler. - As a side-benefit, switches JSON output to use
serializeResult, fixing the latent bug where a failure result'serrorwould have serialized as{}(Error has non-enumerable props). - Tests use real subprocess + temp dirs + audit-mode telemetry helper — no excessive mocking.
A few small things I considered and decided weren't worth blocking on:
loadInvokeConfigis now called twice on the CLI path (once up-front for the protocol attribute, once insidehandleInvokeCLI). Local file reads, fine.- TUI mode always records
exit_reason: 'success'since the callback returns{success: true}afterwaitUntilExit. Per-invoke failures inside an interactive session aren't captured, but that's consistent with onecommand_runmetric per command invocation. - A bad
-Hvalue would throw fromparseHeaderFlagsbefore the telemetry wrapper runs and skip the metric, but that matches existing behavior.
feels worth addressing.
I think this is the behavior we want. We may one day want separate telemetry for the individual invocations.
worth addressing and making it a validation exception. |
…arsing inside telemetry wrapper - Use ValidationError for validation and prompt resolution failures - Pass pre-loaded InvokeContext into handleInvokeCLI to avoid reading config files twice on the happy path - Move parseHeaderFlags inside the telemetry wrapper so invalid -H values are recorded as failures
3f8220a to
0de0e68
Compare
Description
Add telemetry instrumentation to the
invokecommand for all CLI execution paths usingwithCommandRunTelemetry.As with other PRs for instrumenting telemetry, the main changes are refactoring to the error handling to avoid killing the process before telemetry can be emitted.
Related Issue
Closes #
Documentation PR
N/A
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintAdditional testing:
--bearer-token(auth_type) and--session-id(has_session_id)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.