Separate telemetry handling between CLI and the app#2780
Separate telemetry handling between CLI and the app#2780fredrikekelund merged 54 commits intotrunkfrom
Conversation
Add a separate Vite config (vite.config.npm.ts) that externalizes all runtime dependencies and adds a Node.js shebang, producing a standalone CLI package publishable to npm as @automattic/studio-cli. The existing Electron-bundled build (vite.config.ts) is unchanged.
- Remove unused `main` field (CLI-only package) - Include source maps in `files` whitelist - Use `npx --no-install` for deterministic postinstall
Reverts the inline patch-package approach which breaks in CI (patch-package errors on missing packages in workspace context). The script detects monorepo workspaces and skips patching there.
The previous check for node_modules/ existence was unreliable because npm can create the directory with hoisted leftovers even in workspaces. Instead, check if the actual patched packages exist locally.
…ne-cli-telemetry
📊 Performance Test ResultsComparing 7b0a05a vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
| lastBumpStats: z | ||
| .record( z.string(), z.partialRecord( z.enum( StatsMetric ), z.number() ) ) | ||
| .optional(), | ||
| lastBumpStats: z.record( z.string(), z.record( z.string(), z.number() ) ).optional(), |
There was a problem hiding this comment.
I previously advocated for the stricter schema here (#2394 (review)), but it actually causes more trouble than it fixes, given the other changes in this PR. Strictness in runtime types makes sense (i.e., for the function arguments to `bumpStat), but strictness in the appdata schema introduces the risk of runtime exceptions if the config file contains unknown bump stats. That isn't really needed, since we only use this object to check when we last bumped a particular stat.
Anyway, with this change, it's safe to land this PR to trunk even before the config file PR train (#2807 etc) has landed.
bcotrim
left a comment
There was a problem hiding this comment.
Confirmed and works as expected!
Nice separation LGTM 👍
apps/cli/lib/appdata.ts
Outdated
| import { readFile, writeFile } from 'atomically'; | ||
| import { z } from 'zod'; | ||
| import { validateAccessToken } from 'cli/lib/api'; | ||
| import { StatsMetric } from 'cli/lib/types/bump-stats'; |
There was a problem hiding this comment.
Unused import here, it should be safe to remove.
Related issues
How AI was used in this PR
For executing smaller subtasks of the refactor and for planning parts of it.
Proposed Changes
This PR makes a few updates to the telemetry handling in the CLI, in anticipation of distributing the CLI through npm:
StatsGroupandStatsMetricdefinitions between the CLI and the app so that they're not shared.bumpStatandbumpAggregatedUniqueStatlogic.apps/cliandapps/studio.ConfigFileProviderinterface intools/common/lib/bump-stat.tsto prepare for the separation of CLI and app config files (happening in Move CLI site data to dedicated config file #2731 and related PRs).studio-cli-weekly-unq-npmandstudio-cli-weekly-unq-app. These two stats report usage per platform.__ENABLE_CLI_TELEMETRY__constant definition that makes it so the regularnpm run cli:buildoutput disables telemetry altogether. Onlynpm run cli:packageandnpm run cli:build:prodwill enable telemetry.Testing Instructions
npm run cli:build:prodnode apps/cli/dist/cli/main.js site listWould have bumped stat: studio-cli-weekly-unq-app=darwinstudio-cli-weekly-unq-appkey inlastBumpStatsPre-merge Checklist