diff --git a/packages/cloud/src/TelemetryClient.ts b/packages/cloud/src/TelemetryClient.ts index f4b05feed53..f8e8640323a 100644 --- a/packages/cloud/src/TelemetryClient.ts +++ b/packages/cloud/src/TelemetryClient.ts @@ -69,9 +69,7 @@ abstract class BaseTelemetryClient implements TelemetryClient { public abstract capture(event: TelemetryEvent): Promise - public captureException(_error: Error, _additionalProperties?: Record): void { - // No-op - exception capture is only supported by PostHog - } + public async captureException(_error: Error, _additionalProperties?: Record): Promise {} public setProvider(provider: TelemetryPropertiesProvider): void { this.providerRef = new WeakRef(provider) diff --git a/packages/telemetry/src/BaseTelemetryClient.ts b/packages/telemetry/src/BaseTelemetryClient.ts index 5f59e1643b7..ea9c1917c47 100644 --- a/packages/telemetry/src/BaseTelemetryClient.ts +++ b/packages/telemetry/src/BaseTelemetryClient.ts @@ -59,7 +59,7 @@ export abstract class BaseTelemetryClient implements TelemetryClient { public abstract capture(event: TelemetryEvent): Promise - public abstract captureException(error: Error, additionalProperties?: Record): void + public abstract captureException(error: Error, additionalProperties?: Record): Promise public setProvider(provider: TelemetryPropertiesProvider): void { this.providerRef = new WeakRef(provider) diff --git a/packages/telemetry/src/PostHogTelemetryClient.ts b/packages/telemetry/src/PostHogTelemetryClient.ts index 1b491111bda..f5eacd81915 100644 --- a/packages/telemetry/src/PostHogTelemetryClient.ts +++ b/packages/telemetry/src/PostHogTelemetryClient.ts @@ -2,8 +2,9 @@ import { PostHog } from "posthog-node" import * as vscode from "vscode" import { - TelemetryEventName, + type TelemetryProperties, type TelemetryEvent, + TelemetryEventName, getErrorStatusCode, getErrorMessage, shouldReportApiErrorToTelemetry, @@ -69,7 +70,10 @@ export class PostHogTelemetryClient extends BaseTelemetryClient { }) } - public override captureException(error: Error, additionalProperties?: Record): void { + public override async captureException( + error: Error, + additionalProperties?: Record, + ): Promise { if (!this.isTelemetryEnabled()) { if (this.debug) { console.info(`[PostHogTelemetryClient#captureException] Skipping exception: ${error.message}`) @@ -82,7 +86,7 @@ export class PostHogTelemetryClient extends BaseTelemetryClient { const errorCode = getErrorStatusCode(error) const errorMessage = getErrorMessage(error) ?? error.message - // Filter out expected errors (e.g., 429 rate limits) + // Filter out expected errors (e.g., 402 billing, 429 rate limits) if (!shouldReportApiErrorToTelemetry(errorCode, errorMessage)) { if (this.debug) { console.info( @@ -108,7 +112,21 @@ export class PostHogTelemetryClient extends BaseTelemetryClient { // Override the error message with the extracted error message. error.message = errorMessage - this.client.captureException(error, this.distinctId, mergedProperties) + const provider = this.providerRef?.deref() + let telemetryProperties: TelemetryProperties | undefined = undefined + + if (provider) { + try { + telemetryProperties = await provider.getTelemetryProperties() + } catch (_error) { + // Ignore. + } + } + + this.client.captureException(error, this.distinctId, { + ...mergedProperties, + $app_version: telemetryProperties?.appVersion, + }) } /** diff --git a/packages/telemetry/src/__tests__/PostHogTelemetryClient.test.ts b/packages/telemetry/src/__tests__/PostHogTelemetryClient.test.ts index 4e4c5179924..91175952f6c 100644 --- a/packages/telemetry/src/__tests__/PostHogTelemetryClient.test.ts +++ b/packages/telemetry/src/__tests__/PostHogTelemetryClient.test.ts @@ -367,37 +367,69 @@ describe("PostHogTelemetryClient", () => { }) }) - describe("shutdown", () => { - it("should call shutdown on the PostHog client", async () => { - const client = new PostHogTelemetryClient() - await client.shutdown() - expect(mockPostHogClient.shutdown).toHaveBeenCalled() - }) - }) - describe("captureException", () => { - it("should not capture exceptions when telemetry is disabled", () => { + it("should not capture exceptions when telemetry is disabled", async () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(false) const error = new Error("Test error") - client.captureException(error) + await client.captureException(error) expect(mockPostHogClient.captureException).not.toHaveBeenCalled() }) - it("should capture exceptions when telemetry is enabled", () => { + it("should capture exceptions with app version from provider", async () => { + const client = new PostHogTelemetryClient() + client.updateTelemetryState(true) + + const mockProvider: TelemetryPropertiesProvider = { + getTelemetryProperties: vi.fn().mockResolvedValue({ + appVersion: "1.0.0", + vscodeVersion: "1.60.0", + platform: "darwin", + editorName: "vscode", + language: "en", + mode: "code", + }), + } + + client.setProvider(mockProvider) + + const error = new Error("Test error") + await client.captureException(error, { customProp: "value" }) + + expect(mockPostHogClient.captureException).toHaveBeenCalledWith( + error, + "test-machine-id", + expect.objectContaining({ + customProp: "value", + $app_version: "1.0.0", + }), + ) + }) + + it("should capture exceptions with undefined app version when no provider is set", async () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) const error = new Error("Test error") - client.captureException(error, { provider: "TestProvider" }) + await client.captureException(error) expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { - provider: "TestProvider", + $app_version: undefined, }) }) + }) + + describe("shutdown", () => { + it("should call shutdown on the PostHog client", async () => { + const client = new PostHogTelemetryClient() + await client.shutdown() + expect(mockPostHogClient.shutdown).toHaveBeenCalled() + }) + }) + describe("captureException error filtering", () => { it("should filter out 429 rate limit errors (via status property)", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) @@ -410,6 +442,18 @@ describe("PostHogTelemetryClient", () => { expect(mockPostHogClient.captureException).not.toHaveBeenCalled() }) + it("should filter out 402 billing errors (via status property)", () => { + const client = new PostHogTelemetryClient() + client.updateTelemetryState(true) + + // Create an error with status 402 (Payment Required) + const error = Object.assign(new Error("Payment required"), { status: 402 }) + client.captureException(error) + + // Should NOT capture 402 errors + expect(mockPostHogClient.captureException).not.toHaveBeenCalled() + }) + it("should filter out errors with '429' in message", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) @@ -439,7 +483,9 @@ describe("PostHogTelemetryClient", () => { const error = new Error("Internal server error") client.captureException(error) - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", undefined) + expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { + $app_version: undefined, + }) }) it("should capture errors with non-429 status codes", () => { @@ -449,7 +495,9 @@ describe("PostHogTelemetryClient", () => { const error = Object.assign(new Error("Internal server error"), { status: 500 }) client.captureException(error) - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", undefined) + expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { + $app_version: undefined, + }) }) it("should use nested error message from OpenAI SDK error structure for filtering", () => { @@ -470,7 +518,7 @@ describe("PostHogTelemetryClient", () => { expect(mockPostHogClient.captureException).not.toHaveBeenCalled() }) - it("should modify error.message with extracted message from nested metadata.raw", () => { + it("should capture errors with nested metadata and override error.message with extracted message", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) @@ -485,12 +533,14 @@ describe("PostHogTelemetryClient", () => { client.captureException(error) - // Verify error message was modified to use metadata.raw (highest priority) + // The implementation overrides error.message with the extracted message from getErrorMessage expect(error.message).toBe("Upstream provider error: model overloaded") - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", undefined) + expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { + $app_version: undefined, + }) }) - it("should modify error.message with nested error.message when metadata.raw is not available", () => { + it("should capture errors with nested error.message and override error.message with extracted message", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) @@ -504,9 +554,11 @@ describe("PostHogTelemetryClient", () => { client.captureException(error) - // Verify error message was modified to use nested error.message + // The implementation overrides error.message with the extracted message from getErrorMessage expect(error.message).toBe("Upstream provider: connection timeout") - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", undefined) + expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { + $app_version: undefined, + }) }) it("should use primary message when no nested error structure exists", () => { @@ -522,78 +574,55 @@ describe("PostHogTelemetryClient", () => { // Verify error message remains the primary message expect(error.message).toBe("Primary error message") - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", undefined) + expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { + $app_version: undefined, + }) }) - it("should auto-extract properties from ApiProviderError", () => { + it("should capture ApiProviderError and auto-extract properties", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) const error = new ApiProviderError("Test error", "OpenRouter", "gpt-4", "createMessage", 500) client.captureException(error) + // The implementation auto-extracts properties from ApiProviderError expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { provider: "OpenRouter", modelId: "gpt-4", operation: "createMessage", errorCode: 500, + $app_version: undefined, }) }) - it("should auto-extract properties from ApiProviderError without errorCode", () => { - const client = new PostHogTelemetryClient() - client.updateTelemetryState(true) - - const error = new ApiProviderError("Test error", "OpenRouter", "gpt-4", "completePrompt") - client.captureException(error) - - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { - provider: "OpenRouter", - modelId: "gpt-4", - operation: "completePrompt", - }) - }) - - it("should merge auto-extracted properties with additionalProperties", () => { + it("should capture ApiProviderError with additionalProperties merged with auto-extracted properties", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) const error = new ApiProviderError("Test error", "OpenRouter", "gpt-4", "createMessage") client.captureException(error, { customProperty: "value" }) + // additionalProperties take precedence over auto-extracted properties expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { provider: "OpenRouter", modelId: "gpt-4", operation: "createMessage", customProperty: "value", + $app_version: undefined, }) }) - it("should allow additionalProperties to override auto-extracted properties", () => { - const client = new PostHogTelemetryClient() - client.updateTelemetryState(true) - - const error = new ApiProviderError("Test error", "OpenRouter", "gpt-4", "createMessage") - // Explicitly override the provider value - client.captureException(error, { provider: "OverriddenProvider" }) - - expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { - provider: "OverriddenProvider", // additionalProperties takes precedence - modelId: "gpt-4", - operation: "createMessage", - }) - }) - - it("should not auto-extract for non-ApiProviderError errors", () => { + it("should capture regular errors with additionalProperties", () => { const client = new PostHogTelemetryClient() client.updateTelemetryState(true) const error = new Error("Regular error") client.captureException(error, { customProperty: "value" }) - // Should only have the additionalProperties, not any auto-extracted ones expect(mockPostHogClient.captureException).toHaveBeenCalledWith(error, "test-machine-id", { customProperty: "value", + $app_version: undefined, }) }) }) diff --git a/packages/types/src/__tests__/telemetry.test.ts b/packages/types/src/__tests__/telemetry.test.ts index 57baf605e02..5cf58b04d1c 100644 --- a/packages/types/src/__tests__/telemetry.test.ts +++ b/packages/types/src/__tests__/telemetry.test.ts @@ -109,6 +109,11 @@ describe("telemetry error utilities", () => { } }) + it("should return false for 402 billing errors", () => { + expect(shouldReportApiErrorToTelemetry(402)).toBe(false) + expect(shouldReportApiErrorToTelemetry(402, "Payment required")).toBe(false) + }) + it("should return false for 429 rate limit errors", () => { expect(shouldReportApiErrorToTelemetry(429)).toBe(false) expect(shouldReportApiErrorToTelemetry(429, "Rate limit exceeded")).toBe(false) @@ -142,6 +147,16 @@ describe("telemetry error utilities", () => { }) }) + describe("EXPECTED_API_ERROR_CODES", () => { + it("should contain 402 (payment required)", () => { + expect(EXPECTED_API_ERROR_CODES.has(402)).toBe(true) + }) + + it("should contain 429 (rate limit)", () => { + expect(EXPECTED_API_ERROR_CODES.has(429)).toBe(true) + }) + }) + describe("ApiProviderError", () => { it("should create an error with correct properties", () => { const error = new ApiProviderError("Test error", "OpenRouter", "gpt-4", "createMessage", 500) diff --git a/packages/types/src/telemetry.ts b/packages/types/src/telemetry.ts index 912864a2145..84d5ae7aaf9 100644 --- a/packages/types/src/telemetry.ts +++ b/packages/types/src/telemetry.ts @@ -262,7 +262,7 @@ export interface TelemetryClient { setProvider(provider: TelemetryPropertiesProvider): void capture(options: TelemetryEvent): Promise - captureException(error: Error, additionalProperties?: Record): void + captureException(error: Error, additionalProperties?: Record): Promise updateTelemetryState(isOptedIn: boolean): void isTelemetryEnabled(): boolean shutdown(): Promise @@ -273,6 +273,7 @@ export interface TelemetryClient { * These are normal/expected errors that users can't do much about. */ export const EXPECTED_API_ERROR_CODES = new Set([ + 402, // Payment required - billing issues 429, // Rate limit - expected when hitting API limits ])