feat: instrument telemetry for dev command#1223
Open
Hweinstock wants to merge 2 commits into
Open
Conversation
Contributor
Package TarballHow to installnpm install https://github.com/aws/agentcore-cli/releases/download/pr-1223-tarball/aws-agentcore-0.13.1.tgz |
94547e7 to
fa268ad
Compare
agentcore-cli-automation
suggested changes
May 13, 2026
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Telemetry instrumentation looks good overall, but a few concerns about the SIGINT/exit handling for the long-running server modes that I think need to be resolved before merging. The invoke and exec paths look clean.
fa268ad to
b9c1ec1
Compare
b9c1ec1 to
a3eadee
Compare
a3eadee to
160dbbc
Compare
160dbbc to
5c1043c
Compare
agentcore-cli-automation
approved these changes
May 13, 2026
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
All four serious issues raised in earlier review comments appear to be addressed in the latest commit (53c5d65):
--logsSIGINT cleanup race —resolve()is no longer called from the SIGINT handler at lines 407–411; the promise now only resolves viaonExit, so SIGTERM (with the 2s SIGKILL fallback inDevServer.kill) has time to drive the child to exit beforeprocess.exit(0)runs.- Browser-mode SIGINT race — the duplicate
process.once('SIGINT', ...) { resolve() }was removed (lines 472–493). The code now just awaitsrunBrowserModeand includes an inline comment explicitly calling out the limitation that normal shutdown telemetry requires a follow-up refactor ofrunWebUI. Reasonable interim state. - TUI
process.exit(0)fromonBack— explicitcollector?.stop(); process.exit(0)was added after the wrapper at lines 463–464, giving the--no-browserpath a deterministic exit point. --logsfailure path silently exits — replaced withif (!devResult.success) throw devResult.error; process.exit(0);at lines 416–417, matching the pattern used by the other paths and surfacing the error via the outercatch.
The integ test exercises both success and failure telemetry through a real audit dir (no mocking of telemetry internals), which is good. Schema additions (exec action, UiMode, agui protocol, ui_mode field) and unit tests look correct.
LGTM.
95de36f to
a74d5b1
Compare
a74d5b1 to
56f64f6
Compare
56f64f6 to
3905aee
Compare
…metry eagerly Error classification: - ConnectionError for connection-refused (new class in lib/errors/types.ts) - ValidationError for invalid user input (missing --tool, bad JSON, unknown command) - ResourceNotFoundError for missing container runtime Browser mode telemetry: - Emit telemetry eagerly via TelemetryClientAccessor before the blocking runBrowserMode call (which never returns). Do not copy this pattern — prefer withCommandRunTelemetry for commands that return.
3905aee to
88b2a30
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Add telemetry instrumentation to the
devcommand for all execution paths (invoke, exec, server modes).Schema changes:
Actionenum:'exec'UiModeenum:'browser' | 'terminal''agui'toProtocolenumui_modefield toDevAttrsAttributes emitted: action, ui_mode, has_stream, protocol, invoke_count
Note: because the dev server runs indefinitely, we emit telemetry eagerly before the server starts. The alternative is to refactor how runWebUI works to return a result that allows us to determine if the error was a cancellation (ctrl + c) which would count as a success, or a crash, which would count as a failure. However, this is a large refactor and is therefore left out of scope.
The effect is that dev success metrics with browser correspond to "was the user able to launch the browser" rather than, "did the browser launch successfully".
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 lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsAdditional testing:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.