Skip to content

Commit 64b7a24

Browse files
petebacondarwinpetebacondarwin
committed
refactor(wrangler): move telemetry events from yargs middleware to command handlers
This refactoring moves the telemetry events (wrangler command started/completed/errored) from the yargs middleware in index.ts to the command handler in register-yargs-command.ts. Benefits: - Telemetry is now sent after config is loaded, allowing access to send_metrics and hasAssets - Telemetry is only sent for commands that use defineCommand Key changes: - Add CommandHandledError wrapper class to signal when telemetry has been sent - Add getErrorType() function to classify errors for telemetry - Send telemetry events in register-yargs-command.ts after config is loaded - Preserve fallback telemetry in index.ts for yargs validation errors - Handle nested wrangler.parse() calls by not double-wrapping CommandHandledError - Properly unwrap CommandHandledError when writing command-failed output
1 parent 96731a3 commit 64b7a24

File tree

8 files changed

+460
-130
lines changed

8 files changed

+460
-130
lines changed

‎packages/wrangler/src/__tests__/core/handle-errors.test.ts‎

Lines changed: 256 additions & 37 deletions
Large diffs are not rendered by default.

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -336,10 +336,10 @@ describe("metrics", () => {
336336
);
337337
expect(std.out).toMatchInlineSnapshot(`
338338
"
339-
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
340-
341339
⛅️ wrangler x.x.x
342340
──────────────────
341+
342+
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
343343
Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
344344
`);
345345
expect(std.warn).toMatchInlineSnapshot(`""`);
@@ -476,10 +476,10 @@ describe("metrics", () => {
476476
);
477477
expect(std.out).toMatchInlineSnapshot(`
478478
"
479-
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
480-
481479
⛅️ wrangler x.x.x
482480
──────────────────
481+
482+
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
483483
Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
484484
`);
485485
expect(std.warn).toMatchInlineSnapshot(`""`);
@@ -581,10 +581,10 @@ describe("metrics", () => {
581581
await runWrangler("docs arg");
582582
expect(std.out).toMatchInlineSnapshot(`
583583
"
584-
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
585-
586584
⛅️ wrangler x.x.x
587585
──────────────────
586+
587+
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
588588
Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
589589
`);
590590

@@ -616,10 +616,10 @@ describe("metrics", () => {
616616
await runWrangler("docs arg");
617617
expect(std.out).toMatchInlineSnapshot(`
618618
"
619-
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
620-
621619
⛅️ wrangler x.x.x
622620
──────────────────
621+
622+
Cloudflare collects anonymous telemetry about your usage of Wrangler. Learn more at https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/telemetry.md
623623
Opening a link in your default browser: FAKE_DOCS_URL:{\\"params\\":\\"query=arg&hitsPerPage=1&getRankingInfo=0\\"}"
624624
`);
625625
expect(requests.count).toBe(2);

‎packages/wrangler/src/core/CommandHandledError.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
/**
2+
* A wrapper that indicates the original error was thrown from a command handler
3+
* that has already sent telemetry (started + errored events).
4+
*
5+
* When this error is caught in index.ts, the outer error handler should:
6+
* 1. NOT send fallback telemetry (it's already been sent)
7+
* 2. Unwrap and rethrow the original error for proper error handling/display
8+
*
9+
* This is used to distinguish between:
10+
* - Errors from command handlers (telemetry sent by handler)
11+
* - Yargs validation errors (telemetry needs to be sent by fallback handler)
12+
*/
13+
export class CommandHandledError {
14+
constructor(public readonly originalError: unknown) {}
15+
}

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

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,38 @@ function isNetworkFetchFailedError(e: unknown): boolean {
247247
return false;
248248
}
249249

250+
/**
251+
* Determines the error type for telemetry purposes.
252+
* This is a pure function that doesn't log or have side effects.
253+
*/
254+
export function getErrorType(e: unknown): string | undefined {
255+
if (isCloudflareAPIDNSError(e)) {
256+
return "DNSError";
257+
}
258+
if (isPermissionError(e)) {
259+
return "PermissionError";
260+
}
261+
if (isFileNotFoundError(e)) {
262+
return "FileNotFoundError";
263+
}
264+
if (isCloudflareAPIConnectionTimeoutError(e)) {
265+
return "ConnectionTimeout";
266+
}
267+
if (
268+
isAuthenticationError(e) ||
269+
(e instanceof UserError &&
270+
e.cause instanceof ApiError &&
271+
e.cause.status === 403)
272+
) {
273+
return "AuthenticationError";
274+
}
275+
if (isBuildFailure(e) || isBuildFailureFromCause(e)) {
276+
return "BuildFailure";
277+
}
278+
// Fallback to constructor name
279+
return e instanceof Error ? e.constructor.name : undefined;
280+
}
281+
250282
/**
251283
* Handles an error thrown during command execution.
252284
*
@@ -256,9 +288,8 @@ export async function handleError(
256288
e: unknown,
257289
args: ReadConfigCommandArgs,
258290
subCommandParts: string[]
259-
) {
291+
): Promise<void> {
260292
let mayReport = true;
261-
let errorType: string | undefined;
262293
let loggableException = e;
263294

264295
logger.log(""); // Just adds a bit of space
@@ -275,7 +306,6 @@ export async function handleError(
275306
// Handle DNS resolution errors to Cloudflare API with a user-friendly message
276307
if (isCloudflareAPIDNSError(e)) {
277308
mayReport = false;
278-
errorType = "DNSError";
279309
logger.error(dedent`
280310
Unable to resolve Cloudflare's API hostname (api.cloudflare.com or dash.cloudflare.com).
281311
@@ -287,13 +317,12 @@ export async function handleError(
287317
288318
Please check your network connection and DNS settings.
289319
`);
290-
return errorType;
320+
return;
291321
}
292322

293323
// Handle permission errors with a user-friendly message
294324
if (isPermissionError(e)) {
295325
mayReport = false;
296-
errorType = "PermissionError";
297326

298327
// Extract the error message and path, checking both the error and its cause
299328
const errorMessage = e instanceof Error ? e.message : String(e);
@@ -339,13 +368,12 @@ export async function handleError(
339368
340369
Please check the file permissions and try again.
341370
`);
342-
return errorType;
371+
return;
343372
}
344373

345374
// Handle file not found errors with a user-friendly message
346375
if (isFileNotFoundError(e)) {
347376
mayReport = false;
348-
errorType = "FileNotFoundError";
349377

350378
// Extract the error message and path, checking both the error and its cause
351379
const errorMessage = e instanceof Error ? e.message : String(e);
@@ -390,19 +418,18 @@ export async function handleError(
390418
391419
Please check the file path and try again.
392420
`);
393-
return errorType;
421+
return;
394422
}
395423

396424
// Handle connection timeout errors to Cloudflare API with a user-friendly message
397425
if (isCloudflareAPIConnectionTimeoutError(e)) {
398426
mayReport = false;
399-
errorType = "ConnectionTimeout";
400427
logger.error(
401428
"The request to Cloudflare's API timed out.\n" +
402429
"This is likely due to network connectivity issues or slow network speeds.\n" +
403430
"Please check your internet connection and try again."
404431
);
405-
return errorType;
432+
return;
406433
}
407434

408435
// Handle generic "fetch failed" / "Failed to fetch" network errors
@@ -457,7 +484,6 @@ export async function handleError(
457484
e.cause.status === 403)
458485
) {
459486
mayReport = false;
460-
errorType = "AuthenticationError";
461487
if (e.cause instanceof ApiError) {
462488
logger.error(e.cause);
463489
} else {
@@ -519,12 +545,9 @@ export async function handleError(
519545
);
520546
} else if (isBuildFailure(e)) {
521547
mayReport = false;
522-
errorType = "BuildFailure";
523-
524548
logBuildFailure(e.errors, e.warnings);
525549
} else if (isBuildFailureFromCause(e)) {
526550
mayReport = false;
527-
errorType = "BuildFailure";
528551
logBuildFailure(e.cause.errors, e.cause.warnings);
529552
} else if (e instanceof Cloudflare.APIError) {
530553
const error = new APIError({
@@ -576,6 +599,4 @@ export async function handleError(
576599
) {
577600
await captureGlobalException(loggableException);
578601
}
579-
580-
return errorType;
581602
}

‎packages/wrangler/src/core/register-yargs-command.ts‎

Lines changed: 80 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { UserError as ContainersUserError } from "@cloudflare/containers-shared/src/error";
12
import {
23
defaultWranglerConfig,
34
FatalError,
@@ -11,10 +12,13 @@ import { createCloudflareClient } from "../cfetch/internal";
1112
import { readConfig } from "../config";
1213
import { run } from "../experimental-flags";
1314
import { logger } from "../logger";
15+
import { getMetricsDispatcher } from "../metrics";
1416
import { writeOutput } from "../output";
1517
import { dedent } from "../utils/dedent";
1618
import { isLocal, printResourceLocation } from "../utils/is-local";
1719
import { printWranglerBanner } from "../wrangler-banner";
20+
import { CommandHandledError } from "./CommandHandledError";
21+
import { getErrorType } from "./handle-errors";
1822
import { demandSingleValue } from "./helpers";
1923
import type { CommonYargsArgv, SubHelp } from "../yargs-types";
2024
import type {
@@ -30,7 +34,8 @@ import type { PositionalOptions } from "yargs";
3034
*/
3135
export function createRegisterYargsCommand(
3236
yargs: CommonYargsArgv,
33-
subHelp: SubHelp
37+
subHelp: SubHelp,
38+
argv: string[]
3439
) {
3540
return function registerCommand(
3641
segment: string,
@@ -97,13 +102,19 @@ export function createRegisterYargsCommand(
97102
registerSubTreeCallback();
98103
},
99104
// Only attach the handler for commands, not namespaces
100-
def.type === "command" ? createHandler(def, def.command) : undefined
105+
def.type === "command" ? createHandler(def, def.command, argv) : undefined
101106
);
102107
};
103108
}
104109

105-
function createHandler(def: CommandDefinition, commandName: string) {
110+
function createHandler(
111+
def: CommandDefinition,
112+
commandName: string,
113+
argv: string[]
114+
) {
106115
return async function handler(args: HandlerArgs<NamedArgDefinitions>) {
116+
const startTime = Date.now();
117+
107118
try {
108119
const shouldPrintBanner = def.behaviour?.printBanner;
109120

@@ -165,7 +176,7 @@ function createHandler(def: CommandDefinition, commandName: string) {
165176
AUTOCREATE_RESOURCES: args.experimentalAutoCreate,
166177
};
167178

168-
await run(experimentalFlags, () => {
179+
await run(experimentalFlags, async () => {
169180
const config =
170181
def.behaviour?.provideConfig ?? true
171182
? readConfig(args, {
@@ -175,6 +186,14 @@ function createHandler(def: CommandDefinition, commandName: string) {
175186
})
176187
: defaultWranglerConfig;
177188

189+
// Create metrics dispatcher with config info
190+
const dispatcher = getMetricsDispatcher({
191+
sendMetrics: config.send_metrics,
192+
hasAssets: !!config.assets?.directory,
193+
configPath: config.configPath,
194+
argv,
195+
});
196+
178197
if (def.behaviour?.warnIfMultipleEnvsConfiguredButNoneSpecified) {
179198
if (!("env" in args) && config.configPath) {
180199
const { rawConfig } = experimental_readRawConfig(
@@ -196,23 +215,67 @@ function createHandler(def: CommandDefinition, commandName: string) {
196215
}
197216
}
198217

199-
return def.handler(args, {
200-
sdk: createCloudflareClient(config),
201-
config,
202-
errors: { UserError, FatalError },
203-
logger,
204-
fetchResult,
218+
// Send "started" event
219+
dispatcher.sendCommandEvent("wrangler command started", {
220+
command: commandName,
221+
args,
205222
});
206-
});
207223

208-
// TODO(telemetry): send command completed event
209-
} catch (err) {
210-
// TODO(telemetry): send command errored event
224+
try {
225+
const result = await def.handler(args, {
226+
sdk: createCloudflareClient(config),
227+
config,
228+
errors: { UserError, FatalError },
229+
logger,
230+
fetchResult,
231+
});
211232

233+
// Send "completed" event
234+
const durationMs = Date.now() - startTime;
235+
dispatcher.sendCommandEvent("wrangler command completed", {
236+
command: commandName,
237+
args,
238+
durationMs,
239+
durationSeconds: durationMs / 1000,
240+
durationMinutes: durationMs / 1000 / 60,
241+
});
242+
243+
return result;
244+
} catch (err) {
245+
// If the error is already a CommandHandledError (e.g., from a nested wrangler.parse() call),
246+
// don't wrap it again - just rethrow it
247+
if (err instanceof CommandHandledError) {
248+
throw err;
249+
}
250+
251+
// Send "errored" event
252+
const durationMs = Date.now() - startTime;
253+
dispatcher.sendCommandEvent("wrangler command errored", {
254+
command: commandName,
255+
args,
256+
durationMs,
257+
durationSeconds: durationMs / 1000,
258+
durationMinutes: durationMs / 1000 / 60,
259+
errorType: getErrorType(err),
260+
errorMessage:
261+
err instanceof UserError || err instanceof ContainersUserError
262+
? err.telemetryMessage
263+
: undefined,
264+
});
265+
// Wrap the error to signal that telemetry has been sent
266+
throw new CommandHandledError(err);
267+
}
268+
});
269+
} catch (err) {
212270
// Write handler failure to output file if one exists
213-
if (err instanceof Error) {
214-
const code = "code" in err ? (err.code as number) : undefined;
215-
const message = "message" in err ? (err.message as string) : undefined;
271+
// Unwrap CommandHandledError to get the original error for output
272+
const outputErr =
273+
err instanceof CommandHandledError ? err.originalError : err;
274+
if (outputErr instanceof Error) {
275+
const code =
276+
"code" in outputErr ? (outputErr.code as number) : undefined;
277+
const message =
278+
"message" in outputErr ? (outputErr.message as string) : undefined;
216279
writeOutput({
217280
type: "command-failed",
218281
version: 1,

0 commit comments

Comments
 (0)