From 038af51768ae9f5b077327d67f6113233cec3858 Mon Sep 17 00:00:00 2001 From: emily-shen <69125074+emily-shen@users.noreply.github.com> Date: Thu, 21 Nov 2024 17:50:23 +0000 Subject: [PATCH 1/6] isCI and isNonInteractive --- .../wrangler/src/__tests__/metrics.test.ts | 45 ++++++++++++------- .../wrangler/src/metrics/metrics-config.ts | 6 --- .../src/metrics/metrics-dispatcher.ts | 6 +++ packages/wrangler/src/metrics/send-event.ts | 3 ++ 4 files changed, 38 insertions(+), 22 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index abda95a1aaf8..786d91dd4ca5 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -23,16 +23,21 @@ vi.mock("../metrics/helpers"); vi.unmock("../metrics/metrics-config"); describe("metrics", () => { + let isCISpy: MockInstance; const std = mockConsoleMethods(); + const { setIsTTY } = useMockIsTTY(); runInTempDir(); beforeEach(async () => { + isCISpy = vi.spyOn(CI, "isCI").mockReturnValue(false); + setIsTTY(true); vi.stubEnv("SPARROW_SOURCE_KEY", "MOCK_KEY"); logger.loggerLevel = "debug"; }); afterEach(() => { vi.unstubAllEnvs(); + isCISpy.mockClear(); }); describe("getMetricsDispatcher()", () => { @@ -211,11 +216,11 @@ describe("metrics", () => { // command started expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command started","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":0,"wranglerVersion":"1.2.3","isFirstUsage":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"positional"}}}` + `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command started","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":0,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"positional"}}}` ); // command completed expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command completed","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":1,"wranglerVersion":"1.2.3","isFirstUsage":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"positional"},"durationMs":0,"durationSeconds":0,"durationMinutes":0}}` + `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command completed","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":1,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"positional"},"durationMs":0,"durationSeconds":0,"durationMinutes":0}}` ); expect(std.out).toMatchInlineSnapshot(` " @@ -237,14 +242,34 @@ describe("metrics", () => { // command started expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command started","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":0,"wranglerVersion":"1.2.3","isFirstUsage":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"error"}}}` + `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command started","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":0,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"error"}}}` ); // command completed expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command errored","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":1,"wranglerVersion":"1.2.3","isFirstUsage":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"error"},"durationMs":0,"durationSeconds":0,"durationMinutes":0,"errorType":"UserError"}}` + `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command errored","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":1,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"error"},"durationMs":0,"durationSeconds":0,"durationMinutes":0,"errorType":"UserError"}}` ); }); + it("should mark isCI as true if running in CI", async () => { + isCISpy.mockReturnValue(true); + const requests = mockMetricRequest({}, {}); + + await runWrangler("command subcommand positional"); + + expect(requests.count).toBe(2); + expect(std.debug).toContain('isCI":true'); + }); + + it("should mark as non-interactive if running in non-interactive environment", async () => { + setIsTTY(false); + const requests = mockMetricRequest({}, {}); + + await runWrangler("command subcommand positional"); + + expect(requests.count).toBe(2); + expect(std.debug).toContain('isNonInteractive":true'); + }); + describe("banner", () => { beforeEach(() => { vi.mocked(getWranglerVersion).mockReturnValue("1.2.3"); @@ -323,12 +348,7 @@ describe("metrics", () => { }); describe("getMetricsConfig()", () => { - let isCISpy: MockInstance; - - const { setIsTTY } = useMockIsTTY(); beforeEach(() => { - // Default the mock TTY to interactive for all these tests. - setIsTTY(true); isCISpy = vi.spyOn(CI, "isCI").mockReturnValue(false); }); @@ -344,13 +364,6 @@ describe("metrics", () => { }); }); - it("should return false if running in a CI environment", async () => { - isCISpy.mockReturnValue(true); - expect(await getMetricsConfig({})).toMatchObject({ - enabled: false, - }); - }); - it("should return the sendMetrics argument for enabled if it is defined", async () => { expect(await getMetricsConfig({ sendMetrics: false })).toMatchObject({ enabled: false, diff --git a/packages/wrangler/src/metrics/metrics-config.ts b/packages/wrangler/src/metrics/metrics-config.ts index 306ebaa5b4f5..ed131146626e 100644 --- a/packages/wrangler/src/metrics/metrics-config.ts +++ b/packages/wrangler/src/metrics/metrics-config.ts @@ -86,12 +86,6 @@ export function getMetricsConfig({ } } - // We couldn't get the metrics permission from the project-level nor the user-level config. - // If we are not interactive or in a CI build then just bail out. - if (isNonInteractiveOrCI()) { - return { enabled: false, deviceId }; - } - // Otherwise, default to true writeMetricsConfig({ ...config, diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index 8ea528cc5c27..288dc11eeb5b 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -1,6 +1,8 @@ import chalk from "chalk"; import { fetch } from "undici"; +import isInteractive from "../is-interactive"; import { logger } from "../logger"; +import { CI } from "./../is-ci"; import { getOS, getPackageManager, @@ -26,6 +28,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { const platform = getPlatform(); const packageManager = getPackageManager(); const isFirstUsage = readMetricsConfig().permission === undefined; + const isCI = CI.isCI(); + const isNonInteractive = !isInteractive(); const amplitude_session_id = Date.now(); let amplitude_event_id = 0; @@ -76,6 +80,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { platform, packageManager, isFirstUsage, + isCI, + isNonInteractive, }; await dispatch({ diff --git a/packages/wrangler/src/metrics/send-event.ts b/packages/wrangler/src/metrics/send-event.ts index 2e224edd5f9e..6a5c9b51bb7c 100644 --- a/packages/wrangler/src/metrics/send-event.ts +++ b/packages/wrangler/src/metrics/send-event.ts @@ -139,6 +139,9 @@ export type CommonEventProperties = { amplitude_session_id: number; amplitude_event_id: number; + + isCI: boolean; + isNonInteractive: boolean; }; export type Events = From 9f2bde3b0a9a06e4bfefa3f354c09b7c1bda0cbb Mon Sep 17 00:00:00 2001 From: emily-shen <69125074+emily-shen@users.noreply.github.com> Date: Thu, 21 Nov 2024 20:19:44 +0000 Subject: [PATCH 2/6] add argsUsed and argsCombination --- .../wrangler/src/__tests__/metrics.test.ts | 128 +++++++++++++++++- .../src/metrics/metrics-dispatcher.ts | 24 ++++ packages/wrangler/src/metrics/send-event.ts | 2 + 3 files changed, 147 insertions(+), 7 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index 786d91dd4ca5..01185dc7730f 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -214,13 +214,69 @@ describe("metrics", () => { expect(requests.count).toBe(2); - // command started + const expectedStartReq = { + deviceId: "f82b1f46-eb7b-4154-aa9f-ce95f23b2288", + event: "wrangler command started", + timestamp: 1733961600000, + properties: { + amplitude_session_id: 1733961600000, + amplitude_event_id: 0, + wranglerVersion: "1.2.3", + isFirstUsage: false, + isCI: false, + isNonInteractive: false, + argsUsed: ["positional", "xgradualrollouts", "xversions"], + argsCombination: "positional, xgradualrollouts, xversions", + command: "wrangler command subcommand", + args: { + _: ["command", "subcommand"], + "experimental-versions": true, + "x-versions": true, + "experimental-gradual-rollouts": true, + xVersions: true, + experimentalGradualRollouts: true, + experimentalVersions: true, + $0: "wrangler", + positional: "positional", + }, + }, + }; expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command started","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":0,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"positional"}}}` + `Posting data ${JSON.stringify(expectedStartReq)}` ); + const expectedCompleteReq = { + deviceId: "f82b1f46-eb7b-4154-aa9f-ce95f23b2288", + event: "wrangler command completed", + timestamp: 1733961600000, + properties: { + amplitude_session_id: 1733961600000, + amplitude_event_id: 1, + wranglerVersion: "1.2.3", + isFirstUsage: false, + isCI: false, + isNonInteractive: false, + argsUsed: ["positional", "xgradualrollouts", "xversions"], + argsCombination: "positional, xgradualrollouts, xversions", + command: "wrangler command subcommand", + args: { + _: ["command", "subcommand"], + "experimental-versions": true, + "x-versions": true, + "experimental-gradual-rollouts": true, + xVersions: true, + experimentalGradualRollouts: true, + experimentalVersions: true, + $0: "wrangler", + positional: "positional", + }, + durationMs: 0, + durationSeconds: 0, + durationMinutes: 0, + }, + }; // command completed expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command completed","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":1,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"positional"},"durationMs":0,"durationSeconds":0,"durationMinutes":0}}` + `Posting data ${JSON.stringify(expectedCompleteReq)}` ); expect(std.out).toMatchInlineSnapshot(` " @@ -240,13 +296,71 @@ describe("metrics", () => { expect(requests.count).toBe(2); - // command started + const expectedStartReq = { + deviceId: "f82b1f46-eb7b-4154-aa9f-ce95f23b2288", + event: "wrangler command started", + timestamp: 1733961600000, + properties: { + amplitude_session_id: 1733961600000, + amplitude_event_id: 0, + wranglerVersion: "1.2.3", + isFirstUsage: false, + isCI: false, + isNonInteractive: false, + argsUsed: ["positional", "xgradualrollouts", "xversions"], + argsCombination: "positional, xgradualrollouts, xversions", + command: "wrangler command subcommand", + args: { + _: ["command", "subcommand"], + "experimental-versions": true, + "x-versions": true, + "experimental-gradual-rollouts": true, + xVersions: true, + experimentalGradualRollouts: true, + experimentalVersions: true, + $0: "wrangler", + positional: "error", + }, + }, + }; expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command started","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":0,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"error"}}}` + `Posting data ${JSON.stringify(expectedStartReq)}` ); - // command completed + + const expectedErrorReq = { + deviceId: "f82b1f46-eb7b-4154-aa9f-ce95f23b2288", + event: "wrangler command errored", + timestamp: 1733961600000, + properties: { + amplitude_session_id: 1733961600000, + amplitude_event_id: 1, + wranglerVersion: "1.2.3", + isFirstUsage: false, + isCI: false, + isNonInteractive: false, + argsUsed: ["positional", "xgradualrollouts", "xversions"], + argsCombination: "positional, xgradualrollouts, xversions", + command: "wrangler command subcommand", + args: { + _: ["command", "subcommand"], + "experimental-versions": true, + "x-versions": true, + "experimental-gradual-rollouts": true, + xVersions: true, + experimentalGradualRollouts: true, + experimentalVersions: true, + $0: "wrangler", + positional: "error", + }, + durationMs: 0, + durationSeconds: 0, + durationMinutes: 0, + errorType: "UserError", + }, + }; + expect(std.debug).toContain( - `Metrics dispatcher: Posting data {"deviceId":"f82b1f46-eb7b-4154-aa9f-ce95f23b2288","event":"wrangler command errored","timestamp":1733961600000,"properties":{"amplitude_session_id":1733961600000,"amplitude_event_id":1,"wranglerVersion":"1.2.3","isFirstUsage":false,"isCI":false,"isNonInteractive":false,"command":"wrangler command subcommand","args":{"_":["command","subcommand"],"experimental-versions":true,"x-versions":true,"experimental-gradual-rollouts":true,"xVersions":true,"experimentalGradualRollouts":true,"experimentalVersions":true,"$0":"wrangler","positional":"error"},"durationMs":0,"durationSeconds":0,"durationMinutes":0,"errorType":"UserError"}}` + `Posting data ${JSON.stringify(expectedErrorReq)}` ); }); diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index 288dc11eeb5b..beeaccd4e65d 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -73,6 +73,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { return; } printMetricsBanner(); + const argsUsed = normaliseArgs(Object.keys(properties.args ?? [])); + const argsCombination = argsUsed.sort().join(", "); const commonEventProperties: CommonEventProperties = { amplitude_session_id, amplitude_event_id: amplitude_event_id++, @@ -82,6 +84,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { isFirstUsage, isCI, isNonInteractive, + argsUsed, + argsCombination, }; await dispatch({ @@ -171,3 +175,23 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { } export type Properties = Record; + +/** just some pretty naive cleaning so we don't send "experimental-versions", "experimentalVersions", "x-versions" and "xVersions" etc. */ +const normaliseArgs = (args: string[]) => { + const exclude = new Set(["$0", "_"]); + const result: string[] = []; + for (const arg of args) { + if (exclude.has(arg)) { + continue; + } + const normalisedArg = arg + .replace("experimental", "x") + .replaceAll("-", "") + .toLowerCase(); + if (result.includes(normalisedArg)) { + continue; + } + result.push(normalisedArg); + } + return result; +}; diff --git a/packages/wrangler/src/metrics/send-event.ts b/packages/wrangler/src/metrics/send-event.ts index 6a5c9b51bb7c..74b6ba6bda94 100644 --- a/packages/wrangler/src/metrics/send-event.ts +++ b/packages/wrangler/src/metrics/send-event.ts @@ -142,6 +142,8 @@ export type CommonEventProperties = { isCI: boolean; isNonInteractive: boolean; + argsUsed: string[]; + argsCombination: string; }; export type Events = From cb73aa925977da0e8af375539d574a574c34de46 Mon Sep 17 00:00:00 2001 From: emily-shen <69125074+emily-shen@users.noreply.github.com> Date: Thu, 21 Nov 2024 20:45:21 +0000 Subject: [PATCH 3/6] don't include args if value is false --- packages/wrangler/src/__tests__/metrics.test.ts | 8 ++++++++ packages/wrangler/src/metrics/metrics-dispatcher.ts | 11 +++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index 01185dc7730f..933b35532aae 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -197,6 +197,10 @@ describe("metrics", () => { optional: { type: "string", }, + default: { + type: "boolean", + default: false, + }, }, positionalArgs: ["positional"], handler(args, ctx) { @@ -236,6 +240,7 @@ describe("metrics", () => { xVersions: true, experimentalGradualRollouts: true, experimentalVersions: true, + default: false, $0: "wrangler", positional: "positional", }, @@ -266,6 +271,7 @@ describe("metrics", () => { xVersions: true, experimentalGradualRollouts: true, experimentalVersions: true, + default: false, $0: "wrangler", positional: "positional", }, @@ -318,6 +324,7 @@ describe("metrics", () => { xVersions: true, experimentalGradualRollouts: true, experimentalVersions: true, + default: false, $0: "wrangler", positional: "error", }, @@ -349,6 +356,7 @@ describe("metrics", () => { xVersions: true, experimentalGradualRollouts: true, experimentalVersions: true, + default: false, $0: "wrangler", positional: "error", }, diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index beeaccd4e65d..409cf1afb55a 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -73,7 +73,7 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { return; } printMetricsBanner(); - const argsUsed = normaliseArgs(Object.keys(properties.args ?? [])); + const argsUsed = normaliseArgs(properties.args ?? {}); const argsCombination = argsUsed.sort().join(", "); const commonEventProperties: CommonEventProperties = { amplitude_session_id, @@ -177,13 +177,20 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { export type Properties = Record; /** just some pretty naive cleaning so we don't send "experimental-versions", "experimentalVersions", "x-versions" and "xVersions" etc. */ -const normaliseArgs = (args: string[]) => { +const normaliseArgs = (argsWithValues: Record) => { const exclude = new Set(["$0", "_"]); const result: string[] = []; + const args = Object.keys(argsWithValues); for (const arg of args) { if (exclude.has(arg)) { continue; } + if ( + typeof argsWithValues[arg] === "boolean" && + argsWithValues[arg] === false + ) { + continue; + } const normalisedArg = arg .replace("experimental", "x") .replaceAll("-", "") From 9de900272e3cedeb55dd3057557db45494940ec7 Mon Sep 17 00:00:00 2001 From: emily-shen <69125074+emily-shen@users.noreply.github.com> Date: Thu, 21 Nov 2024 21:01:21 +0000 Subject: [PATCH 4/6] redact arg values if string --- .../wrangler/src/__tests__/metrics.test.ts | 105 ++++++++++++++---- .../src/metrics/metrics-dispatcher.ts | 49 +++++++- 2 files changed, 131 insertions(+), 23 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index 933b35532aae..edf4f5ab3c9c 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -10,7 +10,10 @@ import { readMetricsConfig, writeMetricsConfig, } from "../metrics/metrics-config"; -import { getMetricsDispatcher } from "../metrics/metrics-dispatcher"; +import { + getMetricsDispatcher, + redactArgValues, +} from "../metrics/metrics-dispatcher"; import { mockConsoleMethods } from "./helpers/mock-console"; import { useMockIsTTY } from "./helpers/mock-istty"; import { msw } from "./helpers/msw"; @@ -201,6 +204,15 @@ describe("metrics", () => { type: "boolean", default: false, }, + array: { + type: "string", + array: true, + default: ["beep", "boop"], + }, + number: { + type: "number", + default: 42, + }, }, positionalArgs: ["positional"], handler(args, ctx) { @@ -229,11 +241,17 @@ describe("metrics", () => { isFirstUsage: false, isCI: false, isNonInteractive: false, - argsUsed: ["positional", "xgradualrollouts", "xversions"], - argsCombination: "positional, xgradualrollouts, xversions", + argsUsed: [ + "array", + "number", + "positional", + "xgradualrollouts", + "xversions", + ], + argsCombination: + "array, number, positional, xgradualrollouts, xversions", command: "wrangler command subcommand", args: { - _: ["command", "subcommand"], "experimental-versions": true, "x-versions": true, "experimental-gradual-rollouts": true, @@ -241,8 +259,9 @@ describe("metrics", () => { experimentalGradualRollouts: true, experimentalVersions: true, default: false, - $0: "wrangler", - positional: "positional", + array: ["", ""], + number: 42, + positional: "", }, }, }; @@ -260,11 +279,17 @@ describe("metrics", () => { isFirstUsage: false, isCI: false, isNonInteractive: false, - argsUsed: ["positional", "xgradualrollouts", "xversions"], - argsCombination: "positional, xgradualrollouts, xversions", + argsUsed: [ + "array", + "number", + "positional", + "xgradualrollouts", + "xversions", + ], + argsCombination: + "array, number, positional, xgradualrollouts, xversions", command: "wrangler command subcommand", args: { - _: ["command", "subcommand"], "experimental-versions": true, "x-versions": true, "experimental-gradual-rollouts": true, @@ -272,8 +297,9 @@ describe("metrics", () => { experimentalGradualRollouts: true, experimentalVersions: true, default: false, - $0: "wrangler", - positional: "positional", + array: ["", ""], + number: 42, + positional: "", }, durationMs: 0, durationSeconds: 0, @@ -313,11 +339,17 @@ describe("metrics", () => { isFirstUsage: false, isCI: false, isNonInteractive: false, - argsUsed: ["positional", "xgradualrollouts", "xversions"], - argsCombination: "positional, xgradualrollouts, xversions", + argsUsed: [ + "array", + "number", + "positional", + "xgradualrollouts", + "xversions", + ], + argsCombination: + "array, number, positional, xgradualrollouts, xversions", command: "wrangler command subcommand", args: { - _: ["command", "subcommand"], "experimental-versions": true, "x-versions": true, "experimental-gradual-rollouts": true, @@ -325,8 +357,9 @@ describe("metrics", () => { experimentalGradualRollouts: true, experimentalVersions: true, default: false, - $0: "wrangler", - positional: "error", + array: ["", ""], + number: 42, + positional: "", }, }, }; @@ -345,11 +378,17 @@ describe("metrics", () => { isFirstUsage: false, isCI: false, isNonInteractive: false, - argsUsed: ["positional", "xgradualrollouts", "xversions"], - argsCombination: "positional, xgradualrollouts, xversions", + argsUsed: [ + "array", + "number", + "positional", + "xgradualrollouts", + "xversions", + ], + argsCombination: + "array, number, positional, xgradualrollouts, xversions", command: "wrangler command subcommand", args: { - _: ["command", "subcommand"], "experimental-versions": true, "x-versions": true, "experimental-gradual-rollouts": true, @@ -357,8 +396,9 @@ describe("metrics", () => { experimentalGradualRollouts: true, experimentalVersions: true, default: false, - $0: "wrangler", - positional: "error", + array: ["", ""], + number: 42, + positional: "", }, durationMs: 0, durationSeconds: 0, @@ -467,6 +507,29 @@ describe("metrics", () => { }); }); }); + + describe("redactArgValues()", () => { + it("should redact sensitive values", () => { + const args = { + default: false, + array: ["beep", "boop"], + secretArray: ["beep", "boop"], + number: 42, + string: "secret", + secretString: "secret", + }; + + const redacted = redactArgValues(args, ["string", "array"]); + expect(redacted).toMatchObject({ + default: false, + array: ["beep", "boop"], + secretArray: ["", ""], + number: 42, + string: "secret", + secretString: "", + }); + }); + }); }); describe("getMetricsConfig()", () => { diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index 409cf1afb55a..90ff757e2c2f 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -32,6 +32,13 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { const isNonInteractive = !isInteractive(); const amplitude_session_id = Date.now(); let amplitude_event_id = 0; + /** We redact strings in arg values, unless they are named here */ + const allowList = { + // applies to all commands + "*": ["format", "log-level"], + // specific commands + tail: ["status"], + }; return { /** @@ -87,7 +94,9 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { argsUsed, argsCombination, }; - + // we redact all args unless they are in the allowList + const allowedKeys = getAllowedKeys(allowList, properties.command ?? ""); + properties.args = redactArgValues(properties.args ?? {}, allowedKeys); await dispatch({ name, properties: { @@ -176,9 +185,9 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { export type Properties = Record; +const exclude = new Set(["$0", "_"]); /** just some pretty naive cleaning so we don't send "experimental-versions", "experimentalVersions", "x-versions" and "xVersions" etc. */ const normaliseArgs = (argsWithValues: Record) => { - const exclude = new Set(["$0", "_"]); const result: string[] = []; const args = Object.keys(argsWithValues); for (const arg of args) { @@ -202,3 +211,39 @@ const normaliseArgs = (argsWithValues: Record) => { } return result; }; + +const getAllowedKeys = ( + allowList: Record & { "*": string[] }, + key: string +) => { + const commandSpecific = allowList[key] ?? []; + return [...commandSpecific, ...allowList["*"]]; +}; +export const redactArgValues = ( + args: Record, + allowedKeys: string[] +) => { + const result: Record = {}; + + for (const [key, value] of Object.entries(args)) { + if (exclude.has(key)) { + continue; + } + if ( + typeof value === "number" || + typeof value === "boolean" || + allowedKeys.includes(key) + ) { + result[key] = value; + } else if (typeof value === "string") { + result[key] = ""; + } else if (Array.isArray(value)) { + result[key] = value.map((v) => + typeof v === "string" ? "" : v + ); + } else { + result[key] = value; + } + } + return result; +}; From 3ee8f0976784bacdce6c4db3d1015923c204b382 Mon Sep 17 00:00:00 2001 From: emily-shen <69125074+emily-shen@users.noreply.github.com> Date: Thu, 21 Nov 2024 21:03:18 +0000 Subject: [PATCH 5/6] lint --- packages/wrangler/src/metrics/metrics-config.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/wrangler/src/metrics/metrics-config.ts b/packages/wrangler/src/metrics/metrics-config.ts index ed131146626e..2b1abb22ab14 100644 --- a/packages/wrangler/src/metrics/metrics-config.ts +++ b/packages/wrangler/src/metrics/metrics-config.ts @@ -3,7 +3,6 @@ import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; import path from "node:path"; import { getWranglerSendMetricsFromEnv } from "../environment-variables/misc-variables"; import { getGlobalWranglerConfigPath } from "../global-wrangler-config-path"; -import { isNonInteractiveOrCI } from "../is-interactive"; import { logger } from "../logger"; /** From a1950bd704a3090fd691d4b4ac7000c1e2cffa32 Mon Sep 17 00:00:00 2001 From: emily-shen <69125074+emily-shen@users.noreply.github.com> Date: Fri, 22 Nov 2024 08:51:36 +0000 Subject: [PATCH 6/6] isNonInteractive -> isInteractive --- packages/wrangler/src/__tests__/metrics.test.ts | 10 +++++----- packages/wrangler/src/metrics/metrics-dispatcher.ts | 6 +++--- packages/wrangler/src/metrics/send-event.ts | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/wrangler/src/__tests__/metrics.test.ts b/packages/wrangler/src/__tests__/metrics.test.ts index edf4f5ab3c9c..f478a67ec844 100644 --- a/packages/wrangler/src/__tests__/metrics.test.ts +++ b/packages/wrangler/src/__tests__/metrics.test.ts @@ -240,7 +240,7 @@ describe("metrics", () => { wranglerVersion: "1.2.3", isFirstUsage: false, isCI: false, - isNonInteractive: false, + isInteractive: true, argsUsed: [ "array", "number", @@ -278,7 +278,7 @@ describe("metrics", () => { wranglerVersion: "1.2.3", isFirstUsage: false, isCI: false, - isNonInteractive: false, + isInteractive: true, argsUsed: [ "array", "number", @@ -338,7 +338,7 @@ describe("metrics", () => { wranglerVersion: "1.2.3", isFirstUsage: false, isCI: false, - isNonInteractive: false, + isInteractive: true, argsUsed: [ "array", "number", @@ -377,7 +377,7 @@ describe("metrics", () => { wranglerVersion: "1.2.3", isFirstUsage: false, isCI: false, - isNonInteractive: false, + isInteractive: true, argsUsed: [ "array", "number", @@ -429,7 +429,7 @@ describe("metrics", () => { await runWrangler("command subcommand positional"); expect(requests.count).toBe(2); - expect(std.debug).toContain('isNonInteractive":true'); + expect(std.debug).toContain('"isInteractive":false,'); }); describe("banner", () => { diff --git a/packages/wrangler/src/metrics/metrics-dispatcher.ts b/packages/wrangler/src/metrics/metrics-dispatcher.ts index 90ff757e2c2f..d44b28500439 100644 --- a/packages/wrangler/src/metrics/metrics-dispatcher.ts +++ b/packages/wrangler/src/metrics/metrics-dispatcher.ts @@ -1,6 +1,6 @@ import chalk from "chalk"; import { fetch } from "undici"; -import isInteractive from "../is-interactive"; +import _isInteractive from "../is-interactive"; import { logger } from "../logger"; import { CI } from "./../is-ci"; import { @@ -29,7 +29,7 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { const packageManager = getPackageManager(); const isFirstUsage = readMetricsConfig().permission === undefined; const isCI = CI.isCI(); - const isNonInteractive = !isInteractive(); + const isInteractive = _isInteractive(); const amplitude_session_id = Date.now(); let amplitude_event_id = 0; /** We redact strings in arg values, unless they are named here */ @@ -90,7 +90,7 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { packageManager, isFirstUsage, isCI, - isNonInteractive, + isInteractive, argsUsed, argsCombination, }; diff --git a/packages/wrangler/src/metrics/send-event.ts b/packages/wrangler/src/metrics/send-event.ts index 74b6ba6bda94..8f2d56b17d21 100644 --- a/packages/wrangler/src/metrics/send-event.ts +++ b/packages/wrangler/src/metrics/send-event.ts @@ -141,7 +141,7 @@ export type CommonEventProperties = { amplitude_event_id: number; isCI: boolean; - isNonInteractive: boolean; + isInteractive: boolean; argsUsed: string[]; argsCombination: string; };