feat: CLI telemetry #220
feat: CLI telemetry #220
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Mixpanel-based telemetry subsystem to the CLI, instruments command lifecycles/errors, introduces container/CI/machine-id detection utilities, injects a telemetry token at build time, updates CLI package dependencies, and exposes workflow env vars to control telemetry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job Comprehensive Code ReviewTodo List:
Overall AssessmentThis PR adds a well-structured telemetry system to the ZenStack CLI. The implementation shows good attention to privacy concerns and follows best practices for opt-out mechanisms. Security & Privacy ✅Strengths:
Minor Concerns:
Code Quality & Architecture ✅Excellent Design Patterns:
Code Organization: // Good abstraction - packages/cli/src/telemetry.ts:119-125
async trackCommand(command: string, action: () => Promise<void> | void) {
await this.trackSpan('cli:command:start', 'cli:command:complete', 'cli:command:error', { command }, action);
}Environment Detection Utilities ✅Robust Implementation:
Minor Improvements Needed:
CLI Integration ✅Well-Integrated:
Performance Considerations ✅Efficient Implementation:
Good Practice: // Gives telemetry time to send before exit - packages/cli/src/index.ts:154-160
if (telemetry.isTracking) {
setTimeout(() => {
process.exit(exitCode);
}, 200);
}Build System Integration ✅Smart Token Replacement:
Issues & Recommendations🟡 Minor Issues
🟢 Suggestions
Test Coverage Needs
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds telemetry functionality to the CLI package, enabling tracking of CLI usage and events for analytics purposes. The implementation includes platform detection, machine identification, and Mixpanel integration for event tracking.
- Introduces a comprehensive telemetry system with event tracking for CLI commands and errors
- Adds utility modules for detecting runtime environments (Docker, WSL, CI, containers)
- Implements machine ID generation for user anonymization
- Configures build-time token replacement for telemetry tracking
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/tsup.config.ts | Adds build-time replacement of telemetry tracking token in compiled output |
| packages/cli/src/utils/machine-id-utils.ts | Implements cross-platform machine ID generation for user identification |
| packages/cli/src/utils/is-wsl.ts | Detects Windows Subsystem for Linux environment |
| packages/cli/src/utils/is-docker.ts | Detects Docker container execution environment |
| packages/cli/src/utils/is-container.ts | Detects general container environments (Docker/Podman) |
| packages/cli/src/utils/is-ci.ts | Detects continuous integration environment |
| packages/cli/src/telemetry.ts | Core telemetry class with Mixpanel integration and event tracking |
| packages/cli/src/index.ts | Integrates telemetry tracking into CLI command execution flow |
| packages/cli/src/constants.ts | Defines placeholder constant for telemetry token replacement |
| packages/cli/package.json | Adds mixpanel dependency for telemetry functionality |
| .github/workflows/build-test.yml | Configures environment variables for telemetry in CI |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job Comprehensive Code ReviewTodo List:
Overall AssessmentThis PR adds a well-structured telemetry system to the ZenStack CLI. The implementation shows good attention to privacy concerns and follows best practices for opt-out mechanisms. Security & Privacy ✅Strengths:
Minor Concerns:
Code Quality & Architecture ✅Excellent Design Patterns:
Code Organization: // Good abstraction - packages/cli/src/telemetry.ts:119-125
async trackCommand(command: string, action: () => Promise<void> | void) {
await this.trackSpan('cli:command:start', 'cli:command:complete', 'cli:command:error', { command }, action);
}Environment Detection Utilities ✅Robust Implementation:
Minor Improvements Needed:
CLI Integration ✅Well-Integrated:
Performance Considerations ✅Efficient Implementation:
Good Practice: // Gives telemetry time to send before exit - packages/cli/src/index.ts:154-160
if (telemetry.isTracking) {
setTimeout(() => {
process.exit(exitCode);
}, 200);
}Build System Integration ✅Smart Token Replacement:
Issues & Recommendations🟡 Minor Issues
🟢 Suggestions
Test Coverage Needs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
packages/cli/package.json (1)
38-38: Mixpanel dep: confirm ESM/CJS interop and lazy-load to avoid CLI cold-start cost
- Ensure tsconfig/esbuild options allow CJS default import (esModuleInterop/allowSyntheticDefaultImports) or use dynamic import with named default to avoid type/runtime issues.
- Prefer lazy-loading Mixpanel only when telemetry is enabled and a token exists to keep startup snappy.
Example (outside this file, in telemetry init):
// ESM-friendly lazy import async function getMixpanel() { const { default: Mixpanel } = await import('mixpanel'); return Mixpanel; } // ... if (token && !doNotTrack) { const Mixpanel = await getMixpanel(); const client = Mixpanel.init(token); // ... }packages/cli/src/constants.ts (1)
1-2: Guard against placeholder being treated as a real tokenIf build-time replacement doesn’t run, the placeholder may slip through. Ensure telemetry code treats the placeholder as “no token” before initializing Mixpanel.
Option (outside this file, in telemetry init):
const PLACEHOLDER = '<TELEMETRY_TRACKING_TOKEN>'; const token = TELEMETRY_TRACKING_TOKEN; const hasRealToken = token && token !== PLACEHOLDER; if (!hasRealToken || process.env.DO_NOT_TRACK) return /* no-op */;packages/cli/src/utils/is-ci.ts (1)
1-5: Broaden CI detection and normalize CI flag valueCurrent logic misses providers like Azure Pipelines (TF_BUILD), TeamCity, Jenkins, Bitbucket, etc., and treats "False"/" FALSE " as truthy. Normalize value and include common vendor signals, or use a well-known library (is-ci/ci-info).
Apply:
-import { env } from 'node:process'; -export const isInCi = - env['CI'] !== '0' && - env['CI'] !== 'false' && - ('CI' in env || 'CONTINUOUS_INTEGRATION' in env || Object.keys(env).some((key) => key.startsWith('CI_'))); +import { env } from 'node:process'; +const ciVal = String(env.CI ?? '').trim().toLowerCase(); +export const isInCi = + ciVal !== '0' && ciVal !== 'false' && + ( + 'CI' in env || 'CONTINUOUS_INTEGRATION' in env || + 'GITHUB_ACTIONS' in env || 'GITLAB_CI' in env || 'BUILDKITE' in env || + 'DRONE' in env || 'TEAMCITY_VERSION' in env || 'TF_BUILD' in env || + 'JENKINS_URL' in env || 'BITBUCKET_BUILD_NUMBER' in env || + Object.keys(env).some((k) => k.startsWith('CI_')) + );Alternative: depend on "is-ci" for fewer edge cases.
packages/cli/src/utils/is-wsl.ts (1)
4-18: Avoid repeated sync FS reads; memoize result and consider container-on-WSL edgeSync read of /proc/version is fine once, but memoizing prevents repeated blocking calls. Also be aware Docker-on-WSL can include “microsoft” in /proc/version; if you also detect containers, ensure precedence in telemetry context.
Apply:
-export const isWsl = () => { +let _isWsl: boolean | undefined; +export const isWsl = () => { + if (_isWsl !== undefined) return _isWsl; if (process.platform !== 'linux') { - return false; + return (_isWsl = false); } if (os.release().toLowerCase().includes('microsoft')) { - return true; + return (_isWsl = true); } try { - return fs.readFileSync('/proc/version', 'utf8').toLowerCase().includes('microsoft'); + return (_isWsl = fs.readFileSync('/proc/version', 'utf8').toLowerCase().includes('microsoft')); } catch { - return false; + return (_isWsl = false); } };packages/cli/src/utils/is-docker.ts (2)
24-31: Return a guaranteed boolean and simplify cachingEnsure the exported API is
boolean, notboolean | undefined, and compute+return in one expression.Apply:
-export default function isDocker() { - // TODO: Use `??=` when targeting Node.js 16. - if (isDockerCached === undefined) { - isDockerCached = hasDockerEnv() || hasDockerCGroup(); - } - - return isDockerCached; -} +export default function isDocker(): boolean { + // TODO: Use `??=` when targeting Node.js 16. + const result = isDockerCached ?? (isDockerCached = hasDockerEnv() || hasDockerCGroup()); + return result; +}
16-22: Broaden cgroup detection (covers containerd/LXC and cgroup v2/mountinfo)Modern environments may not include plain "docker" in
/proc/self/cgroup. Expand checks to improve detection fidelity.-function hasDockerCGroup() { - try { - return fs.readFileSync('/proc/self/cgroup', 'utf8').includes('docker'); - } catch { - return false; - } -} +function hasDockerCGroup() { + const files = ['/proc/self/cgroup', '/proc/1/cgroup', '/proc/self/mountinfo']; + const markers = ['docker', 'containerd', 'lxc']; + for (const f of files) { + try { + const data = fs.readFileSync(f, 'utf8'); + if (markers.some((m) => data.includes(m))) return true; + } catch { + // ignore per-file errors + } + } + return false; +}packages/cli/src/utils/machine-id-utils.ts (3)
21-27: Prefer'in'overhasOwnPropertyonprocess.envAvoids prototype pitfalls and the eslint disable.
- // eslint-disable-next-line no-prototype-builtins - if (process.arch === 'ia32' && process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432')) { + if (process.arch === 'ia32' && ('PROCESSOR_ARCHITEW6432' in process.env)) { return 'mixed'; }
41-47: Make Windows REG parsing locale-agnosticRelying on the literal
REG_SZcan break on localized output; use a regex to capture the value after the type token.- case 'win32': - return result - .toString() - .split('REG_SZ')[1] - ?.replace(/\r+|\n+|\s+/gi, '') - .toLowerCase(); + case 'win32': { + const m = /REG_SZ\s+([^\r\n]+)/i.exec(result.toString()); + return m?.[1]?.trim().toLowerCase(); + }
62-76: Memoize and type the result; avoid repeated blocking execsThis function can be called multiple times; cache the hashed ID and declare an explicit return type.
+let cachedId: string | undefined; -export function getMachineId() { +export function getMachineId(): string { - if (!(platform in guid)) { - return randomUUID(); - } + if (cachedId) return cachedId; + if (!(platform in guid)) { + return (cachedId = randomUUID()); + } try { const value = execSync(guid[platform as keyof typeof guid]); const id = expose(value.toString()); if (!id) { - return randomUUID(); + return (cachedId = randomUUID()); } - return hash(id); + return (cachedId = hash(id)); } catch { - return randomUUID(); + return (cachedId = randomUUID()); } }packages/cli/src/utils/is-container.ts (2)
7-14: Minor: useexistsSyncfor simple presence checkSlightly simpler and avoids exceptions for the expected “not present” case.
-const hasContainerEnv = () => { - try { - fs.statSync('/run/.containerenv'); - return true; - } catch { - return false; - } -}; +const hasContainerEnv = () => fs.existsSync('/run/.containerenv');
16-23: Return a strict boolean and simplify cachingAvoid leaking
boolean | undefinedand compute+return in one expression.-export function isInContainer() { - // TODO: Use `??=` when targeting Node.js 16. - if (cachedResult === undefined) { - cachedResult = hasContainerEnv() || isDocker(); - } - - return cachedResult; -} +export function isInContainer(): boolean { + // TODO: Use `??=` when targeting Node.js 16. + const result = cachedResult ?? (cachedResult = hasContainerEnv() || isDocker()); + return result; +}packages/cli/src/telemetry.ts (4)
31-45: Defer expensive environment collection until tracking is enabledField initializers call getMachineId/OS/containers detection even when telemetry is disabled, slowing CLI startup. Compute these lazily only when mixpanel exists (e.g., inside track) or cache on first use.
I can provide a minimal lazy-base-props refactor if you want.
58-81: Send Mixpanel time as unix seconds (number), not Date objectMixpanel’s time is expected as a unix timestamp (seconds). Also consider dropping $os unless you specifically rely on it.
- time: new Date(), + time: Math.floor(Date.now() / 1000),
90-117: Flush events for short-lived CLI processesWithout flush, events can be dropped when the process exits quickly. Flush after emitting the complete event.
} finally { this.track(completeEvent, { duration: Date.now() - start, success, ...properties, }); + if (this.mixpanel && typeof (this.mixpanel as any).flush === 'function') { + await new Promise<void>((resolve) => (this.mixpanel as any).flush(resolve)); + } }
139-139: Consider lazy singleton to reduce startup overheadInstantiating Telemetry at import time triggers environment checks eagerly. Export a getter that constructs on first use.
I can provide a small helper: getTelemetry() that memoizes the instance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/build-test.yml(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/constants.ts(1 hunks)packages/cli/src/index.ts(2 hunks)packages/cli/src/telemetry.ts(1 hunks)packages/cli/src/utils/is-ci.ts(1 hunks)packages/cli/src/utils/is-container.ts(1 hunks)packages/cli/src/utils/is-docker.ts(1 hunks)packages/cli/src/utils/is-wsl.ts(1 hunks)packages/cli/src/utils/machine-id-utils.ts(1 hunks)packages/cli/tsup.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/cli/package.jsonpackages/cli/src/constants.tspackages/cli/src/utils/is-wsl.tspackages/cli/src/utils/is-ci.tspackages/cli/src/index.tspackages/cli/src/utils/is-container.tspackages/cli/src/utils/is-docker.tspackages/cli/src/telemetry.tspackages/cli/tsup.config.tspackages/cli/src/utils/machine-id-utils.ts
🧬 Code graph analysis (4)
packages/cli/src/index.ts (1)
packages/cli/src/telemetry.ts (1)
telemetry(139-139)
packages/cli/src/utils/is-container.ts (1)
packages/cli/src/utils/is-docker.ts (1)
isDocker(24-31)
packages/cli/src/telemetry.ts (7)
packages/cli/src/utils/machine-id-utils.ts (1)
getMachineId(62-76)packages/cli/src/utils/version-utils.ts (1)
getVersion(5-13)packages/cli/src/utils/is-docker.ts (1)
isDocker(24-31)packages/cli/src/utils/is-wsl.ts (1)
isWsl(4-18)packages/cli/src/utils/is-container.ts (1)
isInContainer(16-23)packages/cli/src/utils/is-ci.ts (1)
isInCi(2-5)packages/cli/src/constants.ts (1)
TELEMETRY_TRACKING_TOKEN(2-2)
packages/cli/src/utils/machine-id-utils.ts (1)
packages/cli/src/utils/exec-utils.ts (1)
execSync(6-15)
🪛 GitHub Actions: Build and Test
packages/cli/src/telemetry.ts
[error] 1-1: TS2307: Cannot find module '@paralleldrive/cuid2' or its corresponding type declarations. (src/telemetry.ts:1,26).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (5)
packages/cli/src/index.ts (3)
9-10: Nice: command handlers wrapped with telemetryClean, consistent use of
telemetry.trackCommandfor all commands/subcommands.Also applies to: 12-18, 21-26, 29-30
163-163: No action required: entry file not imported by any module
Verified that packages/cli/src/index.ts isn’t imported elsewhere in the repository, so invokingmain()here only affects the CLI entry point.
32-33: Removal ofcreateProgramexport is non-breaking
NoexportofcreateProgramexists in packages/cli/src/index.ts, and no import or require of it was found across the repo. Safe to remove.packages/cli/src/telemetry.ts (2)
83-88: Avoid double-counting errors when using trackSpanIf callers use trackSpan and also call trackError on the same error, you’ll emit two error events. Clarify usage or guard inside trackError.
I can add a small guard (e.g., a symbol flag on Error) if desired.
49-50: Retaingeolocate: true– the Mixpanel Node.js SDK (since v0.16.0) supports ageolocateinit option to enable automatic IP-based geolocation. Only remove or override it if you intend to disable geolocation (e.g. by settinggeolocate: false). (github.com, docs.mixpanel.com)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/actions/generate.ts (1)
116-119: Don’t swallow plugin failures—success message can be misleading.Errors are logged but execution continues, so the top-level message still says “completed successfully.” Propagate failure state or throw so callers can surface an accurate status.
Sketch:
// track failures let hadFailure = false; try { await cliPlugin.generate(...); spinner?.succeed(); } catch (err) { hadFailure = true; spinner?.fail(); console.error(err); } // after loop: if (hadFailure) throw new CliError('One or more plugins failed. See logs above.');Then only print the success banner when no error was thrown.
🧹 Nitpick comments (2)
packages/cli/src/actions/generate.ts (2)
42-45: Polish the docs line: clearer wording, highlight, and avoid hardcoded string in place.
- Minor copy: “Docs:” reads better than “Check documentation:”.
- Highlight link for visibility.
- Consider centralizing the URL in a constant (and ideally deriving the major from package version) to prevent drift across files.
-``` - -Check documentation: https://zenstack.dev/docs/3.x`); +``` + +${colors.cyan('Docs: https://zenstack.dev/docs/3.x')}`);Optionally, move the URL to a constant:
// packages/cli/src/constants.ts export const DOCS_URL = 'https://zenstack.dev/docs/3.x';and use:
${colors.cyan(`Docs: ${DOCS_URL}`)}
37-37: Import path in example may be invalid on Windows or when outputPath is absolute.If outputPath is absolute, the import specifier won’t be a valid module path; Windows backslashes also break ESM imports. Suggest normalizing to a relative, posix-style path before interpolation.
Example:
import path from 'node:path'; const rel = path.isAbsolute(outputPath) ? path.relative(process.cwd(), outputPath) : outputPath; const importPath = (rel.startsWith('.') ? rel : `./${rel}`).split(path.sep).join('/'); // then: import { schema } from '${importPath}/schema';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/cli/package.json(1 hunks)packages/cli/src/actions/generate.ts(1 hunks)packages/cli/src/utils/is-container.ts(1 hunks)packages/cli/src/utils/is-wsl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/utils/is-container.ts
- packages/cli/src/utils/is-wsl.ts
- packages/cli/package.json
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/cli/src/actions/generate.ts
🧠 Learnings (2)
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Schema is used to instantiate `ZenStackClient` with type-safe CRUD operations
Applied to files:
packages/cli/src/actions/generate.ts
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: `zenstack generate` compiles ZModel to TypeScript schema (`schema.ts`)
Applied to files:
packages/cli/src/actions/generate.ts
|
Claude finished @ymc9's task —— View job Comprehensive Code ReviewTodo List:
Overall AssessmentThis PR adds a well-structured telemetry system to the ZenStack CLI. The implementation shows good attention to privacy concerns and follows best practices for opt-out mechanisms. Security & Privacy ✅Strengths:
Minor Concerns:
Code Quality & Architecture ✅Excellent Design Patterns:
Code Organization: // Good abstraction - packages/cli/src/telemetry.ts:119-125
async trackCommand(command: string, action: () => Promise<void> | void) {
await this.trackSpan('cli:command:start', 'cli:command:complete', 'cli:command:error', { command }, action);
}Environment Detection Utilities ✅Robust Implementation:
Minor Improvements Needed:
CLI Integration ✅Well-Integrated:
Performance Considerations ✅Efficient Implementation:
Good Practice: // Gives telemetry time to send before exit - packages/cli/src/index.ts:154-160
if (telemetry.isTracking) {
setTimeout(() => {
process.exit(exitCode);
}, 200);
}Build System Integration ✅Smart Token Replacement:
Issues & Recommendations🟡 Minor Issues
🟢 Suggestions
Test Coverage Needs
|
|
Claude finished @ymc9's task —— View job Comprehensive Code ReviewTodo List:
Overall AssessmentThis PR adds a well-structured telemetry system to the ZenStack CLI. The implementation shows good attention to privacy concerns and follows best practices for opt-out mechanisms. Security & Privacy ✅Strengths:
Minor Concerns:
Code Quality & Architecture ✅Excellent Design Patterns:
Code Organization: // Good abstraction - packages/cli/src/telemetry.ts:119-125
async trackCommand(command: string, action: () => Promise<void> | void) {
await this.trackSpan('cli:command:start', 'cli:command:complete', 'cli:command:error', { command }, action);
}Environment Detection Utilities ✅Robust Implementation:
Minor Improvements Needed:
CLI Integration ✅Well-Integrated:
Performance Considerations ✅Efficient Implementation:
Good Practice: // Gives telemetry time to send before exit - packages/cli/src/index.ts:154-160
if (telemetry.isTracking) {
setTimeout(() => {
process.exit(exitCode);
}, 200);
}Build System Integration ✅Smart Token Replacement:
Issues & Recommendations🟡 Minor Issues
🟢 Suggestions
Test Coverage Needs
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/cli/src/utils/machine-id-utils.ts (2)
7-10: Avoid hasOwnProperty on process.env; tighten typing for Windows arch detection.Use the in-operator and drop the eslint suppression. Also mark the map as const and return a literal union to keep indexing type-safe.
-const win32RegBinPath = { +const win32RegBinPath = { native: '%windir%\\System32', mixed: '%windir%\\sysnative\\cmd.exe /c %windir%\\System32', -}; +} as const; -function isWindowsProcessMixedOrNativeArchitecture() { - // eslint-disable-next-line no-prototype-builtins - if (process.arch === 'ia32' && process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432')) { +function isWindowsProcessMixedOrNativeArchitecture(): 'native' | 'mixed' { + if (process.arch === 'ia32' && ('PROCESSOR_ARCHITEW6432' in process.env)) { return 'mixed'; } return 'native'; }Also applies to: 21-27
1-2: Add upstream license header (required for compliance).You noted this is modified from automation-stack/node-machine-id. Copy the original license header into this file or add it to third‑party notices and reference it here.
-// modified from https://github.com/automation-stack/node-machine-id +// Modified from https://github.com/automation-stack/node-machine-id +// SPDX-License-Identifier: MIT +// Original work copyright per upstream repository. +// See THIRD_PARTY_NOTICES.md for full license text.packages/cli/src/index.ts (3)
143-145: Minor: simplify closure passed totrackCliApply:
- await telemetry.trackCli(async () => { - await program.parseAsync(); - }); + await telemetry.trackCli(() => program.parseAsync());
136-142: Attach global error handlers before parsing; ensure telemetry sees crashesUnhandled rejections/exceptions won’t be caught by the try/catch. Register handlers before starting parse so telemetry can record them and so we set
exitCode.Apply:
async function main() { let exitCode = 0; + // Attach early so we catch errors outside commander flow + process.on('unhandledRejection', (reason) => { + telemetry.trackError(reason instanceof Error ? reason : new Error(String(reason))); + exitCode = 1; + }); + process.on('uncaughtException', (err) => { + telemetry.trackError(err); + exitCode = 1; + }); + const program = createProgram(); program.exitOverride();
160-167: Don’t callprocess.exit; await a short delay and setprocess.exitCodeUsing
process.exitrisks dropping telemetry; the timeout variant also forcibly kills the process. Prefer awaiting a small delay (or a telemetry flush if available) and let Node exit naturally.Apply:
- if (telemetry.isTracking) { - // give telemetry a chance to send events before exit - setTimeout(() => { - process.exit(exitCode); - }, 200); - } else { - process.exit(exitCode); - } + if (telemetry.isTracking) { + // Optionally: await telemetry.flush?.(200); + await new Promise((r) => setTimeout(r, 200)); + } + process.exitCode = exitCode;
🧹 Nitpick comments (8)
packages/cli/src/utils/machine-id-utils.ts (5)
11-19: Optional: simplify Windows query using 64-bit registry view flag.Using REG’s “/reg:64” removes the need for sysnative/arch juggling. If adopted, you can delete win32RegBinPath and isWindowsProcessMixedOrNativeArchitecture().
- win32: - `${win32RegBinPath[isWindowsProcessMixedOrNativeArchitecture()]}\\REG.exe ` + - 'QUERY HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Cryptography ' + - '/v MachineGuid', + win32: + 'REG.exe QUERY HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Cryptography ' + + '/v MachineGuid ' + + ((process.arch === 'ia32' && ('PROCESSOR_ARCHITEW6432' in process.env)) ? '/reg:64' : ''),
33-60: Trim redundant toString calls; consolidate linux/freebsd branch.expose receives a string; repeated toString() is unnecessary. Linux and FreeBSD branches are identical—combine for brevity.
function expose(result: string): string | undefined { switch (platform) { case 'darwin': return result .split('IOPlatformUUID')[1] ?.split('\n')[0] ?.replace(/=|\s+|"/gi, '') .toLowerCase(); case 'win32': - return result - .toString() + return result .split('REG_SZ')[1] ?.replace(/\r+|\n+|\s+/gi, '') .toLowerCase(); - case 'linux': - return result - .toString() - .replace(/\r+|\n+|\s+/gi, '') - .toLowerCase(); - case 'freebsd': - return result - .toString() - .replace(/\r+|\n+|\s+/gi, '') - .toLowerCase(); + case 'linux': + case 'freebsd': + return result.replace(/\r+|\n+|\s+/gi, '').toLowerCase(); default: throw new Error(`Unsupported platform: ${process.platform}`); } }
67-69: Return strings directly from execSync; pass encoding to avoid Buffer conversions.Avoid multiple toString() calls and ensure consistent UTF‑8 handling.
- const value = execSync(guid[platform as keyof typeof guid]); - const id = expose(value.toString()); + const value = execSync(guid[platform as keyof typeof guid], { encoding: 'utf8' }) as unknown as string; + const id = expose(value);
29-31: Consider product-scoped hashing (HMAC) to reduce cross-app correlation risk.Hashing raw IDs with SHA‑256 enables cross‑product linkage. A stable, product-scoped salt (HMAC) keeps determinism while limiting correlation.
-import { createHash, randomUUID } from 'node:crypto'; +import { createHash, createHmac, randomUUID } from 'node:crypto'; @@ -function hash(guid: string): string { - return createHash('sha256').update(guid).digest('hex'); -} +const TELEMETRY_SCOPE_SALT = process.env.ZENSTACK_TELEMETRY_SALT ?? 'zenstack_cli'; +function hash(guid: string): string { + // Prefer HMAC for scoped, deterministic hashing; fallback kept for safety. + try { + return createHmac('sha256', TELEMETRY_SCOPE_SALT).update(guid).digest('hex'); + } catch { + return createHash('sha256').update(guid).digest('hex'); + } +}
62-76: Add return type and memoize to avoid repeated subprocess calls.This is called during telemetry setup; memoizing avoids extra execs on subsequent uses within the same process.
-export function getMachineId() { +export function getMachineId(): string { + // simple per-process cache + if ((getMachineId as any)._cache) return (getMachineId as any)._cache as string; if (!(platform in guid)) { - return randomUUID(); + const v = randomUUID(); + (getMachineId as any)._cache = v; + return v; } try { const value = execSync(guid[platform as keyof typeof guid], { encoding: 'utf8' }) as unknown as string; const id = expose(value); if (!id) { - return randomUUID(); + const v = randomUUID(); + (getMachineId as any)._cache = v; + return v; } - return hash(id); + const v = hash(id); + (getMachineId as any)._cache = v; + return v; } catch { - return randomUUID(); + const v = randomUUID(); + (getMachineId as any)._cache = v; + return v; } }If you prefer a typed cache, add near the top of the module:
let cachedId: string | undefined;and replace the function’s cache lines accordingly.
packages/cli/src/index.ts (3)
13-19: Type safety: avoidanyfor subcommand optionsUse the action signatures to type options.
Apply:
-const migrateAction = async (subCommand: string, options: any): Promise<void> => { +const migrateAction = async ( + subCommand: string, + options: Parameters<typeof actions.migrate>[1] +): Promise<void> => { await telemetry.trackCommand(`migrate ${subCommand}`, () => actions.migrate(subCommand, options)); }; -const dbAction = async (subCommand: string, options: any): Promise<void> => { +const dbAction = async ( + subCommand: string, + options: Parameters<typeof actions.db>[1] +): Promise<void> => { await telemetry.trackCommand(`db ${subCommand}`, () => actions.db(subCommand, options)); };
147-158: Stronger error logging for non-Commander errorsLog stack when available and handle non-Error thrown values.
Apply:
- } catch (e) { - if (e instanceof CommanderError) { + } catch (e: unknown) { + if (e instanceof CommanderError) { // ignore exitCode = e.exitCode; - } else if (e instanceof CliError) { + } else if (e instanceof CliError) { // log console.error(colors.red(e.message)); exitCode = 1; - } else { - console.error(colors.red(`Unhandled error: ${e}`)); + } else if (e instanceof Error) { + console.error(colors.red(`Unhandled error: ${e.stack ?? e.message}`)); + exitCode = 1; + } else { + console.error(colors.red(`Unhandled error: ${String(e)}`)); exitCode = 1; } }
10-31: Event name consistency (optional)Consider
migrate:<subCommand>anddb:<subCommand>for easier querying/filtering.Example:
- await telemetry.trackCommand(`migrate ${subCommand}`, () => actions.migrate(subCommand, options)); + await telemetry.trackCommand(`migrate:${subCommand}`, () => actions.migrate(subCommand, options));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/cli/src/index.ts(2 hunks)packages/cli/src/telemetry.ts(1 hunks)packages/cli/src/utils/machine-id-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/telemetry.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/cli/src/index.tspackages/cli/src/utils/machine-id-utils.ts
🧬 Code graph analysis (1)
packages/cli/src/index.ts (2)
packages/cli/src/telemetry.ts (1)
telemetry(139-139)packages/cli/src/cli-error.ts (1)
CliError(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (4)
packages/cli/src/index.ts (4)
6-6: LGTM: telemetry importImport placement and usage are correct.
10-10: Good: command wrapped with telemetryWrapping generate with
trackCommandis correct and awaited.
22-31: Good: consistent telemetry wrapping
info,init, andcheckare all wrapped and awaited consistently.
33-33: Ignore export-breaking warning forcreateProgram— No imports or calls tocreateProgramexist outsidepackages/cli/src/index.ts; it’s only invoked internally at line 139, so its removal as an export cannot break external code.Likely an incorrect or invalid review comment.
|
Claude finished @ymc9's task —— View job Comprehensive Code ReviewTodo List:
Overall AssessmentThis PR adds a well-structured telemetry system to the ZenStack CLI. The implementation shows good attention to privacy concerns and follows best practices for opt-out mechanisms. Security & Privacy ✅Strengths:
Minor Concerns:
Code Quality & Architecture ✅Excellent Design Patterns:
Code Organization: // Good abstraction - packages/cli/src/telemetry.ts:119-125
async trackCommand(command: string, action: () => Promise<void> | void) {
await this.trackSpan('cli:command:start', 'cli:command:complete', 'cli:command:error', { command }, action);
}Environment Detection Utilities ✅Robust Implementation:
Minor Improvements Needed:
CLI Integration ✅Well-Integrated:
Performance Considerations ✅Efficient Implementation:
Good Practice: // Gives telemetry time to send before exit - packages/cli/src/index.ts:154-160
if (telemetry.isTracking) {
setTimeout(() => {
process.exit(exitCode);
}, 200);
}Build System Integration ✅Smart Token Replacement:
Issues & Recommendations🟡 Minor Issues
🟢 Suggestions
Test Coverage Needs
|
Summary by CodeRabbit