feat(telemetry): aggregate task completion telemetry with delta insta… - #1071
feat(telemetry): aggregate task completion telemetry with delta insta…#1071edelauna wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes add incremental task telemetry for tool usage and message counts. Tasks flush telemetry on completion, idle, and shutdown. Completion handling separates telemetry reporting from public completion events and prevents duplicate reporting during stale history replays. ChangesTask telemetry lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/core/tools/__tests__/attemptCompletionTool.spec.ts (1)
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting on
flushTelemetryInstallmentinstead of the payload this mock synthesizes.The mock forwards
mockTask.toolUsageandmockTask.messageCountsas-is, so thetoolsUsedandmessageCountarguments asserted throughout this file come from the mock, not fromTask#flushTelemetryInstallment. Delta semantics are covered inTask.spec.ts. What this file needs to verify is that the tool calls the flush and passes"attempt_completion". Assertingexpect(task.flushTelemetryInstallment).toHaveBeenCalledWith("attempt_completion")states that intent directly and stays correct if the payload shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/attemptCompletionTool.spec.ts` around lines 88 - 90, Update the attempt-completion tests to assert directly on task.flushTelemetryInstallment, verifying it is called with "attempt_completion". Remove assertions on synthesized toolsUsed and messageCount payloads from mockCaptureTaskCompleted, while preserving coverage of the tool’s completion behavior.src/core/task/Task.ts (1)
4777-4793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe idle check re-fires every 5 minutes after the first idle flush, and
lastTelemetryFlushAtnever suppresses it.
lastMessageTsonly advances on new activity.flushTelemetryInstallmentupdateslastTelemetryFlushAt, notlastMessageTs. After the first idle flush,idleForMsstays aboveIDLE_TELEMETRY_THRESHOLD_MS, so the interval callsflushTelemetryInstallment("idle")again every 5 minutes for the rest of the task's life. Each repeat no-ops on the empty-delta check, so no duplicate events are sent, but the comment at lines 4779-4783 states that this check "avoids waking up to do that check needlessly" — the condition never becomes false.
lastTelemetryFlushAtis also read only in the??fallback, so it has no effect wheneverlastMessageTsis set. Compare against the later of the two timestamps so a completed flush actually resets the window.♻️ Proposed fix
private startIdleTelemetryCheck(): void { this.idleTelemetryCheckInterval = setInterval(() => { - // lastMessageTs only moves forward on activity, so comparing it against the - // last flush tells us whether anything happened since that flush -- if the - // task has been quiet since well before the last flush, there's nothing new - // to report and flushTelemetryInstallment's own empty-check would no-op anyway, - // but skipping here avoids waking up to do that check needlessly. - const idleForMs = Date.now() - (this.lastMessageTs ?? this.lastTelemetryFlushAt) + // Measure idleness from the later of the last activity and the last flush. + // Using lastMessageTs alone would keep this condition true forever after the + // first idle flush, re-running the empty-delta check on every interval tick. + const lastEventAt = Math.max(this.lastMessageTs ?? 0, this.lastTelemetryFlushAt) + const idleForMs = Date.now() - lastEventAt if (idleForMs >= Task.IDLE_TELEMETRY_THRESHOLD_MS) { this.flushTelemetryInstallment("idle") } }, Task.IDLE_TELEMETRY_CHECK_INTERVAL_MS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/Task.ts` around lines 4777 - 4793, Update startIdleTelemetryCheck to calculate idleForMs from the later of lastMessageTs and lastTelemetryFlushAt, while preserving the fallback when neither timestamp is set. Ensure a completed flush resets the idle threshold so the interval does not repeatedly invoke flushTelemetryInstallment("idle") until new activity occurs.src/core/tools/AttemptCompletionTool.ts (1)
126-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
emitFinalTokenUsageUpdate()is already called insideflushTelemetryInstallment.
Task#flushTelemetryInstallmentcallsthis.emitFinalTokenUsageUpdate()atTask.tsline 4769 before it captures the event. The explicit call here is redundant. The same duplication exists at lines 183-184. One difference: the internal call runs only when a delta exists, so removing the external calls also removes the forced token emit for a no-delta completion attempt. Decide which layer owns the responsibility and keep one call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/AttemptCompletionTool.ts` around lines 126 - 127, Remove the redundant emitFinalTokenUsageUpdate call from the completion flow in AttemptCompletionTool, including the duplicate occurrence near the other reported location, and retain flushTelemetryInstallment("attempt_completion") as the single ownership point. Verify the resulting behavior still matches the intended no-delta token usage handling.src/core/task/__tests__/Task.spec.ts (1)
3121-3124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDispose each created
TaskinafterEachso the 5-minute interval does not outlive its test.
createTask()starts a realsetIntervalin theTaskconstructor. Only the threedisposetests clear it. The other seven tests leave a live interval that holds theTaskinstance and can invoke the sharedcaptureTaskCompletedSpywhile a later test in this file is running. That makes the suite order-dependent.♻️ Proposed fix
+ const createdTasks: Task[] = [] + afterEach(() => { + for (const task of createdTasks) { + task.dispose() + } + createdTasks.length = 0 vi.useRealTimers() captureTaskCompletedSpy.mockRestore() }) function createTask() { - return new Task({ + const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", startTask: false, }) + createdTasks.push(task) + return task }
dispose()is idempotent for the interval, so the threedisposetests remain correct.As per path instructions,
**/*.{test,spec}.{ts,tsx,js}: "Use package-local unit tests for pure logic, parsing, state transitions, ...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.spec.ts` around lines 3121 - 3124, Update the Task test cleanup in afterEach to dispose every Task instance created by createTask before restoring timers and mocks. Track created tasks within the test setup and call each task’s idempotent dispose method during teardown, preserving the existing dispose-test behavior.Source: Path instructions
src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts (1)
28-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests re-implement the Task.ts counter steps, so they cannot catch a regression in Task.ts.
simulatePopOnEmptyResponseandsimulateDeclineRetryhard-codemessageCounts.user--andmessageCounts.user++. Only the add decision comes from production code. If someone removesthis.messageCounts.user--atTask.tsline 3701, or thethis.messageCounts.user++at line 3769, these tests still pass. Consider driving the empty-response retry cycle throughTask#recursivelyMakeClineRequestswith a stubbed stream, or extract the counter mutations intomessageCounting.tsso both sides share one implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts` around lines 28 - 41, Replace the hard-coded counter mutations in simulatePopOnEmptyResponse and simulateDeclineRetry with tests that exercise the production Task#recursivelyMakeClineRequests retry flow using a stubbed stream, or move the shared counter mutations into messageCounting.ts and test that implementation directly. Ensure the tests fail if Task removes either the user decrement for an empty response or the user increment during declined retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/Task.ts`:
- Around line 3695-3701: Guard the decrement in the retry cleanup around
apiConversationHistory.pop() so messageCounts.user is reduced only when this
method previously incremented it for the removed message, preserving the
existing restoration behavior otherwise. Also update flushTelemetryInstallment
to clamp both user and assistant message-count deltas to zero or greater before
emitting telemetry.
---
Nitpick comments:
In `@src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts`:
- Around line 28-41: Replace the hard-coded counter mutations in
simulatePopOnEmptyResponse and simulateDeclineRetry with tests that exercise the
production Task#recursivelyMakeClineRequests retry flow using a stubbed stream,
or move the shared counter mutations into messageCounting.ts and test that
implementation directly. Ensure the tests fail if Task removes either the user
decrement for an empty response or the user increment during declined retry.
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 3121-3124: Update the Task test cleanup in afterEach to dispose
every Task instance created by createTask before restoring timers and mocks.
Track created tasks within the test setup and call each task’s idempotent
dispose method during teardown, preserving the existing dispose-test behavior.
In `@src/core/task/Task.ts`:
- Around line 4777-4793: Update startIdleTelemetryCheck to calculate idleForMs
from the later of lastMessageTs and lastTelemetryFlushAt, while preserving the
fallback when neither timestamp is set. Ensure a completed flush resets the idle
threshold so the interval does not repeatedly invoke
flushTelemetryInstallment("idle") until new activity occurs.
In `@src/core/tools/__tests__/attemptCompletionTool.spec.ts`:
- Around line 88-90: Update the attempt-completion tests to assert directly on
task.flushTelemetryInstallment, verifying it is called with
"attempt_completion". Remove assertions on synthesized toolsUsed and
messageCount payloads from mockCaptureTaskCompleted, while preserving coverage
of the tool’s completion behavior.
In `@src/core/tools/AttemptCompletionTool.ts`:
- Around line 126-127: Remove the redundant emitFinalTokenUsageUpdate call from
the completion flow in AttemptCompletionTool, including the duplicate occurrence
near the other reported location, and retain
flushTelemetryInstallment("attempt_completion") as the single ownership point.
Verify the resulting behavior still matches the intended no-delta token usage
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 610a96d6-9c0b-4461-b893-f1da90e47d3b
📒 Files selected for processing (9)
packages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/TelemetryService.task-completed.test.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/messageCounting.retrySymmetry.spec.tssrc/core/task/__tests__/messageCounting.spec.tssrc/core/task/messageCounting.tssrc/core/tools/AttemptCompletionTool.tssrc/core/tools/__tests__/attemptCompletionTool.spec.ts
| // Remove the last user message that we added earlier. Decrement | ||
| // messageCounts.user to match -- both retry branches below mark | ||
| // userMessageWasRemoved so the message (and its count) is restored | ||
| // exactly once when the retry succeeds, keeping the total symmetric | ||
| // regardless of how many empty-response cycles occur first. | ||
| this.apiConversationHistory.pop() | ||
| this.messageCounts.user-- |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The decrement can be unmatched, which lets messageCounts.user go negative and ship a negative delta to telemetry.
The decrement runs whenever the last entry in apiConversationHistory has role "user". That entry is not always one this method counted:
flushPendingToolResultsToHistory()appends a user message at line 1009 and never incrementsmessageCounts.user.- A resumed task starts with history loaded from disk while
messageCountsstarts at{ user: 0, assistant: 0 }.
In a delegation resume, shouldAddUserMessageToHistory returns false for empty content, so no increment occurred this turn, yet the last history message is the flushed tool-result user message. The pop then drives the counter to -1.
flushTelemetryInstallment gates emission on messageCountDelta.user > 0, but it does not clamp the value. Once any tool delta exists, the negative messageCount.user is sent in the payload, and summing installments per taskId no longer reconstructs the turn count.
Guard the decrement so it only reverses an increment this method made.
🐛 Proposed fix
const state = await this.providerRef.deref()?.getState()
if (this.apiConversationHistory.length > 0) {
const lastMessage = this.apiConversationHistory[this.apiConversationHistory.length - 1]
if (lastMessage.role === "user") {
// Remove the last user message that we added earlier. Decrement
// messageCounts.user to match -- both retry branches below mark
// userMessageWasRemoved so the message (and its count) is restored
// exactly once when the retry succeeds, keeping the total symmetric
// regardless of how many empty-response cycles occur first.
this.apiConversationHistory.pop()
- this.messageCounts.user--
+ // Only reverse a count this turn actually added. The popped message
+ // may predate this turn (resumed history, or a user message appended
+ // by flushPendingToolResultsToHistory, neither of which incremented).
+ if (shouldAddUserMessage) {
+ this.messageCounts.user--
+ }
}
}As an additional safeguard, clamp both delta components in flushTelemetryInstallment so a negative value can never leave the process.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Remove the last user message that we added earlier. Decrement | |
| // messageCounts.user to match -- both retry branches below mark | |
| // userMessageWasRemoved so the message (and its count) is restored | |
| // exactly once when the retry succeeds, keeping the total symmetric | |
| // regardless of how many empty-response cycles occur first. | |
| this.apiConversationHistory.pop() | |
| this.messageCounts.user-- | |
| // Remove the last user message that we added earlier. Decrement | |
| // messageCounts.user to match -- both retry branches below mark | |
| // userMessageWasRemoved so the message (and its count) is restored | |
| // exactly once when the retry succeeds, keeping the total symmetric | |
| // regardless of how many empty-response cycles occur first. | |
| this.apiConversationHistory.pop() | |
| // Only reverse a count this turn actually added. The popped message | |
| // may predate this turn (resumed history, or a user message appended | |
| // by flushPendingToolResultsToHistory, neither of which incremented). | |
| if (shouldAddUserMessage) { | |
| this.messageCounts.user-- | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/task/Task.ts` around lines 3695 - 3701, Guard the decrement in the
retry cleanup around apiConversationHistory.pop() so messageCounts.user is
reduced only when this method previously incremented it for the removed message,
preserving the existing restoration behavior otherwise. Also update
flushTelemetryInstallment to clamp both user and assistant message-count deltas
to zero or greater before emitting telemetry.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Related GitHub Issue
Refs: #830 (task completion telemetry aggregation — split from #835; independent of #1069 and #1070)
Description
Replaces per-turn `Conversation Message` and `Tool Used` events with a single `Task Completed` installment per model-initiated `attempt_completion` call.
Delta invariants. Each installment carries only the delta in `toolsUsed` and `messageCount` since the prior installment for that task. Summing installments for a `taskId` reconstructs full-task totals without double-counting. An unchanged task produces no empty installment.
Completion reasons. `completionReason` is constrained to three values:
Idle and shutdown flushes. A 5-minute check interval fires an idle installment after 30 minutes of quiet. `dispose()` flushes any unreported activity as a shutdown installment. Neither path mutates `task.toolUsage` or `task.messageCounts` — those stay running totals for the public `RooCodeEventName.TaskCompleted` API event and the UI.
Stale replay suppression. When a user revisits a completed subtask from history, the handler runs again on a fresh `Task` instance with a zero baseline. The `isStaleHistoryReplay` flag (set when `historyItem.status === "completed"`) prevents that replay from emitting a duplicate installment or a duplicate public `TaskCompleted` event.
Public API preserved. `RooCodeEventName.TaskCompleted` still represents genuine task completion or acceptance. The PostHog telemetry flush is independent of it and fires earlier (on every model-initiated `attempt_completion`).
Retry symmetry. `messageCounts.user` decrements when `Task.ts` pops a message on an empty assistant response, and restores exactly once on the next successful attempt via `userMessageWasRemoved`. The manual-retry branch now also sets `userMessageWasRemoved: true` (previously it did not, causing the message count to stay at zero on a user-approved retry). The decline-to-retry branch increments both `messageCounts.user` and `messageCounts.assistant` to match the messages it re-adds directly.
Scope
Included:
Excluded (per scope):
Test Procedure
Pre-Submission Checklist
Summary by CodeRabbit