Skip to content

feat: allow making telemetry calls from CLI#346

Merged
binoy14 merged 1 commit intomainfrom
sdk-729
Feb 3, 2026
Merged

feat: allow making telemetry calls from CLI#346
binoy14 merged 1 commit intomainfrom
sdk-729

Conversation

@binoy14
Copy link
Copy Markdown
Contributor

@binoy14 binoy14 commented Jan 27, 2026

This PR sets up the functions and ways to make telemetry calls in the command. (See the stacked PR on top on how to use it.)

The way this works is that once we initialize the telemetry store we inject a new context of the cli trace to globals this maybe neat or maybe silly as this is same as window. but I have not found a great way to share this information between hooks and commands. I looked thru oclif github issues and this was one of the way it was recommended.

Happy to look thru other options if anyone has any ideas

@linear
Copy link
Copy Markdown

linear bot commented Jan 27, 2026

@claude
Copy link
Copy Markdown

claude bot commented Jan 27, 2026

Claude finished @binoy14's task in 1m 53s —— View job


Code Review

Issues Found

1. Missing Type Exports (packages/@sanity/cli-core/src/index.ts:22)

The public types CLITelemetryStore and TelemetryUserProperties are not exported from the main index file, but CLITelemetryStore is used as a return type in the exported getCliTelemetry() function and as a property type in SanityCommand. This will cause TypeScript errors for consumers.

Fix: Add to index.ts:

export {type CLITelemetryStore, type TelemetryUserProperties} from './telemetry/types.js'

2. Potential Race Condition in SanityCommand (packages/@sanity/cli-core/src/SanityCommand.ts:113)

The telemetry is initialized in SanityCommand.init() by calling getCliTelemetry(), which throws an error if telemetry hasn't been set up yet. However, if a command's init() is called before the setupTelemetry prerun hook completes, this will crash.

Fix: Consider making telemetry optional or adding proper error handling in getCliTelemetry() to handle cases where init is called before the prerun hook completes.


3. Redundant Dependency (packages/@sanity/cli-core/package.json:76)

The package adds typeid-js as a dependency, but @sanity/telemetry (which is already a peer dependency) provides createSessionId() that uses typeid internally. The codebase creates its own createTraceId() function, but this duplicates functionality.

Fix: Use createSessionId() from @sanity/telemetry instead of adding typeid-js as a separate dependency, or document why a separate trace ID implementation is needed.


4. Worker Error Handling Loses Context (packages/@sanity/cli/src/hooks/prerun/flushTelemetry.worker.ts:17-19)

The worker silently exits on error without logging anything, making debugging failures difficult.

Fix:

} catch (error) {
  // Log to stderr for debugging without blocking parent
  console.error('Telemetry flush failed:', error)
  process.exit(1)
}

5. Process Exit Error Handling (packages/@sanity/cli/src/hooks/prerun/setupTelemetry.ts:70-72)

The TODO comment indicates error handling is incomplete. Non-zero exit codes get a generic error message regardless of the actual error that occurred.

Recommendation: Implement proper error tracking (possibly via oclif's error handling hooks) or document why this approach is sufficient.


6. Test Coverage Gap (packages/@sanity/cli/src/hooks/prerun/flushTelemetry.worker.ts)

Coverage report shows 0% coverage for the worker file. The worker is a critical path for telemetry reliability.

Fix: Add tests for runFlushWorker() function, mocking the flush operation.

Copy link
Copy Markdown
Contributor Author

binoy14 commented Jan 27, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 27, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 74.42% 6118 / 8220
🔵 Statements 73.81% 6279 / 8506
🔵 Functions 68.32% 936 / 1370
🔵 Branches 61.46% 2716 / 4419
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/@sanity/cli-core/src/SanityCommand.ts 88.23% 100% 75% 100% 36, 46
packages/@sanity/cli-core/src/telemetry/telemetryStoreDebug.ts 100% 100% 100% 100%
packages/@sanity/cli-core/src/util/getCliTelemetry.ts 100% 100% 100% 100%
packages/@sanity/cli/src/actions/telemetry/getLearnMoreMessage.ts 100% 100% 100% 100%
packages/@sanity/cli/src/actions/telemetry/getStatusDisplay.ts 66.66% 60% 100% 66.66% 12, 18
packages/@sanity/cli/src/actions/telemetry/getStatusMessage.ts 88.88% 92.3% 100% 88.88% 34
packages/@sanity/cli/src/actions/telemetry/resolveConsent.ts 100% 100% 100% 100%
packages/@sanity/cli/src/actions/telemetry/setConsent.ts 96.66% 91.17% 100% 96.66% 133
packages/@sanity/cli/src/hooks/prerun/flushTelemetry.worker.ts 0% 0% 0% 0% 9-19
packages/@sanity/cli/src/hooks/prerun/setupTelemetry.ts 95% 50% 100% 95% 72
Generated in workflow #1946 for commit a43610c by the Vitest Coverage Report Action

@binoy14 binoy14 force-pushed the sdk-729 branch 2 times, most recently from b96cc57 to 30b9f03 Compare January 27, 2026 17:38
@binoy14 binoy14 marked this pull request as ready for review January 27, 2026 17:49
@binoy14 binoy14 requested a review from a team as a code owner January 27, 2026 17:49
@binoy14 binoy14 requested review from cngonzalez and removed request for a team January 27, 2026 17:49
@binoy14 binoy14 marked this pull request as draft January 27, 2026 20:26
@binoy14 binoy14 marked this pull request as ready for review January 27, 2026 23:41
@binoy14 binoy14 marked this pull request as draft January 27, 2026 23:42
@binoy14 binoy14 force-pushed the sdk-729 branch 5 times, most recently from d317d8a to b977c98 Compare January 30, 2026 15:35
@binoy14 binoy14 marked this pull request as ready for review January 30, 2026 16:12
@binoy14 binoy14 requested review from mttdnt and rexxars January 30, 2026 16:13
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 31, 2026

Preview this PR with pkg.pr.new

Run the Sanity CLI

npx https://pkg.pr.new/sanity-io/cli/@sanity/cli@a43610c <command>

...Or upgrade project dependencies

📦 @sanity/cli
pnpm install https://pkg.pr.new/@sanity/cli@a43610c
📦 @sanity/cli-core
pnpm install https://pkg.pr.new/@sanity/cli-core@a43610c
📦 @sanity/cli-test
pnpm install https://pkg.pr.new/@sanity/cli-test@a43610c
📦 @sanity/eslint-config-cli
pnpm install https://pkg.pr.new/@sanity/eslint-config-cli@a43610c

View Commit (a43610c)

Copy link
Copy Markdown
Member

@rexxars rexxars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@binoy14 binoy14 merged commit 41ef21e into main Feb 3, 2026
83 of 85 checks passed
@binoy14 binoy14 deleted the sdk-729 branch February 3, 2026 00:42
@squiggler-legacy squiggler-legacy bot mentioned this pull request Feb 2, 2026
This was referenced Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants