-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Collect hardware details telemetry. #16119
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: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @gundermanc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to the application's telemetry system by enabling the collection of detailed hardware specifications. It now gathers information about the CPU (model and core count), total RAM, and GPU, providing a clearer picture of the diverse environments in which the application operates. This data will be instrumental in understanding performance characteristics and optimizing future development. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds telemetry for hardware details like CPU, GPU, and RAM. The implementation has a couple of issues. First, there's a potential TypeError when accessing CPU information if os.cpus() returns an empty array, which would prevent telemetry events from being logged. Second, the hardware information is collected on every single telemetry event, which is inefficient. The getGpuInfo function in particular uses execSync, a blocking call that can cause performance bottlenecks. My review includes suggestions to fix the potential crash and to cache the hardware information to improve performance.
| function getGpuInfo(): string { | ||
| try { | ||
| switch (process.platform) { | ||
| case 'darwin': | ||
| return execSync( | ||
| 'system_profiler SPDisplaysDataType | grep "Chipset Model" | cut -d: -f2 | xargs', | ||
| ) | ||
| .toString() | ||
| .trim(); | ||
| case 'linux': | ||
| return execSync( | ||
| "lspci | grep -i 'vga\\|3d\\|2d' | head -n 1 | sed 's/.*: //'", | ||
| ) | ||
| .toString() | ||
| .trim(); | ||
| case 'win32': | ||
| return execSync('wmic path win32_videocontroller get name') | ||
| .toString() | ||
| .trim(); | ||
| default: | ||
| return 'NA'; | ||
| } | ||
| } catch (error) { | ||
| debugLogger.error( | ||
| 'Failed to get GPU information for telemetry', | ||
| getErrorMessage(error), | ||
| ); | ||
| return 'FAILED'; | ||
| } | ||
| } |
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.
Hardware information (CPU, GPU, RAM) is collected on every telemetry event within createBasicLogEvent. This is inefficient, particularly the getGpuInfo function which uses execSync and blocks the main thread, creating a performance bottleneck. Since this hardware information is unlikely to change during the CLI's runtime, it should be collected only once and cached. A good approach would be to gather this information in the ClearcutLogger constructor, store it in private class properties, and reuse it for all events.
|
Size Change: +720 kB (+3.23%) Total Size: 23 MB
ℹ️ View Unchanged
|
17fc344 to
dec5dc9
Compare
e695d88 to
34faa5d
Compare
4ccba63 to
64a1993
Compare
Summary
Adds additional hardware telemetry. Now captures CPU model name, GPU model name, core count, and available RAM.
Note that making the entire stack asynchronous was going to require a significant amount of refactors so for now I use
voidin loggers.ts to fire and forget without raising a linter error.Details
The change is separate into commits. The first few are the actual change. The final commit is just the refactors needed to change the logging methods from sync to async.
Pre-Merge Checklist