Skip to content

Commit 5d5e967

Browse files
roomoteroomote
committed
fix: address review comments for telemetry retry queue
- Add internationalization support for all notification strings - Fix telemetry_connection_check event type by adding it to TelemetryEventName enum - Align naming consistency for isAboveWarningThreshold callback parameter - Add error stack trace to TelemetryStatusMonitor initialization error logging - Add immediate queue processing when retry manager starts All requested changes have been implemented as per the review feedback.
1 parent e8a47c3 commit 5d5e967

File tree

8 files changed

+116
-59
lines changed

8 files changed

+116
-59
lines changed

‎packages/cloud/src/CloudService.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ export class CloudService {
201201
this.telemetryClient!.setConnectionStatusCallback(callback)
202202
}
203203

204-
public setTelemetryQueueSizeCallback(callback: (size: number, isAboveThreshold: boolean) => void): void {
204+
public setTelemetryQueueSizeCallback(callback: (size: number, isAboveWarningThreshold: boolean) => void): void {
205205
this.ensureInitialized()
206206
this.telemetryClient!.setQueueSizeCallback(callback)
207207
}

‎packages/cloud/src/TelemetryClient.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ export class TelemetryClient extends BaseTelemetryClient {
1616
private queue?: TelemetryQueue
1717
private retryManager?: TelemetryRetryManager
1818
private connectionStatusCallback?: (isConnected: boolean) => void
19-
private queueSizeCallback?: (size: number, isAboveThreshold: boolean) => void
19+
private queueSizeCallback?: (size: number, isAboveWarningThreshold: boolean) => void
2020

2121
constructor(
2222
private authService: AuthService,
@@ -56,9 +56,9 @@ export class TelemetryClient extends BaseTelemetryClient {
5656
this.connectionStatusCallback(isConnected)
5757
}
5858
},
59-
onQueueSizeChange: (size, isAboveThreshold) => {
59+
onQueueSizeChange: (size, isAboveWarningThreshold) => {
6060
if (this.queueSizeCallback) {
61-
this.queueSizeCallback(size, isAboveThreshold)
61+
this.queueSizeCallback(size, isAboveWarningThreshold)
6262
}
6363
},
6464
},
@@ -79,7 +79,7 @@ export class TelemetryClient extends BaseTelemetryClient {
7979
/**
8080
* Sets a callback for queue size changes
8181
*/
82-
public setQueueSizeCallback(callback: (size: number, isAboveThreshold: boolean) => void): void {
82+
public setQueueSizeCallback(callback: (size: number, isAboveWarningThreshold: boolean) => void): void {
8383
this.queueSizeCallback = callback
8484
}
8585

‎packages/cloud/src/telemetry/TelemetryRetryManager.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
1-
import { TelemetryEvent } from "@roo-code/types"
1+
import { TelemetryEvent, TelemetryEventName } from "@roo-code/types"
22
import { TelemetryQueue, QueuedTelemetryEvent } from "./TelemetryQueue"
33

44
export interface RetryManagerConfig {
55
retryIntervalMs: number
66
batchSize: number
77
onConnectionStatusChange?: (isConnected: boolean) => void
8-
onQueueSizeChange?: (size: number, isAboveThreshold: boolean) => void
8+
onQueueSizeChange?: (size: number, isAboveWarningThreshold: boolean) => void
99
}
1010

1111
const DEFAULT_CONFIG: RetryManagerConfig = {
@@ -42,6 +42,9 @@ export class TelemetryRetryManager {
4242
this.retryTimer = setInterval(() => {
4343
this.processQueue()
4444
}, this.config.retryIntervalMs)
45+
46+
// Process immediately on start
47+
this.processQueue()
4548
}
4649

4750
/**
@@ -190,7 +193,7 @@ export class TelemetryRetryManager {
190193
try {
191194
// Try to send a minimal test event
192195
await this.sendEvent({
193-
event: "telemetry_connection_check" as TelemetryEvent["event"],
196+
event: TelemetryEventName.TELEMETRY_CONNECTION_CHECK,
194197
properties: { timestamp: now },
195198
})
196199

‎packages/cloud/src/telemetry/__tests__/TelemetryRetryManager.test.ts‎

Lines changed: 77 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -59,44 +59,56 @@ describe("TelemetryRetryManager", () => {
5959
it("should start retry timer", async () => {
6060
retryManager.start()
6161

62-
// Should not process immediately
63-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(0)
62+
// Should process immediately on start
63+
await vi.waitFor(() => {
64+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
65+
})
6466

6567
// Advance timer and run pending timers
6668
await vi.advanceTimersByTimeAsync(30000)
67-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
69+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(2)
6870

6971
// Advance timer again
7072
await vi.advanceTimersByTimeAsync(30000)
71-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(2)
73+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(3)
7274
})
7375

7476
it("should not start multiple timers", async () => {
7577
retryManager.start()
7678
retryManager.start()
7779

80+
// Should only process once on start
81+
await vi.waitFor(() => {
82+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
83+
})
84+
7885
// Advance timer
7986
await vi.advanceTimersByTimeAsync(30000)
80-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
87+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(2)
8188

8289
// Advance timer again
8390
await vi.advanceTimersByTimeAsync(30000)
84-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(2)
91+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(3)
8592
})
8693

8794
it("should stop retry timer", async () => {
8895
retryManager.start()
8996

97+
// Should process immediately on start
98+
await vi.waitFor(() => {
99+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
100+
})
101+
90102
// Advance timer once
91103
await vi.advanceTimersByTimeAsync(30000)
92-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
104+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(2)
93105

94106
retryManager.stop()
95107

96108
// Advance timer again
97109
await vi.advanceTimersByTimeAsync(30000)
98-
// Should still only be called once
99-
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(1)
110+
// Should still only be called twice
111+
expect(mockQueue.getEventsForRetry).toHaveBeenCalledTimes(2)
100112
})
101113
})
102114

@@ -156,13 +168,15 @@ describe("TelemetryRetryManager", () => {
156168

157169
retryManager.start()
158170

159-
// Advance timer to trigger processing
160-
await vi.advanceTimersByTimeAsync(30000)
171+
// Wait for immediate processing to complete
172+
await vi.waitFor(() => {
173+
expect(mockQueue.getEventsForRetry).toHaveBeenCalled()
174+
})
161175

162176
// Should process in batches of 10
163177
// Filter out connection check events
164178
const actualEventCalls = sendEventMock.mock.calls.filter(
165-
(call) => (call[0].event as string) !== "telemetry_connection_check",
179+
(call) => call[0].event !== TelemetryEventName.TELEMETRY_CONNECTION_CHECK,
166180
)
167181
expect(actualEventCalls).toHaveLength(25)
168182
expect(mockQueue.updateEventAfterRetry).toHaveBeenCalledTimes(25)
@@ -184,8 +198,10 @@ describe("TelemetryRetryManager", () => {
184198

185199
retryManager.start()
186200

187-
// Advance timer to trigger processing
188-
await vi.advanceTimersByTimeAsync(30000)
201+
// Wait for immediate processing to complete
202+
await vi.waitFor(() => {
203+
expect(mockQueue.updateEventAfterRetry).toHaveBeenCalled()
204+
})
189205

190206
expect(mockQueue.updateEventAfterRetry).toHaveBeenCalledWith("event-1", true, undefined)
191207
})
@@ -206,8 +222,10 @@ describe("TelemetryRetryManager", () => {
206222

207223
retryManager.start()
208224

209-
// Advance timer to trigger processing
210-
await vi.advanceTimersByTimeAsync(30000)
225+
// Wait for immediate processing to complete
226+
await vi.waitFor(() => {
227+
expect(mockQueue.updateEventAfterRetry).toHaveBeenCalled()
228+
})
211229

212230
expect(mockQueue.updateEventAfterRetry).toHaveBeenCalledWith("event-1", false, "Network error")
213231
})
@@ -232,15 +250,33 @@ describe("TelemetryRetryManager", () => {
232250

233251
retryManager.start()
234252

235-
// Advance timer to trigger processing
236-
await vi.advanceTimersByTimeAsync(30000)
253+
// Wait for immediate processing to complete
254+
await vi.waitFor(() => {
255+
expect(connectionStatusCallback).toHaveBeenCalled()
256+
})
237257

238258
expect(connectionStatusCallback).toHaveBeenCalledWith(false)
239259

240-
// Now succeed
241-
sendEventMock.mockResolvedValueOnce(undefined)
260+
// Reset mocks for next iteration
261+
connectionStatusCallback.mockClear()
262+
mockQueue.getEventsForRetry.mockClear()
263+
mockQueue.updateEventAfterRetry.mockClear()
264+
sendEventMock.mockClear()
265+
266+
// Now succeed - mock the send to succeed this time
267+
sendEventMock.mockResolvedValue(undefined)
268+
269+
// Set up the queue to return the same event again
270+
mockQueue.getEventsForRetry.mockResolvedValue(events)
271+
242272
await vi.advanceTimersByTimeAsync(30000)
243273

274+
// Wait for the processing to complete
275+
await vi.waitFor(() => {
276+
expect(mockQueue.updateEventAfterRetry).toHaveBeenCalled()
277+
})
278+
279+
// Now check that connection status was updated
244280
expect(connectionStatusCallback).toHaveBeenCalledWith(true)
245281
})
246282

@@ -249,8 +285,10 @@ describe("TelemetryRetryManager", () => {
249285

250286
retryManager.start()
251287

252-
// Advance timer to trigger processing
253-
await vi.advanceTimersByTimeAsync(30000)
288+
// Wait for immediate processing to complete
289+
await vi.waitFor(() => {
290+
expect(mockQueue.pruneFailedEvents).toHaveBeenCalled()
291+
})
254292

255293
expect(mockQueue.pruneFailedEvents).toHaveBeenCalled()
256294
})
@@ -275,7 +313,7 @@ describe("TelemetryRetryManager", () => {
275313
it("should periodically check connection status", async () => {
276314
// Mock successful connection check
277315
sendEventMock.mockImplementation((event: TelemetryEvent) => {
278-
if ((event.event as string) === "telemetry_connection_check") {
316+
if (event.event === TelemetryEventName.TELEMETRY_CONNECTION_CHECK) {
279317
return Promise.resolve()
280318
}
281319
return Promise.reject(new Error("Other error"))
@@ -296,16 +334,22 @@ describe("TelemetryRetryManager", () => {
296334

297335
retryManager.start()
298336

299-
// First advance to trigger initial processing (30s)
300-
await vi.advanceTimersByTimeAsync(30000)
337+
// Wait for immediate processing to complete
338+
await vi.waitFor(() => {
339+
expect(mockQueue.getEventsForRetry).toHaveBeenCalled()
340+
})
301341

302-
// Then advance past connection check interval (1 minute more)
303-
await vi.advanceTimersByTimeAsync(35000)
342+
// Clear previous calls from immediate processing
343+
sendEventMock.mockClear()
344+
mockQueue.getEventsForRetry.mockClear()
345+
346+
// Advance past connection check interval (1 minute + 30s)
347+
await vi.advanceTimersByTimeAsync(90000)
304348

305349
// Should have sent a connection check event
306350
expect(sendEventMock).toHaveBeenCalledWith(
307351
expect.objectContaining({
308-
event: "telemetry_connection_check",
352+
event: TelemetryEventName.TELEMETRY_CONNECTION_CHECK,
309353
}),
310354
)
311355
})
@@ -329,12 +373,12 @@ describe("TelemetryRetryManager", () => {
329373

330374
retryManager.start()
331375

332-
// First advance to trigger initial processing (30s)
333-
await vi.advanceTimersByTimeAsync(30000)
334-
335-
// Then advance past connection check interval (1 minute more)
336-
await vi.advanceTimersByTimeAsync(35000)
376+
// Wait for immediate processing to complete
377+
await vi.waitFor(() => {
378+
expect(connectionStatusCallback).toHaveBeenCalled()
379+
})
337380

381+
// Connection should already be marked as disconnected from immediate processing
338382
expect(connectionStatusCallback).toHaveBeenCalledWith(false)
339383
})
340384
})

‎packages/types/src/telemetry.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ export enum TelemetryEventName {
6666
SHELL_INTEGRATION_ERROR = "Shell Integration Error",
6767
CONSECUTIVE_MISTAKE_ERROR = "Consecutive Mistake Error",
6868
CODE_INDEX_ERROR = "Code Index Error",
69+
TELEMETRY_CONNECTION_CHECK = "Telemetry Connection Check",
6970
}
7071

7172
/**
@@ -167,6 +168,7 @@ export const rooCodeTelemetryEventSchema = z.discriminatedUnion("type", [
167168
TelemetryEventName.TAB_SHOWN,
168169
TelemetryEventName.MODE_SETTINGS_CHANGED,
169170
TelemetryEventName.CUSTOM_MODE_CREATED,
171+
TelemetryEventName.TELEMETRY_CONNECTION_CHECK,
170172
]),
171173
properties: telemetryPropertiesSchema,
172174
}),

‎src/extension.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import {
3939
registerTerminalActions,
4040
CodeActionProvider,
4141
} from "./activate"
42-
import { initializeI18n } from "./i18n"
42+
import { initializeI18n, t } from "./i18n"
4343

4444
/**
4545
* Built using https://github.com/microsoft/vscode-webview-ui-toolkit
@@ -87,7 +87,11 @@ export async function activate(context: vscode.ExtensionContext) {
8787
telemetryStatusMonitor.initialize()
8888
context.subscriptions.push(telemetryStatusMonitor)
8989
} catch (error) {
90-
outputChannel.appendLine(`[TelemetryStatusMonitor] Failed to initialize: ${error}`)
90+
outputChannel.appendLine(
91+
t("telemetry:statusMonitor.failedToInitialize", {
92+
error: `${error}${error instanceof Error && error.stack ? "\n" + error.stack : ""}`,
93+
}),
94+
)
9195
}
9296

9397
// Initialize MDM service

‎src/i18n/locales/en/telemetry.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"connectionLost": "Telemetry connection lost. Events will be queued and retried automatically.",
3+
"connectionRestored": "Telemetry connection restored. Queued events are being sent.",
4+
"queueBuildingUp": "Telemetry queue is building up ({{count}} events). Check your connection to Roo Code Cloud.",
5+
"queueCleared": "Telemetry queue cleared. All events have been sent.",
6+
"statusMonitor": {
7+
"failedToInitialize": "[TelemetryStatusMonitor] Failed to initialize: {{error}}"
8+
}
9+
}

‎src/services/telemetry/TelemetryStatusMonitor.ts‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as vscode from "vscode"
22
import { CloudService } from "@roo-code/cloud"
3+
import { t } from "../../i18n"
34

45
/**
56
* TelemetryStatusMonitor provides visual feedback about telemetry connection status
@@ -43,8 +44,8 @@ export class TelemetryStatusMonitor {
4344
this.onConnectionStatusChange(isConnected)
4445
})
4546

46-
CloudService.instance.setTelemetryQueueSizeCallback((size, isAboveThreshold) => {
47-
this.onQueueSizeChange(size, isAboveThreshold)
47+
CloudService.instance.setTelemetryQueueSizeCallback((size, isAboveWarningThreshold) => {
48+
this.onQueueSizeChange(size, isAboveWarningThreshold)
4849
})
4950

5051
// Get initial status
@@ -64,34 +65,28 @@ export class TelemetryStatusMonitor {
6465

6566
// Show notification on status change
6667
if (wasConnected && !isConnected) {
67-
this.showNotification(
68-
"Telemetry connection lost. Events will be queued and retried automatically.",
69-
"warning",
70-
)
68+
this.showNotification(t("telemetry:connectionLost"), "warning")
7169
} else if (!wasConnected && isConnected) {
72-
this.showNotification("Telemetry connection restored. Queued events are being sent.", "info")
70+
this.showNotification(t("telemetry:connectionRestored"), "info")
7371
}
7472
}
7573

7674
/**
7775
* Handles queue size changes
7876
*/
79-
private onQueueSizeChange(size: number, isAboveThreshold: boolean): void {
77+
private onQueueSizeChange(size: number, isAboveWarningThreshold: boolean): void {
8078
this.queueSize = size
8179
const wasAboveThreshold = this.isAboveThreshold
82-
this.isAboveThreshold = isAboveThreshold
80+
this.isAboveThreshold = isAboveWarningThreshold
8381

8482
// Update status bar
8583
this.updateStatusBar()
8684

8785
// Show notification when crossing threshold
88-
if (!wasAboveThreshold && isAboveThreshold) {
89-
this.showNotification(
90-
`Telemetry queue is building up (${size} events). Check your connection to Roo Code Cloud.`,
91-
"warning",
92-
)
93-
} else if (wasAboveThreshold && !isAboveThreshold && size === 0) {
94-
this.showNotification("Telemetry queue cleared. All events have been sent.", "info")
86+
if (!wasAboveThreshold && isAboveWarningThreshold) {
87+
this.showNotification(t("telemetry:queueBuildingUp", { count: size }), "warning")
88+
} else if (wasAboveThreshold && !isAboveWarningThreshold && size === 0) {
89+
this.showNotification(t("telemetry:queueCleared"), "info")
9590
}
9691
}
9792

0 commit comments

Comments
 (0)