fix(telemetry): report hosting type and a single install id - #5676
Conversation
|
🚓 Slop Cop ✅ Nothing worth flagging. The diff looks consistent with the PR's stated intent and the code-style rules. The diff matches the stated intent (telemetry hosting-type/install-id fixes plus the disclosed install-handoff change), is small and coherent, and shows no integrity red flags or code-style rule violations in the added lines. Automated, non-blocking heads-up from an LLM. It can be wrong — use your judgment. Regenerates on every push. |
Self-hosted (server) projects were effectively untrackable, and CLI events
could not be joined to admin events.
- Admin events now send `installation_id`, the name the CLI already uses. They
previously sent the same value as `project_id`, so nothing joined the two
surfaces. PostHog queries on `project_id` need updating.
- Every CLI and admin event carries `hostingType` ("aws" or "server"), read
from the marker both CLI bins already set.
- `cli-create-webiny-project-end` and `-error` carry `hostingType` too. The
`-start` event does not, because it fires before the user picks one.
- New `cli-project-serve-start` (plus error variants) for self-hosted projects,
which have no deploy command to report.
- The `newUser` flag now flips for self-hosted projects, after a successful
admin build. It only hung off `AdminAfterDeploy`, which self-hosted never
reaches, so those users were reported as new users forever.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`serve` is a convenience for zero-infra self-hosting, not the production path. The self-hosted deploy artifact ships `build/start.mjs`, so the common route is to build the api and admin apps, copy `build/` into a container and run `node start.mjs`, which never touches the CLI. Counting `serve` would measure the dev convenience and read as low adoption whenever people are in Docker. What covers the funnel instead: `admin-app-start` and the install-wizard events now carry `hostingType` and `installation_id`, both baked into the admin bundle at `webiny build admin` time, so they report in the container path too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eads Matches `cli.js`, which already declares each optional property group and fills it with a plain `if`. Per ai-context/code-style/no-inline-conditional-spreads.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
525ecd4 to
078d850
Compare
The "Start using Webiny" CTA routes through webiny.com/install/finish so the site's anonymous wts_did cookie can be aliased to the deployer's machine id. It only did that when the admin was served from a `.cloudfront.net` host, so self-hosted installs were never aliased and install-to-marketing attribution was AWS-only. The website drops its matching CloudFront check on `return_to`, so both halves of the handoff work for any host. Sequencing: the website change has to be live first. Until it is, a non CloudFront admin sends the user to /install/finish and the page answers "Invalid request" instead of redirecting back, which dead-ends the install wizard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesTelemetry installation lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CreateWebinyProject
participant telemetry_cli
participant ProjectBuildHooks
participant telemetry_react
participant ProjectServer
CreateWebinyProject->>telemetry_cli: send hosting-aware project event
ProjectBuildHooks->>telemetry_react: provide REACT_APP_WEBINY_HOSTING_TYPE
telemetry_react->>telemetry_react: resolve installationId and emit admin event fields
ProjectServer->>ProjectServer: set global newUser to false after admin build
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Why
Before we promote the self-hosted (server) hosting type, its telemetry has to be trustworthy. Right now it isn't:
installation_id(packages/telemetry/cli.js), while the admin app sends the identical value asproject_id(packages/telemetry/react.js). Filteringadmin-app-startbyinstallation_idreturns nothing, and no funnel crosses the two surfaces.newUserflag is cleared byTelemetryNoLongerNewUser, anAdminAfterDeployhook registered only inproject-aws. Self-hosted projects never deploy, so the flag staystruefor the life of that machine's~/.webiny/configand skews everynewUsersegment.hostingType("aws" or "server") is known at scaffold time and at every CLI invocation, but no event carries it.cli-create-webiny-project-start/-endpassed no properties at all, so self-hosted project creations weren't countable.What changed
installation_idinstead ofproject_idpackages/telemetry/react.jshostingTypeon every CLI event, from the existingWEBINY_HOSTING_TYPEmarkerpackages/telemetry/cli.jshostingTypeon every admin event, mirrored into the bundle at build/watch timepackages/telemetry/react.js,packages/project/src/extensions/Project/SetAdminAppEnvVars*.tshostingTypeoncli-create-webiny-project-endand-errorpackages/create-webiny-project/src/features/CreateWebinyProject.tsnewUserflips on the self-hosted path, after a successful admin buildpackages/project-server/src/extensions/ProjectServer/TelemetryNoLongerNewUser.tspackages/app-admin/src/presentation/installation/components/SystemInstaller/steps/FinishSetup/handleStartUsing.tsThe localStorage key behind the install id stays
wts_project_id. Renaming it would mint a fresh id for every admin session that already has one.Decisions worth reviewing
cli-create-webiny-project-startdeliberately has nohostingType. It fires before the hosting-type prompt runs. Sending the placeholder default would inflate "aws", so-startstays the funnel denominator and-end/-errorcarry the choice. If the run fails before the prompt,-errorreportshostingType: "unknown"rather than guessing.newUserflips on admin build, not serve. A successful admin build is the closest self-hosted analogue of the AWS milestone: same app, point at which the user has something working. It's also the one step every self-hosted path runs, including the container path below.No CLI-side "it's running" event for self-hosted. An earlier revision of this PR added
cli-project-serve-start. Dropped in 5e60851:serveis a convenience for zero-infra self-hosting, not the production path. The deploy artifact shipsbuild/start.mjs, so the common route is to build both apps, copybuild/into a container and runnode start.mjs, which never touches the CLI. That event would have measured the dev convenience and read as low adoption whenever people are in Docker.admin-app-startand the install-wizard events cover the same question and do fire in the container path, sincehostingTypeandinstallation_idare baked into the admin bundle at build time.If we do want a CLI-side signal later,
buildis the honest place for it: every self-hosted path runs it. It's hosting-agnostic though, so it would change AWS event volume too and would need care not to fire on every watch rebuild. Worth deciding separately.Follow-ups, not in this PR
Resolved during review:
deploymentType: "standalone"was a false alarm. It doesn't exist anywhere in webiny-js (git log --all -S "deploymentType"returns nothing), nor on wts-server'sv3branch, and self-hosted can't emitcli-project-deploy-*at all, since that decorator lives in cli-aws andwebiny-serverhas no deploy command. Sven traced it: PostHog's AI had pickedallow-local-state-files: 1as the filter for "standalone", so the deploy events under it were never self-hosted events.project_idtoinstallation_idrename is safe end to end. Checked all three consumers rather than assuming: wts-server (v3) spreads...event.propertiesthrough verbatim inposthog.ts:55, with no allowlist and no property remapping, so the value simply arrives under the new name; the website's/install/finishpage reads only themachine_idquery param and thewts_didcookie; and per Sven, nothing in PostHog uses either property today. No dashboard migration needed.cli-create-webiny-project-*keeps going out without aninstallation_id. Worth knowing why it's absent rather than empty: the CLI reads it from<cwd>/package.json, and during scaffolding the cwd is still the parent directory.Not in scope, and worth stating because the name is misleading: the WCP telemetry client that
WcpInjectTelemetryClientAfterBuildinjects into the api handler is a different system from the one in this PR. It reports to the WCP API (app.webiny.com) so customers can see their own API call counts. It is usage metering, not product analytics, and it never touchest.webiny.comor PostHog.Now covered, with a release-order constraint:
Self-hosted installs can be aliased to the website session. The "Start using Webiny" CTA routes through
webiny.com/install/finishso the site's anonymouswts_didcookie can be aliased to the deployer's machine id, which is what links "read the docs" to "installed Webiny". It only ran when the admin was served from a.cloudfront.nethost, so self-hosted installs were never aliased. That check is gone, and the website drops its matching restriction onreturn_to, so both halves of the handoff work for any host./install/finishand the page answers "Invalid request" instead of redirecting back, dead-ending the install wizard. Everything else in this PR is independent of that, so if the website isn't ready, drop commit3ab9a3a4f1and merge the rest.The website-side restriction exists to stop
/install/finish?return_to=https://evil.comturning webiny.com into a phishing redirector, and a self-hosted admin can live on any hostname, so there's no allowlist pattern that covers it. Worth being explicit about what removing it accepts: unvalidated origins can be redirected to, and anyone can POST arbitrarymachine_idvalues and pollute the alias graph. The second was already true for anything CloudFront-shaped.Test plan
yarn build(full monorepo) passes.yarn adio,yarn lint,yarn format,yarn webiny sync-dependenciesall clean.@webiny/telemetry. Verifying the emitted payloads means creating a project on each hosting type and watching the requests tot.webiny.com, which I haven't done yet.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes