-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor(wrangler): move telemetry events from yargs middleware to command handlers #12055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@petebacondarwin could you please create an issue to clean the deprecated stuff and add this to the v5 milestone. @NuroDev has a PR touching the "non-Command" commands and IMO we should not introduce (convoluted) code for something that is deprecated/going to be removed soon if non criticial. If I read this correctly, we only need to migrate the |
892be6d to
4f1c154
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: |
4f1c154 to
3fd5325
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…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
…reateCommand Migrate containers and cloudchamber CLI commands from the legacy yargs defineCommand approach to the new createCommand pattern. This aligns these commands with the updated telemetry handling where events are dispatched directly from command handlers rather than middleware.
…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
3fd5325 to
7e9344a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Pete!
Looks great for the first pass!
Do you think it is possible to split this PR in 2?
- the metrcis
- the migration of the commands
If it's easy for an AI maybe we should do that?
I take a quick look at what Devin flagged, some of that should be looked at I think.
I'll do a second (after you look at if the PR can be separated)
| ); | ||
| // so testing the actual UI will be harder than expected | ||
| // TODO: think better on how to test UI actions | ||
| expect(std.out).toMatchInlineSnapshot(MOCK_DEPLOYMENTS_COMPLEX_RESPONSE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look inline to me 🤔
| " | ||
| ⛅️ wrangler x.x.x | ||
| ────────────────── | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the changes, the output was valid JSON, was that a req?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I'll find out.
| wrangler cloudchamber registries credentials [domain] get a temporary password for a specific domain | ||
| wrangler cloudchamber registries remove [domain] removes the registry at the given domain | ||
| wrangler cloudchamber registries list list registries configured for this account | ||
| Configure registries via Cloudchamber [alpha] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this wanted ?
-> % wrangler r2
wrangler r2
📦 Manage R2 buckets & objects
COMMANDS
wrangler r2 object Manage R2 objects
wrangler r2 bucket Manage R2 buckets
wrangler r2 sql Send queries and manage R2 SQL [open-beta]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I missed a namespace definition here...
| return Buffer.from(`v1:${credentials.password}`).toString("base64"); | ||
| } | ||
|
|
||
| // --- New createCommand-based commands --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete ?
| throw err; | ||
| } | ||
|
|
||
| // Send "errored" event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Send "errored" event |
| const code = | ||
| "code" in outputErr ? (outputErr.code as number) : undefined; | ||
| const message = | ||
| "message" in outputErr ? (outputErr.message as string) : undefined; | ||
| writeOutput({ | ||
| type: "command-failed", | ||
| version: 1, | ||
| code, | ||
| message, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks weird to use shorthand to write longer code
| writeOutput({ | |
| type: "command-failed", | |
| version: 1, | |
| code: "code" in outputErr ? (outputErr.code as number) : undefined, | |
| message: "message" in outputErr ? (outputErr.message as string) : undefined, | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this suggestion.
I find it easier to parse logic in creating a variable outside of an object literal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid personal pref.
I pref my way because it also clearly tells you that the var is not used elsewhere, easier to parse for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it turns out that we don't need to do any casting for message!
| } | ||
|
|
||
| const durationMs = Date.now() - startTime; | ||
| // Fallback telemetry for errors that occurred before handler ran |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO "Fallback" is more confusing than helping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll reword
| "wrangler command completed", | ||
| { | ||
| // Send "started" event (since handler never got to send it) | ||
| dispatcher.sendCommandEvent("wrangler command started", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any interest in sending a start to send an error right after?
maybe the errorType could be used to flag that instead of sending 2 events.
If there is a actually a need, it would be nice to explain why in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The telemetry around commands always comes in pairs:
- start + completed
- start + errored
So this keeps that symmetry and makes reporting on the telemetry simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question was "why".
Here it makes no sense to send 2 back to back event (on top of that there is a free form errorType)
A "why" could be a limitation of the backend, I think it should be documented.
@vicb - The PR is made up of 3 commits, which are exactly that separation. Is this enough for you? |
| " | ||
| ⛅️ wrangler x.x.x | ||
| ────────────────── |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about JSON output...
The comments for the PR review were taking from the previous PR (#12055)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devin Review found 1 new potential issue.
🔴 1 issue in files not directly in the diff
🔴 OpenAPI.BASE cached globally causes containers vs cloudchamber requests to hit the wrong API base after first command (packages/wrangler/src/cloudchamber/common.ts:182-190)
fillOpenAPIConfiguration() only sets OpenAPI.BASE when it is currently empty, but the base URL depends on the scope (containers vs cloudchamber).
Actual behavior: if the first command that runs sets OpenAPI.BASE to /accounts/:id/cloudchamber, then later running a containers command will reuse that base and send containers requests to the cloudchamber API prefix (or vice versa).
Expected behavior: OpenAPI.BASE should be configured per-scope (or reset/updated on each call), so that containers calls go to /accounts/:id/containers and cloudchamber calls go to /accounts/:id/cloudchamber.
Impact: commands can make requests to incorrect endpoints, leading to failures or potentially unintended API interactions.
Click to expand
In fillOpenAPIConfiguration(), the base is only set once:
if (OpenAPI.BASE.length === 0) {
const [base] = await wrap(getAPIUrl(config, accountId, scope));
OpenAPI.BASE = base;
}This ignores the scope parameter on subsequent calls. See packages/wrangler/src/cloudchamber/common.ts:182-190.
Recommendation: Make OpenAPI.BASE include the scope in its cache key (e.g., store per-scope base in a map), or always recompute and assign OpenAPI.BASE each time fillOpenAPIConfiguration() is called (and ensure concurrent calls can’t race).
View issue and 12 additional flags in Devin Review.
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:
Key changes:
A picture of a cute animal (not mandatory, but encouraged)