fix(telemetry): flush terminal events immediately so error telemetry is delivered#41
fix(telemetry): flush terminal events immediately so error telemetry is delivered#41mgar wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
This diff correctly refactors telemetry flushing to trigger on terminal events (postrun, command-error, command-not-found) rather than only on postrun, ensuring error telemetry is actually delivered. The code changes are clean, well-commented, and the tests are updated to match the new behavior. No security, performance, or correctness issues found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The changes correctly extend flush behavior to terminal events (command-error, command-not-found) and add a flush-once guard per process. The logic is well-documented, tests cover the new behavior including the double-fire edge case, and the README is updated to match. No significant issues found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is well-structured and the logic is sound. The de-duplication of the command-not-found/command-error typo path is clear and intentional, the module-level commandNotFoundFired flag is reset in init to avoid cross-command leakage, and the tests cover all the meaningful new paths. One minor concern: the commandNotFoundFired flag is never reset after the de-dupe drop, meaning a second unrelated command-error in the same process (e.g., from a nested config.runCommand) would also be silently dropped — but this is an edge-case given the existing process model. No blocking issues.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The previously raised issue about commandNotFoundFired not being reset after suppressing the follow-up command-error has been addressed — the flag is now cleared in the else-if branch, and a dedicated test (only the first command-error after a not-found is suppressed) verifies the fix. The diff is clean: logic, documentation, and tests are all consistent, and init also resets the flag for new sessions.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
The batched-flush design only spawned the flush worker on the postrun hook. But postrun does not run after a command fails or is not found, so those metrics were buffered in memory and silently dropped on exit — error/not-found telemetry was never delivered. Lifecycle (verified against @oclif/core 4.11.4 and @adobe/aio-cli 11.1.0): oclif runs postrun only after command.run() returns; on a throw the error propagates first, and on a not-found oclif throws before postrun. command_error is not an oclif hook — the host CLI fires it from its top-level catch. So no postrun follows an error or not-found. Flush on any terminal event (postrun, command-error, command-not-found), merging buffered non-terminal metrics into the batch. Update the hook tests to exercise the real lifecycle (the error/not-found hook flushes on its own, no manufactured postrun) and the README "Flush architecture" section.
mgar
force-pushed
the
miguel/fix/flush-terminal-telemetry-events
branch
from
d7c55c0 to
9933eb6
Compare
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is well-structured and the logic is sound. The de-duplication of the command-not-found/command-error pair is clearly explained and correctly scoped. Tests are thorough and cover the edge cases (nested commands, flag reset). No security, performance, or significant correctness issues found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
mgar
force-pushed
the
miguel/fix/flush-terminal-telemetry-events
branch
from
9933eb6 to
3dfa389
Compare
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is well-structured and clearly documented. The de-duplication logic for the command-not-found/command-error typo path is sound, and the test coverage is thorough for the new terminal-event flushing behavior. No security, performance, or significant correctness issues found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
…telemetry A mistyped command fires command-not-found (from oclif) and then command-error (the host CLI rethrowing the same typo, exit 127), recording a typo twice. Track a per-process commandNotFoundFired flag (reset in init) and drop only the single command-error that follows a command-not-found, then clear the flag so a later, unrelated command-error still flushes. Scoped to the typo on purpose: init fires once per process while nested config.runCommand calls re-fire prerun/postrun (e.g. app:undeploy -> app:clean), so a blunt "first terminal event wins" flag would mask real command-errors and misattribute commands. Also clear the flag in trackPrerun so plugin-not-found's "Did you mean ...? Yes" (which runs the suggestion) doesn't mask its errors. Mark command-not-found as commandSuccess: false (a missing command did not succeed). Add tests for the single-typo entry, flag reset, both "Did you mean?" answers, and nested commands.
mgar
force-pushed
the
miguel/fix/flush-terminal-telemetry-events
branch
from
3dfa389 to
a7ca31f
Compare
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is a well-structured change that promotes command-error and command-not-found to terminal (flush-triggering) events, adds a deduplication guard for the oclif typo rethrow, and backs everything with thorough tests. The logic is clear, the state machine is small and easy to follow, and the README is updated to match. No security, performance, or error-handling issues were found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
2 participants
Description
command-error/command-not-foundtelemetry was buffered and silently dropped: the batched flush only ran onpostrun, which oclif doesn't run after an error or a not-found command. This restores delivery of error telemetry.Changes
postrun,command-error,command-not-found) instead of onlypostrun.command-not-foundand then the host rethrows it ascommand-error; drop that one rethrow. Scoped narrowly (and cleared onprerun) so nestedconfig.runCommandand "Did you mean…? Yes" don't get masked.command-not-foundis nowcommandSuccess: false(a missing command didn't succeed).Related Issue
Flagged in code review on PR #39 (
src/telemetry-lib.js)How Has This Been Tested?
Unit tests, 100% coverage, lint clean (one pre-existing test fails only inside an AI-agent shell — passes in CI).
Verified end-to-end against the prod proxy / New Relic: real errors and typos each deliver a single correct event; nested commands each flush (no masking).
Screenshots (if appropriate):
Types of changes
Checklist: