From d8a1a88301b5976c05f736bdf4293fbfdc77ef92 Mon Sep 17 00:00:00 2001 From: Samuel Macleod Date: Fri, 13 Dec 2024 18:34:51 +0000 Subject: [PATCH 1/5] Add additional metrics * Detect Pages & Workers CI * Filter out default args (e.g. --x-versions, --x-dev-env, and --latest) --- .../wrangler/src/__tests__/metrics.test.ts | 42 ++++++++++++--- packages/wrangler/src/index.ts | 52 ++++++++++++------- packages/wrangler/src/is-ci.ts | 4 +- .../src/metrics/metrics-dispatcher.ts | 20 +++++-- packages/wrangler/src/metrics/types.ts | 8 +++ packages/wrangler/turbo.json | 1 + 6 files changed, 94 insertions(+), 33 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index 876e1754030d..5d867d24a5ca 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -186,15 +186,11 @@ describe("metrics", () => { isFirstUsage: false, configFileType: "toml", isCI: false, + isPagesCI: false, + isWorkersCI: false, isInteractive: true, - argsUsed: [ - "j", - "search", - "xGradualRollouts", - "xJsonConfig", - "xVersions", - ], - argsCombination: "j, search, xGradualRollouts, xJsonConfig, xVersions", + argsUsed: [], + argsCombination: "", command: "wrangler docs", args: { xJsonConfig: true, @@ -343,6 +339,36 @@ describe("metrics", () => { expect(std.debug).toContain('isCI":true'); }); + it("should mark isPagesCI as true if running in Pages CI", async () => { + vi.stubEnv("CF_PAGES", "1"); + const requests = mockMetricRequest(); + + await runWrangler("docs arg"); + + expect(requests.count).toBe(2); + expect(std.debug).toContain('isPagesCI":true'); + }); + + it("should mark isWorkersCI as true if running in Workers CI", async () => { + vi.stubEnv("WORKERS_CI", "1"); + const requests = mockMetricRequest(); + + await runWrangler("docs arg"); + + expect(requests.count).toBe(2); + expect(std.debug).toContain('isWorkersCI":true'); + }); + + it("should include args provided by the user", async () => { + const requests = mockMetricRequest(); + + await runWrangler("docs arg --search 'some search term'"); + + expect(requests.count).toBe(2); + // Notably, this _doesn't_ include default args (e.g. --x-versions) + expect(std.debug).toContain('argsCombination":"search"'); + }); + it("should mark as non-interactive if running in non-interactive environment", async () => { setIsTTY(false); const requests = mockMetricRequest(); diff --git a/packages/wrangler/src/index.ts b/packages/wrangler/src/index.ts index 2e0e4d076b72..c99b28cd4f06 100644 --- a/packages/wrangler/src/index.ts +++ b/packages/wrangler/src/index.ts @@ -1153,10 +1153,14 @@ export async function main(argv: string[]): Promise { addBreadcrumb(command); // NB despite 'applyBeforeValidation = true', this runs *after* yargs 'validates' options, // e.g. if a required arg is missing, yargs will error out before we send any events :/ - dispatcher?.sendCommandEvent("wrangler command started", { - command, - args, - }); + dispatcher?.sendCommandEvent( + "wrangler command started", + { + command, + args, + }, + argv + ); }, /* applyBeforeValidation */ true); let cliHandlerThrew = false; @@ -1165,13 +1169,17 @@ export async function main(argv: string[]): Promise { const durationMs = Date.now() - startTime; - dispatcher?.sendCommandEvent("wrangler command completed", { - command, - args: metricsArgs, - durationMs, - durationSeconds: durationMs / 1000, - durationMinutes: durationMs / 1000 / 60, - }); + dispatcher?.sendCommandEvent( + "wrangler command completed", + { + command, + args: metricsArgs, + durationMs, + durationSeconds: durationMs / 1000, + durationMinutes: durationMs / 1000 / 60, + }, + argv + ); } catch (e) { cliHandlerThrew = true; let mayReport = true; @@ -1281,15 +1289,19 @@ export async function main(argv: string[]): Promise { const durationMs = Date.now() - startTime; - dispatcher?.sendCommandEvent("wrangler command errored", { - command, - args: metricsArgs, - durationMs, - durationSeconds: durationMs / 1000, - durationMinutes: durationMs / 1000 / 60, - errorType: - errorType ?? (e instanceof Error ? e.constructor.name : undefined), - }); + dispatcher?.sendCommandEvent( + "wrangler command errored", + { + command, + args: metricsArgs, + durationMs, + durationSeconds: durationMs / 1000, + durationMinutes: durationMs / 1000 / 60, + errorType: + errorType ?? (e instanceof Error ? e.constructor.name : undefined), + }, + argv + ); throw e; } finally { diff --git a/packages/wrangler/src/is-ci.ts b/packages/wrangler/src/is-ci.ts index 7ec96824ba2b..77891fa2c80d 100644 --- a/packages/wrangler/src/is-ci.ts +++ b/packages/wrangler/src/is-ci.ts @@ -9,6 +9,8 @@ import isCI from "is-ci"; export const CI = { /** Is Wrangler currently running in a CI? */ isCI() { - return isCI; + return ( + isCI || process.env.CF_PAGES === "1" || process.env.WORKERS_CI === "1" + ); }, }; diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index 28a5b4fa2a6e..b4918a7db518 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -73,7 +73,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { properties: Omit< Extract["properties"], keyof CommonEventProperties - > + >, + argv?: string[] ) { try { if ( @@ -91,7 +92,7 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { printMetricsBanner(); } - const argsUsed = sanitiseUserInput(properties.args ?? {}); + const argsUsed = sanitiseUserInput(properties.args ?? {}, argv); const argsCombination = argsUsed.sort().join(", "); const commonEventProperties: CommonEventProperties = { amplitude_session_id, @@ -104,6 +105,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { isFirstUsage: readMetricsConfig().permission === undefined, configFileType: configFormat(options.configPath), isCI: CI.isCI(), + isPagesCI: process.env.CF_PAGES === "1", + isWorkersCI: process.env.WORKERS_CI === "1", isInteractive: isInteractive(), argsUsed, argsCombination, @@ -213,11 +216,20 @@ const normalise = (arg: string) => { }; const exclude = new Set(["$0", "_"]); -/** just some pretty naive cleaning so we don't send "experimental-versions", "experimentalVersions", "x-versions" and "xVersions" etc. */ -const sanitiseUserInput = (argsWithValues: Record) => { +/** + * just some pretty naive cleaning so we don't send duplicates of "experimental-versions", "experimentalVersions", "x-versions" and "xVersions" etc. + * optionally, if an argv is provided remove all args that were not specified in argv (which means that default values will be filtered out) + */ +const sanitiseUserInput = ( + argsWithValues: Record, + argv?: string[] +) => { const result: string[] = []; const args = Object.keys(argsWithValues); for (const arg of args) { + if (Array.isArray(argv) && !argv.some((a) => a.includes(arg))) { + continue; + } if (exclude.has(arg)) { continue; } diff --git a/packages/wrangler/src/metrics/types.ts b/packages/wrangler/src/metrics/types.ts index eeece4d58eb1..d2b514824fbb 100644 --- a/packages/wrangler/src/metrics/types.ts +++ b/packages/wrangler/src/metrics/types.ts @@ -40,6 +40,14 @@ export type CommonEventProperties = { * Whether the Wrangler client is running in CI */ isCI: boolean; + /** + * Whether the Wrangler client is running in Pages CI + */ + isPagesCI: boolean; + /** + * Whether the Wrangler client is running in Workers CI + */ + isWorkersCI: boolean; /** * Whether the Wrangler client is running in an interactive instance */ diff --git a/packages/wrangler/turbo.json b/packages/wrangler/turbo.json index 5afbdd89cc1c..4647f496f2e3 100644 --- a/packages/wrangler/turbo.json +++ b/packages/wrangler/turbo.json @@ -36,6 +36,7 @@ "EXPERIMENTAL_MIDDLEWARE", "FORMAT_WRANGLER_ERRORS", "CF_PAGES", + "WORKERS_CI", "CI", "CF_PAGES_UPLOAD_JWT", "EXPERIMENTAL_MIDDLEWARE", From 5e5c3d066d88820e6409aa249b37eb770d1e0785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Somhairle=20MacLe=C3=B2id?= Date: Fri, 13 Dec 2024 18:37:53 +0000 Subject: [PATCH 2/5] Create selfish-yaks-hope.md --- .changeset/selfish-yaks-hope.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/selfish-yaks-hope.md diff --git a/.changeset/selfish-yaks-hope.md b/.changeset/selfish-yaks-hope.md new file mode 100644 index 000000000000..7d378bed1f0c --- /dev/null +++ b/.changeset/selfish-yaks-hope.md @@ -0,0 +1,7 @@ +--- +"wrangler": patch +--- + +Expand metrics collection to: +* Detect Pages & Workers CI +* Filter out default args (e.g. `--x-versions`, `--x-dev-env`, and `--latest`) by only including args that were in `argv` From 85809f8527372e59582773f50a2feb1eb3c7f932 Mon Sep 17 00:00:00 2001 From: Samuel Macleod Date: Fri, 13 Dec 2024 18:56:47 +0000 Subject: [PATCH 3/5] fix format --- .changeset/selfish-yaks-hope.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.changeset/selfish-yaks-hope.md b/.changeset/selfish-yaks-hope.md index 7d378bed1f0c..f4325b5dd866 100644 --- a/.changeset/selfish-yaks-hope.md +++ b/.changeset/selfish-yaks-hope.md @@ -3,5 +3,6 @@ --- Expand metrics collection to: -* Detect Pages & Workers CI -* Filter out default args (e.g. `--x-versions`, `--x-dev-env`, and `--latest`) by only including args that were in `argv` + +- Detect Pages & Workers CI +- Filter out default args (e.g. `--x-versions`, `--x-dev-env`, and `--latest`) by only including args that were in `argv` From b7bf528881bbe4ab16c84bca937a898ea4192225 Mon Sep 17 00:00:00 2001 From: Samuel Macleod Date: Tue, 17 Dec 2024 11:53:38 +0000 Subject: [PATCH 4/5] extract CI check --- packages/wrangler/src/is-ci.ts | 11 ++++++++--- packages/wrangler/src/is-interactive.ts | 4 ++-- packages/wrangler/src/metrics/metrics-dispatcher.ts | 6 +++--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/wrangler/src/is-ci.ts b/packages/wrangler/src/is-ci.ts index 77891fa2c80d..92c4a517bb7e 100644 --- a/packages/wrangler/src/is-ci.ts +++ b/packages/wrangler/src/is-ci.ts @@ -1,5 +1,12 @@ import isCI from "is-ci"; +export function isPagesCI() { + return process.env.CF_PAGES === "1"; +} + +export function isWorkersCI() { + return process.env.WORKERS_CI === "1"; +} /** * Use this object to find out if we are currently running in a continuous integration environment. * @@ -9,8 +16,6 @@ import isCI from "is-ci"; export const CI = { /** Is Wrangler currently running in a CI? */ isCI() { - return ( - isCI || process.env.CF_PAGES === "1" || process.env.WORKERS_CI === "1" - ); + return isCI || isPagesCI() || isWorkersCI(); }, }; diff --git a/packages/wrangler/src/is-interactive.ts b/packages/wrangler/src/is-interactive.ts index 49dd90257405..89ff8d6c8c48 100644 --- a/packages/wrangler/src/is-interactive.ts +++ b/packages/wrangler/src/is-interactive.ts @@ -1,5 +1,5 @@ import { isInteractive as __isInteractive } from "@cloudflare/cli/interactive"; -import { CI } from "./is-ci"; +import { CI, isPagesCI, isWorkersCI } from "./is-ci"; /** * Test whether the process is "interactive". @@ -7,7 +7,7 @@ import { CI } from "./is-ci"; * or you're piping values from / to another process, etc */ export default function isInteractive(): boolean { - if (process.env.CF_PAGES === "1") { + if (isPagesCI() || isWorkersCI()) { return false; } diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index b4918a7db518..be06f4326da6 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -4,7 +4,7 @@ import { configFormat } from "../config"; import isInteractive from "../is-interactive"; import { logger } from "../logger"; import { sniffUserAgent } from "../package-manager"; -import { CI } from "./../is-ci"; +import { CI, isPagesCI, isWorkersCI } from "./../is-ci"; import { getNodeVersion, getOS, @@ -105,8 +105,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { isFirstUsage: readMetricsConfig().permission === undefined, configFileType: configFormat(options.configPath), isCI: CI.isCI(), - isPagesCI: process.env.CF_PAGES === "1", - isWorkersCI: process.env.WORKERS_CI === "1", + isPagesCI: isPagesCI(), + isWorkersCI: isWorkersCI(), isInteractive: isInteractive(), argsUsed, argsCombination, From 0d6ad0c23590879a07c03e93a2655dd62724579b Mon Sep 17 00:00:00 2001 From: Samuel Macleod Date: Tue, 17 Dec 2024 12:33:43 +0000 Subject: [PATCH 5/5] sanitise wrangler login --- packages/wrangler/src/__tests__/metrics.test.ts | 14 ++++++++++++++ packages/wrangler/src/__tests__/vitest.setup.ts | 5 +++-- .../wrangler/src/metrics/metrics-dispatcher.ts | 3 +++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index 5d867d24a5ca..ffccf2e75a9a 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -359,6 +359,20 @@ describe("metrics", () => { expect(std.debug).toContain('isWorkersCI":true'); }); + it("should not send arguments with wrangler login", async () => { + const requests = mockMetricRequest(); + + await expect( + runWrangler("login username password") + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Unknown arguments: username, password]` + ); + + expect(requests.count).toBe(2); + expect(std.debug).toContain('"argsCombination":""'); + expect(std.debug).toContain('"command":"wrangler login"'); + }); + it("should include args provided by the user", async () => { const requests = mockMetricRequest(); diff --git a/packages/wrangler/src/__tests__/vitest.setup.ts b/packages/wrangler/src/__tests__/vitest.setup.ts index 19ba6cceefd2..a5edb90b087a 100644 --- a/packages/wrangler/src/__tests__/vitest.setup.ts +++ b/packages/wrangler/src/__tests__/vitest.setup.ts @@ -135,8 +135,9 @@ vi.mock("../user/generate-auth-url", () => { }; }); -vi.mock("../is-ci", () => { - return { CI: { isCI: vi.fn().mockImplementation(() => false) } }; +vi.mock("../is-ci", async (importOriginal) => { + const original = await importOriginal(); + return { ...original, CI: { isCI: vi.fn().mockImplementation(() => false) } }; }); vi.mock("../user/generate-random-state", () => { diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index be06f4326da6..53f2e32b3a00 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -77,6 +77,9 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { argv?: string[] ) { try { + if (properties.command?.startsWith("wrangler login")) { + properties.command = "wrangler login"; + } if ( properties.command === "wrangler telemetry disable" || properties.command === "wrangler metrics disable"