-
Notifications
You must be signed in to change notification settings - Fork 32
feat(telemetry): adding initial telemetry functionality to the cli #956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v1.x
Are you sure you want to change the base?
Conversation
| // Temporarily commented out due to dependency version mismatch. | ||
| // SDK has "alpm" but registry's EcosystemString doesn't yet. | ||
| // type MissingInEcosystemString = Exclude<PURL_Type, EcosystemString> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sdk synced the latest version of OAS spec.
| hooks: { | ||
| onRequest: (info: RequestInfo) => { | ||
| // Skip tracking for telemetry submission endpoints to prevent infinite loop. | ||
| const isTelemetryEndpoint = info.url.includes('/telemetry') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sdk is calling the hooks on some endpoint and not others. I am not sure if this is the intended pattern. (Should only some endpoints and functions gets hooks and not others? If so, that could simplify this awkward logic.)
bcc83ee to
3082a6e
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment @cursor review or bugbot run to trigger another review on this PR
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment @cursor review or bugbot run to trigger another review on this PR
| await trackSubprocessError(command, startTime, error, exitCode) | ||
| } else if (exitCode === 0) { | ||
| await trackSubprocessComplete(command, startTime, exitCode) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Signal-terminated subprocesses not tracked in telemetry
The trackSubprocessExit function only tracks events when exitCode !== null && exitCode !== 0 (error) or exitCode === 0 (complete). When a subprocess is killed by a signal, Node.js sets code to null and signalName to the signal. This case falls through without tracking any telemetry event, leaving a gap in subprocess tracking for signal-terminated processes.
|
|
||
| // eslint-disable-next-line n/no-process-exit | ||
| process.exit(1) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Async handlers for process events may not complete
Using async handlers with process.on('uncaughtException') and process.on('unhandledRejection') is problematic because Node.js doesn't wait for async operations to complete before the handler returns. If the async telemetry calls (trackCliError, finalizeTelemetry) fail or take time, the process may exit before they complete. Additionally, any errors thrown within these async handlers won't be caught, potentially causing secondary unhandled rejections.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the telemetry will await to be flushed before being exited with 1
1f673f5 to
da523cf
Compare
Summary
This PR adds telemetry functionality to the Socket CLI to track usage patterns, performance metrics, and errors. The implementation includes instrumentation across CLI commands, subprocess executions, and API interactions.
Telemetry Infrastructure
Event Types Tracked
PII Sanitization
Example Sanitization
Telemetry Configuration
Breaking Changes
None. Telemetry is opt-in via organization configuration and fails gracefully.
Note
Introduces org-scoped telemetry across the CLI, SDK, and package manager wrappers with sanitization, batching/flush, global error handling, and comprehensive tests; also updates ecosystems and bumps the SDK.
utils/telemetry/*(integration, service, types) for org-scoped events, argv/error sanitization, session IDs, batching, periodic flush, and timeouts.src/cli.mtsto trackcli_start,cli_complete,cli_error; ensurefinalizeTelemetry(); add handlers for uncaught exceptions and unhandled rejections.npm,npx,pnpm,yarn):cmd-*.mtsand flush telemetry before process exit.utils/sdk.mts, add request/response hooks to emitapi_request,api_response,api_error(skipping telemetry endpoints) with optional debug logging.ALL_ECOSYSTEMS(e.g.,alpm,qpkg,vscode) and comment out strict type check due to temporary registry/SDK mismatch.src/test/*.mts,utils/telemetry/*.test.mts).@socketsecurity/sdkto1.4.95(lockfile updated).Written by Cursor Bugbot for commit 1f673f5. Configure here.