feat: optionally track telemetry for the main commands #614
+125
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request implements optional telemetry tracking for the main commands. It introduces a new telemetry utility function that sends telemetry data to the specified instance. The function is called at the end of each command execution, with the command name and status. The telemetry data includes the command name, status, workspace, and application information. If any error occurs during the command execution, the error message is also included in the telemetry data. Sequence diagram for telemetry tracking flowsequenceDiagram
participant User
participant CLI
participant SettleMint
User->>CLI: Execute command (connect/login/create/etc)
activate CLI
Note over CLI: Execute command logic
alt Success case
CLI->>SettleMint: POST /cm/telemetry
Note right of CLI: {command, status: 'success', workspace, application}
else Error case
CLI->>SettleMint: POST /cm/telemetry
Note right of CLI: {command, status: 'error', message, workspace, application}
end
CLI-->>User: Command result
deactivate CLI
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📦 Packages
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @roderik - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Review instructions: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sdk/cli/src/commands/login.ts
Outdated
Comment on lines
59
to
68
| personalAccessToken = await Promise.race([ | ||
| (async () => { | ||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of process.stdin) { | ||
| chunks.push(Buffer.from(chunk)); | ||
| } | ||
| return Buffer.concat(chunks).toString().trim(); | ||
| })(), | ||
| new Promise<string>((resolve) => setTimeout(() => resolve(""), 1_000)), | ||
| ]); |
There was a problem hiding this comment.
suggestion: 1-second timeout for stdin might be too short in some environments.
Consider making the timeout configurable or providing a more user-friendly error message if the timeout is reached.
Suggested change
| personalAccessToken = await Promise.race([ | |
| (async () => { | |
| const chunks: Buffer[] = []; | |
| for await (const chunk of process.stdin) { | |
| chunks.push(Buffer.from(chunk)); | |
| } | |
| return Buffer.concat(chunks).toString().trim(); | |
| })(), | |
| new Promise<string>((resolve) => setTimeout(() => resolve(""), 1_000)), | |
| ]); | |
| const STDIN_TIMEOUT_MS = process.env.STDIN_TIMEOUT_MS ? parseInt(process.env.STDIN_TIMEOUT_MS, 10) : 5_000; | |
| personalAccessToken = await Promise.race([ | |
| (async () => { | |
| const chunks: Buffer[] = []; | |
| for await (const chunk of process.stdin) { | |
| chunks.push(Buffer.from(chunk)); | |
| } | |
| return Buffer.concat(chunks).toString().trim(); | |
| })(), | |
| new Promise<string>((resolve, reject) => | |
| setTimeout(() => | |
| reject(new Error(`Timeout waiting for token input via STDIN after ${STDIN_TIMEOUT_MS}ms. Set STDIN_TIMEOUT_MS environment variable to adjust this timeout.`)), | |
| STDIN_TIMEOUT_MS | |
| ) | |
| ), | |
| ]); |
sdk/cli/src/commands/codegen.ts
Outdated
| promises.push(codegenIpfs(env)); | ||
| } | ||
| const results = await Promise.allSettled(promises); | ||
| if (results.some((r) => r.status === "rejected")) { |
There was a problem hiding this comment.
suggestion: Error handling in Promise.allSettled could be more detailed.
Consider providing more detailed information about which specific task failed to aid in debugging.
Suggested implementation:
const results = await Promise.allSettled(promises);
const failures = results
.map((result, index) => ({ result, index }))
.filter(({ result }) => result.status === "rejected")
.map(({ result, index }) => `Task ${index + 1}: ${(result as PromiseRejectedResult).reason}`);
if (failures.length > 0) {
cancel(`Errors occurred while generating resources:\n${failures.join('\n')}`);
}For even better error handling, you might want to:
- Add a mapping of promise index to task name/description when creating the promises array
- Use that mapping in the error message instead of just showing "Task 1", "Task 2", etc.
sdk/cli/src/utils/telemetry.ts
Outdated
| application: env.SETTLEMINT_APPLICATION, | ||
| }), | ||
| }).catch(() => {}), // Swallow fetch errors | ||
| new Promise((resolve) => setTimeout(resolve, 500)), // Timeout after 500ms |
There was a problem hiding this comment.
suggestion (review_instructions): Potential performance issue with 500ms timeout in telemetry function.
The 500ms timeout in the telemetry function might introduce unnecessary delays. Consider reducing the timeout duration to improve performance.
Review instructions:
Path patterns: **/*.ts
Instructions:
Always write correct, up to date, bug free, fully functional and working, secure, performant and efficient code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add telemetry to the create, connect, codegen, login, and logout commands.
New Features:
SETTLEMINT_DISABLE_TELEMETRYenvironment variable.Tests: