refactor(wrangler): move telemetry events from yargs middleware to command handlers #12069
Conversation
…mmand 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
|
…hers Move the requests tracking from per-dispatcher instance to module-level scope so that all dispatchers share the same Set of pending requests. This ensures all metrics requests are awaited before Wrangler exits, regardless of which dispatcher created them. - Add module-level pendingRequests Set with auto-cleanup via .finally() - Export waitForAllMetricsDispatches() to await all pending requests - Remove the per-dispatcher requests getter
| * Wait for all pending metrics requests to complete. | ||
| * This should be called before the process exits to ensure all metrics are sent. | ||
| */ | ||
| export function waitForAllMetricsDispatches(): Promise<void> { |
There was a problem hiding this comment.
if you want to keep it sync getMetricsDispatchesCompleted could be a good name but looking at the call sites, making it async (+ keep the name) sounds like the best thing to do. There is only one call that is not awaited which is not a problem as it needs a promise.
Context: A lot (including me) have been confused by ctx.waitUntil being sync. My understanding is that the runtime team would change the name if it wasn't too late.
There was a problem hiding this comment.
What do you mean by "make it async"? It returns a Promise, which makes it async from the outside. How it is implemented internally makes no difference.
There was a problem hiding this comment.
export async function waitForAllMetricsDispatches(): Promise<void>
or drop "waitFor" because sync function can't wait
There was a problem hiding this comment.
To be clearer:
export async function waitForAllMetricsDispatches(): Promise<void> {
await Promise.allSettled(pendingRequests);
}There was a problem hiding this comment.
I still don't understand how this makes the function any clearer. It has the same signature: () => Promise<void>.
The caller doesn't see the async modifier in intellisense.
There was a problem hiding this comment.
Could not explain any better than:
The comments for the PR review were taking from the previous PR (#12055)
petebacondarwin
force-pushed
the
pbd/wrangler/telemetry-event-refactor
branch
from
be3fd85 to
e0829f5
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
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
Key changes:
A picture of a cute animal (not mandatory, but encouraged)