refactor(wrangler): move telemetry events from yargs middleware to command handlers

packages/wrangler/src/__tests__/metrics.test.ts

@@ -18,7 +18,10 @@ import {
 	readMetricsConfig,
 	writeMetricsConfig,
 } from "../metrics/metrics-config";
-import { getMetricsDispatcher } from "../metrics/metrics-dispatcher";
+import {
+	getMetricsDispatcher,
+	waitForAllMetricsDispatches,
+} from "../metrics/metrics-dispatcher";
 import { sniffUserAgent } from "../package-manager";
 import { mockConsoleMethods } from "./helpers/mock-console";
 import { useMockIsTTY } from "./helpers/mock-istty";
@@ -79,7 +82,8 @@ describe("metrics", () => {
 			});
 		});
 
-		afterEach(() => {
+		afterEach(async () => {
+			await waitForAllMetricsDispatches();
 			vi.useRealTimers();
 		});
 
@@ -101,7 +105,7 @@ describe("metrics", () => {
 					sendMetrics: true,
 				});
 				dispatcher.sendAdhocEvent("some-event", { a: 1, b: 2 });
-				await Promise.all(dispatcher.requests);
+				await waitForAllMetricsDispatches();
 				expect(requests.count).toBe(1);
 				expect(std.debug).toMatchInlineSnapshot(
 					`"Metrics dispatcher: Posting data {\\"deviceId\\":\\"f82b1f46-eb7b-4154-aa9f-ce95f23b2288\\",\\"event\\":\\"some-event\\",\\"timestamp\\":1733961600000,\\"properties\\":{\\"category\\":\\"Workers\\",\\"wranglerVersion\\":\\"1.2.3\\",\\"wranglerMajorVersion\\":1,\\"wranglerMinorVersion\\":2,\\"wranglerPatchVersion\\":3,\\"os\\":\\"foo:bar\\",\\"agent\\":null,\\"a\\":1,\\"b\\":2}}"`
@@ -118,7 +122,7 @@ describe("metrics", () => {
 					sendMetrics: true,
 				});
 				dispatcher.sendAdhocEvent("version-test");
-				await Promise.all(dispatcher.requests);
+				await waitForAllMetricsDispatches();
 				expect(requests.count).toBe(1);
 				expect(std.debug).toContain('"wranglerVersion":"1.2.3"');
 				expect(std.debug).toContain('"wranglerMajorVersion":1');
@@ -152,7 +156,7 @@ describe("metrics", () => {
 					sendMetrics: true,
 				});
 				dispatcher.sendAdhocEvent("some-event", { a: 1, b: 2 });
-				await Promise.all(dispatcher.requests);
+				await waitForAllMetricsDispatches();
 
 				expect(std.debug).toMatchInlineSnapshot(`
 					"Metrics dispatcher: Posting data {\\"deviceId\\":\\"f82b1f46-eb7b-4154-aa9f-ce95f23b2288\\",\\"event\\":\\"some-event\\",\\"timestamp\\":1733961600000,\\"properties\\":{\\"category\\":\\"Workers\\",\\"wranglerVersion\\":\\"1.2.3\\",\\"wranglerMajorVersion\\":1,\\"wranglerMinorVersion\\":2,\\"wranglerPatchVersion\\":3,\\"os\\":\\"foo:bar\\",\\"agent\\":null,\\"a\\":1,\\"b\\":2}}
@@ -194,7 +198,7 @@ describe("metrics", () => {
 					sendMetrics: true,
 				});
 				dispatcher.sendAdhocEvent("some-event", { a: 1 });
-				await Promise.all(dispatcher.requests);
+				await waitForAllMetricsDispatches();
 
 				expect(requests.count).toBe(1);
 				expect(std.debug).toContain('"agent":"claude-code"');
@@ -210,34 +214,13 @@ describe("metrics", () => {
 					sendMetrics: true,
 				});
 				dispatcher.sendAdhocEvent("some-event", { a: 1 });
-				await Promise.all(dispatcher.requests);
+				await waitForAllMetricsDispatches();
 
 				expect(requests.count).toBe(1);
 				expect(std.debug).toContain('"agent":null');
 			});
 		});
 
-		it("should keep track of all requests made", async () => {
-			const requests = mockMetricRequest();
-			const dispatcher = getMetricsDispatcher({
-				sendMetrics: true,
-			});
-
-			dispatcher.sendAdhocEvent("some-event", { a: 1, b: 2 });
-			expect(dispatcher.requests.length).toBe(1);
-
-			expect(requests.count).toBe(0);
-			await Promise.allSettled(dispatcher.requests);
-			expect(requests.count).toBe(1);
-
-			dispatcher.sendAdhocEvent("another-event", { c: 3, d: 4 });
-			expect(dispatcher.requests.length).toBe(2);
-
-			expect(requests.count).toBe(1);
-			await Promise.allSettled(dispatcher.requests);
-			expect(requests.count).toBe(2);
-		});
-
 		describe("sendCommandEvent()", () => {
 			const reused = {
 				wranglerVersion: "1.2.3",
@@ -336,10 +319,10 @@ describe("metrics", () => {
 				);
 				expect(std.out).toMatchInlineSnapshot(`
 					"
-					Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
-
 					 ⛅️ wrangler x.x.x
 					──────────────────
+
+					Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
 					Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
 				`);
 				expect(std.warn).toMatchInlineSnapshot(`""`);
@@ -476,10 +459,10 @@ describe("metrics", () => {
 				);
 				expect(std.out).toMatchInlineSnapshot(`
 					"
-					Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
-
 					 ⛅️ wrangler x.x.x
 					──────────────────
+
+					Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
 					Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
 				`);
 				expect(std.warn).toMatchInlineSnapshot(`""`);
@@ -581,10 +564,10 @@ describe("metrics", () => {
 					await runWrangler("docs arg");
 					expect(std.out).toMatchInlineSnapshot(`
 						"
-						Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
-
 						 ⛅️ wrangler x.x.x
 						──────────────────
+
+						Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
 						Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
 					`);
 
@@ -616,10 +599,10 @@ describe("metrics", () => {
 					await runWrangler("docs arg");
 					expect(std.out).toMatchInlineSnapshot(`
 						"
-						Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
-
 						 ⛅️ wrangler x.x.x
 						──────────────────
+
+						Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
 						Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
 					`);
 					expect(requests.count).toBe(2);

packages/wrangler/src/core/CommandHandledError.ts

@@ -0,0 +1,10 @@
+/**
+ * A wrapper Error that indicates the original error was thrown during Command handler execution.
+ *
+ * This is used to distinguish between:
+ * - Errors from within command handlers: telemetry and error reporting already sent by handler.
+ * - Yargs validation errors: telemetry and error reporting needs to be sent in the yargs middleware.
+ */
+export class CommandHandledError {
+	constructor(public readonly originalError: unknown) {}
+}

packages/wrangler/src/core/handle-errors.ts

@@ -247,6 +247,32 @@ function isNetworkFetchFailedError(e: unknown): boolean {
 	return false;
 }
 
+/**
+ * Determines the error type for telemetry purposes, or `undefined` if it cannot be determined.
+ */
+export function getErrorType(e: unknown): string | undefined {
+	if (isCloudflareAPIDNSError(e)) {
+		return "DNSError";
+	}
+	if (isPermissionError(e)) {
+		return "PermissionError";
+	}
+	if (isFileNotFoundError(e)) {
+		return "FileNotFoundError";
+	}
+	if (isCloudflareAPIConnectionTimeoutError(e)) {
+		return "ConnectionTimeout";
+	}
+	if (isAuthenticationError(e) || isContainersAuthenticationError(e)) {
+		return "AuthenticationError";
+	}
+	if (isBuildFailure(e) || isBuildFailureFromCause(e)) {
+		return "BuildFailure";
+	}
+	// Fallback to constructor name
+	return e instanceof Error ? e.constructor.name : undefined;
+}
+
 /**
  * Handles an error thrown during command execution.
  *
@@ -256,9 +282,8 @@ export async function handleError(
 	e: unknown,
 	args: ReadConfigCommandArgs,
 	subCommandParts: string[]
-) {
+): Promise<void> {
 	let mayReport = true;
-	let errorType: string | undefined;
 	let loggableException = e;
 
 	logger.log(""); // Just adds a bit of space
@@ -275,7 +300,6 @@ export async function handleError(
 	// Handle DNS resolution errors to Cloudflare API with a user-friendly message
 	if (isCloudflareAPIDNSError(e)) {
 		mayReport = false;
-		errorType = "DNSError";
 		logger.error(dedent`
 			Unable to resolve Cloudflare's API hostname (api.cloudflare.com or dash.cloudflare.com).
 
@@ -287,13 +311,12 @@ export async function handleError(
 
 			Please check your network connection and DNS settings.
 		`);
-		return errorType;
+		return;
 	}
 
 	// Handle permission errors with a user-friendly message
 	if (isPermissionError(e)) {
 		mayReport = false;
-		errorType = "PermissionError";
 
 		// Extract the error message and path, checking both the error and its cause
 		const errorMessage = e instanceof Error ? e.message : String(e);
@@ -339,13 +362,12 @@ export async function handleError(
 
 			Please check the file permissions and try again.
 		`);
-		return errorType;
+		return;
 	}
 
 	// Handle file not found errors with a user-friendly message
 	if (isFileNotFoundError(e)) {
 		mayReport = false;
-		errorType = "FileNotFoundError";
 
 		// Extract the error message and path, checking both the error and its cause
 		const errorMessage = e instanceof Error ? e.message : String(e);
@@ -390,19 +412,18 @@ export async function handleError(
 
 			Please check the file path and try again.
 		`);
-		return errorType;
+		return;
 	}
 
 	// Handle connection timeout errors to Cloudflare API with a user-friendly message
 	if (isCloudflareAPIConnectionTimeoutError(e)) {
 		mayReport = false;
-		errorType = "ConnectionTimeout";
 		logger.error(
 			"The request to Cloudflare's API timed out.\n" +
 				"This is likely due to network connectivity issues or slow network speeds.\n" +
 				"Please check your internet connection and try again."
 		);
-		return errorType;
+		return;
 	}
 
 	// Handle generic "fetch failed" / "Failed to fetch" network errors
@@ -448,16 +469,8 @@ export async function handleError(
 		}
 
 		await wrangler.parse();
-	} else if (
-		isAuthenticationError(e) ||
-		// Is this a Containers/Cloudchamber-based auth error?
-		// This is different because it uses a custom OpenAPI-based generated client
-		(e instanceof UserError &&
-			e.cause instanceof ApiError &&
-			e.cause.status === 403)
-	) {
+	} else if (isAuthenticationError(e) || isContainersAuthenticationError(e)) {
 		mayReport = false;
-		errorType = "AuthenticationError";
 		if (e.cause instanceof ApiError) {
 			logger.error(e.cause);
 		} else {
@@ -519,12 +532,9 @@ export async function handleError(
 		);
 	} else if (isBuildFailure(e)) {
 		mayReport = false;
-		errorType = "BuildFailure";
-
 		logBuildFailure(e.errors, e.warnings);
 	} else if (isBuildFailureFromCause(e)) {
 		mayReport = false;
-		errorType = "BuildFailure";
 		logBuildFailure(e.cause.errors, e.cause.warnings);
 	} else if (e instanceof Cloudflare.APIError) {
 		const error = new APIError({
@@ -576,6 +586,17 @@ export async function handleError(
 	) {
 		await captureGlobalException(loggableException);
 	}
+}
 
-	return errorType;
+/**
+ * Is this a Containers/Cloudchamber-based auth error?
+ *
+ * This is different because it uses a custom OpenAPI-based generated client
+ */
+function isContainersAuthenticationError(e: unknown): e is UserError {
+	return (
+		e instanceof UserError &&
+		e.cause instanceof ApiError &&
+		e.cause.status === 403
+	);
 }