From 42b942916efbd4eb8060e4d61c2e805ec78a1a89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Somhairle=20MacLe=C3=B2id?= Date: Tue, 17 Dec 2024 17:13:43 +0000 Subject: [PATCH] Add additional metrics (#7549) * Add additional metrics * Detect Pages & Workers CI * Filter out default args (e.g. --x-versions, --x-dev-env, and --latest) * Create selfish-yaks-hope.md * fix format * extract CI check * sanitise wrangler login --- .changeset/selfish-yaks-hope.md | 8 +++ .../wrangler/src/__tests__/metrics.test.ts | 56 ++++++++++++++++--- .../wrangler/src/__tests__/vitest.setup.ts | 5 +- packages/wrangler/src/index.ts | 52 ++++++++++------- packages/wrangler/src/is-ci.ts | 9 ++- packages/wrangler/src/is-interactive.ts | 4 +- .../src/metrics/metrics-dispatcher.ts | 25 +++++++-- packages/wrangler/src/metrics/types.ts | 8 +++ packages/wrangler/turbo.json | 1 + 9 files changed, 130 insertions(+), 38 deletions(-) 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..f4325b5dd866 --- /dev/null +++ b/.changeset/selfish-yaks-hope.md @@ -0,0 +1,8 @@ +--- +"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` diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index 876e1754030d..ffccf2e75a9a 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,50 @@ 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 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(); + + 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/__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/index.ts b/packages/wrangler/src/index.ts index 1511d4da84b5..80ea83c4723f 100644 --- a/packages/wrangler/src/index.ts +++ b/packages/wrangler/src/index.ts @@ -1169,10 +1169,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; @@ -1181,13 +1185,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; @@ -1297,15 +1305,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..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,6 +16,6 @@ import isCI from "is-ci"; export const CI = { /** Is Wrangler currently running in a CI? */ isCI() { - return isCI; + 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 28a5b4fa2a6e..53f2e32b3a00 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, @@ -73,9 +73,13 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { properties: Omit< Extract["properties"], keyof CommonEventProperties - > + >, + argv?: string[] ) { try { + if (properties.command?.startsWith("wrangler login")) { + properties.command = "wrangler login"; + } if ( properties.command === "wrangler telemetry disable" || properties.command === "wrangler metrics disable" @@ -91,7 +95,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 +108,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { isFirstUsage: readMetricsConfig().permission === undefined, configFileType: configFormat(options.configPath), isCI: CI.isCI(), + isPagesCI: isPagesCI(), + isWorkersCI: isWorkersCI(), isInteractive: isInteractive(), argsUsed, argsCombination, @@ -213,11 +219,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",