Skip to content

Conversation

sarumaj
Copy link

@sarumaj sarumaj commented Aug 5, 2025

I added support for AMD and NVIDIA GPUs by using platform specific commands. Tested only on Linux, the implementations for Windows and MacOS require appropriate review.

Copy link
Contributor

coderabbitai bot commented Aug 5, 2025

Walkthrough

This change set introduces GPU monitoring support across the system, spanning backend data collection, frontend visualization, documentation, and testing. A new documentation file details the GPU monitoring feature, including platform-specific command-line integrations for Linux, macOS, and Windows. The backend (sysinfo.go) implements GPU metrics collection using platform-specific tools, aggregates results, and integrates them with existing system metrics. Corresponding test cases are added to validate platform detection and GPU data parsing. The frontend is updated to support new GPU-related plot types and metadata, and a new CSS variable for GPU color is introduced. Minor unrelated cleanup is performed in file write logic, and a new constant for GPU time series is added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

  • Complexity: The changes span multiple domains—backend system-level integration (with substantial new code for platform-specific GPU data collection and parsing), frontend plotting logic, documentation, and testing. The backend changes are intricate due to platform differences and error handling. The frontend and style updates are straightforward but require cross-verification with new backend metrics.
  • Scope: Multiple new functions, structs, and test cases are introduced, along with updates to the frontend and documentation.
  • Estimated review time: ~40 minutes, factoring in the need to validate correctness, robustness, and cross-platform handling in the backend, and to ensure frontend and documentation alignment.

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1aebc5b and 14cfc5a.

📒 Files selected for processing (1)
  • pkg/wshrpc/wshremote/sysinfo_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/wshrpc/wshremote/sysinfo_test.go
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (7)
pkg/wshrpc/wshremote/sysinfo_test.go (1)

20-28: Consider adding verification for tool availability tests.

While logging the availability is useful, consider adding assertions to verify expected behavior in CI environments or when specific tools are known to be present.

pkg/wshrpc/wshremote/sysinfo.go (6)

189-189: Consider using debug-level logging for GPU detection.

This log statement will execute every second for each GPU, potentially flooding the logs. Consider using debug-level logging or removing it.

-		log.Printf("Found macOS GPU: %s", name)
+		// Only log in debug mode or remove entirely

399-399: Document the GPU memory estimation heuristic.

The 10% estimation of system memory for GPU is arbitrary and could be misleading. Consider adding a comment explaining this is a rough fallback estimate.

 			// Estimate GPU memory as a fraction of system memory
-			// This is a rough estimate and varies by GPU
+			// This is a rough fallback estimate (10% of system RAM) when actual VRAM info is unavailable
+			// Actual GPU memory varies significantly and this should be treated as unreliable
 			return totalGB * 0.1 // Assume 10% of system memory for GPU

463-463: Consider using debug-level logging for Windows GPU detection.

Similar to the macOS implementation, this will log every second for each GPU. Consider using debug-level logging or removing it.

-		log.Printf("Found Windows GPU: %s (%.2f GB total, %.2f GB used)", gpu.Name, gpu.MemTotal, memUsed)
+		// Only log in debug mode or remove entirely

415-427: Consider extracting PowerShell scripts to separate files or constants.

Large embedded PowerShell scripts are difficult to maintain and test. Consider extracting them to constants or external files for better maintainability.

+const psGetGpuInfoScript = `
+	$gpus = Get-WmiObject -Class Win32_VideoController | Where-Object { $_.Name -notlike "*Basic*" -and $_.Name -notlike "*Standard*" }
+	$gpuInfo = @()
+	foreach ($gpu in $gpus) {
+		$gpuInfo += [PSCustomObject]@{
+			Name = $gpu.Name
+			AdapterRAM = $gpu.AdapterRAM
+			VideoProcessor = $gpu.VideoProcessor
+			DriverVersion = $gpu.DriverVersion
+		}
+	}
+	$gpuInfo | ConvertTo-Json -Compress
+`
 
 func getWindowsGpuData() ([]GpuData, error) {
 	// ...
-	psCommand := `
-		$gpus = Get-WmiObject -Class Win32_VideoController | Where-Object { $_.Name -notlike "*Basic*" -and $_.Name -notlike "*Standard*" }
-		// ... rest of script
-	`
+	psCommand := psGetGpuInfoScript

641-643: Consider logging GPU detection failures for debugging.

Silently ignoring GPU detection errors makes troubleshooting difficult. Consider logging the error at debug level.

 if err != nil || len(gpus) == 0 {
+	if err != nil {
+		log.Printf("Failed to get GPU data: %v", err)
+	}
 	return
 }

697-697: Remove redundant inline comment.

The function name getGpuData is self-documenting. The inline comment adds no value.

-	getGpuData(values) // Add this line to get GPU data
+	getGpuData(values)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d339af and be0ea10.

📒 Files selected for processing (7)
  • docs/GPU_MONITORING.md (1 hunks)
  • frontend/app/theme.scss (1 hunks)
  • frontend/app/view/sysinfo/sysinfo.tsx (2 hunks)
  • pkg/wshrpc/wshremote/sysinfo.go (3 hunks)
  • pkg/wshrpc/wshremote/sysinfo_test.go (1 hunks)
  • pkg/wshrpc/wshremote/wshremote.go (1 hunks)
  • pkg/wshrpc/wshrpctypes.go (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: the `copyremote` function in waveterm's file operations has proper error handling that prevents part...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1725
File: pkg/remote/fileshare/wavefs/wavefs.go:441-494
Timestamp: 2025-01-29T04:21:11.649Z
Learning: The `CopyRemote` function in WaveTerm's file operations has proper error handling that prevents partial writes by failing fast and using context cancellation. Each step (path cleaning, file operations, tar reading/writing) is guarded by error checks that prevent proceeding with writes on error.

Applied to files:

  • pkg/wshrpc/wshremote/wshremote.go
📚 Learning: the rpcclient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.

Applied to files:

  • pkg/wshrpc/wshremote/wshremote.go
  • pkg/wshrpc/wshremote/sysinfo.go
📚 Learning: the aws connection code path in pkg/remote/awsconn/awsconn.go is currently inactive, so temporary fi...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/awsconn/awsconn.go:32-33
Timestamp: 2025-01-22T01:02:32.621Z
Learning: The AWS connection code path in pkg/remote/awsconn/awsconn.go is currently inactive, so temporary file cleanup is not a critical concern at this time.

Applied to files:

  • pkg/wshrpc/wshremote/wshremote.go
📚 Learning: the getrpccontext() method in wshutil package is guaranteed to never return nil due to type constrai...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.

Applied to files:

  • pkg/wshrpc/wshremote/sysinfo.go
🔇 Additional comments (11)
pkg/wshrpc/wshrpctypes.go (1)

583-583: LGTM! Consistent constant addition.

The new TimeSeries_Gpu constant follows the existing naming pattern and integrates well with the GPU monitoring functionality.

frontend/app/theme.scss (1)

119-119: LGTM! Consistent color variable addition.

The new GPU color variable follows the existing pattern and provides good visual distinction from CPU and memory colors.

pkg/wshrpc/wshremote/wshremote.go (1)

813-815: LGTM! Good cleanup of unused variables.

Removing the unused byte count variables while preserving error handling improves code cleanliness without affecting functionality.

pkg/wshrpc/wshremote/sysinfo_test.go (4)

12-18: LGTM! Good basic platform detection test.

The test appropriately verifies that platform detection returns a non-empty string and logs the result for visibility.


30-46: Good exploratory test for GPU data collection.

The test properly handles the case where no GPU data is available, which is important for systems without GPU monitoring tools.


48-77: Excellent test coverage for macOS GPU parsing.

The test uses realistic sample data and covers multiple parsing functions. The logging provides good visibility into the parsing results.


79-93: Good Windows GPU parsing test coverage.

The test validates both GPU detection and memory usage parsing with realistic JSON sample data.

frontend/app/view/sysinfo/sysinfo.tsx (3)

69-95: Excellent GPU plot type implementations.

The new GPU plot types follow the existing patterns perfectly:

  • Consistent filtering and sorting logic
  • Proper key extraction for utilization and memory metrics
  • Good integration with existing CPU + memory combinations

104-104: Good addition of base GPU metadata.

The GPU utilization metadata reuses the CPU percentage logic, which is appropriate since both represent utilization percentages.


109-121: Comprehensive GPU metadata coverage.

The loop creates metadata for up to 8 GPUs covering:

  • Utilization (%), memory (GB), and temperature (°C)
  • Consistent use of the new GPU color variable
  • Appropriate value ranges and decimal places

This provides excellent support for multi-GPU systems.

pkg/wshrpc/wshremote/sysinfo.go (1)

128-129: Verify regex correctness against real rocm-smi output
We weren’t able to run rocm-smi in this environment, so it’s unclear whether the single-line regex at pkg/wshrpc/wshremote/sysinfo.go:128–129 will ever match the actual (multi-line) output.

Please test on a machine with AMD ROCm installed and:

  • Capture a sample of rocm-smi --showproductname --showmeminfo vram --showtemp output
  • Confirm the current regex matches each GPU block, or adjust it to account for line breaks/format changes
  • Consider a more robust parsing approach (e.g., splitting on blank lines and using separate patterns per field) if necessary

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (2)
pkg/wshrpc/wshremote/sysinfo.go (2)

50-61: Platform detection correctly implemented!

The function now properly uses runtime.GOOS for cross-platform detection, which resolves the previous issue with uname not being available on Windows.


108-110: Inconsistent error handling for parsing GPU metrics.

While errors are handled for index and util parsing (lines 98-107), the parsing of memUsed, memTotal, and temp still ignores errors. This inconsistency could lead to confusion about which metrics are reliable.

Apply consistent error handling:

-memUsed, _ := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
-memTotal, _ := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
-temp, _ := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+memUsed, err := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory used: %v", err)
+    memUsed = 0
+}
+memTotal, err := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory total: %v", err)
+    memTotal = 0
+}
+temp, err := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi temperature: %v", err)
+    temp = 0
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between be0ea10 and 59257cd.

📒 Files selected for processing (2)
  • docs/GPU_MONITORING.md (1 hunks)
  • pkg/wshrpc/wshremote/sysinfo.go (3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: in go, using a single if statement with the `&&` operator (e.g. `if x != nil && x.field != nil`) is ...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1807
File: pkg/wshrpc/wshremote/wshremote.go:718-726
Timestamp: 2025-01-23T06:09:05.334Z
Learning: In Go, using a single if statement with the `&&` operator (e.g. `if x != nil && x.Field != nil`) is the idiomatic way to handle multiple nil checks. Avoid suggesting nested if statements as they make the code more verbose without adding value.

Applied to files:

  • pkg/wshrpc/wshremote/sysinfo.go
📚 Learning: the getrpccontext() method in wshutil package is guaranteed to never return nil due to type constrai...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.

Applied to files:

  • pkg/wshrpc/wshremote/sysinfo.go
📚 Learning: the rpcclient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.

Applied to files:

  • pkg/wshrpc/wshremote/sysinfo.go
🧬 Code Graph Analysis (1)
pkg/wshrpc/wshremote/sysinfo.go (1)
pkg/wshrpc/wshrpctypes.go (1)
  • TimeSeries_Gpu (583-583)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
pkg/wshrpc/wshremote/sysinfo.go (1)

49-61: Platform detection correctly implemented.

The function now properly uses runtime.GOOS for reliable cross-platform detection, addressing the previous Windows compatibility issue.

🧹 Nitpick comments (4)
pkg/wshrpc/wshremote/sysinfo.go (4)

108-110: Complete error handling for remaining fields.

While error handling was added for index and utilization parsing, the memory and temperature parsing still ignores errors. For consistency and robustness, consider adding error handling for these fields as well.

-memUsed, _ := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
-memTotal, _ := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
-temp, _ := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+memUsed, err := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory used: %v", err)
+    memUsed = 0
+}
+memTotal, err := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory total: %v", err)
+    memTotal = 0
+}
+temp, err := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi temperature: %v", err)
+    temp = 0
+}

125-179: Good improvements with error handling, but consider output format robustness.

The error handling has been properly implemented as requested in previous reviews. However, the regex pattern for parsing rocm-smi output might be fragile if the output format changes between versions.

Consider adding more flexible parsing or fallback mechanisms for different rocm-smi output formats to improve robustness across AMD driver versions.


254-280: Improve macOS GPU utilization detection reliability.

The current approach to parse iostat output for GPU utilization is quite fragile. The parsing logic looks for lines containing "gpu" or "GPU" but iostat typically doesn't provide GPU-specific information in most versions.

Consider returning 0 or using a more reliable method for GPU utilization on macOS, as this parsing is likely to fail in most cases:

 func getMacGpuUtilization() float64 {
-	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
-	defer cancel()
-
-	// Try to get GPU utilization using iostat
-	cmd := exec.CommandContext(ctx, "iostat", "-n", "1", "1")
-	output, err := cmd.Output()
-	if err != nil {
-		return 0
-	}
-
-	// Parse iostat output for GPU utilization
-	lines := strings.Split(string(output), "\n")
-	for _, line := range lines {
-		if strings.Contains(line, "gpu") || strings.Contains(line, "GPU") {
-			fields := strings.Fields(line)
-			if len(fields) >= 2 {
-				if util, err := strconv.ParseFloat(fields[1], 64); err == nil {
-					return util
-				}
-			}
-		}
-	}
-
-	return 0
+	// iostat doesn't typically provide GPU utilization on macOS
+	// Return 0 to indicate unavailable data
+	return 0
 }

468-510: Improve Windows GPU output parsing robustness.

The parseWindowsGpuOutput function uses regex to parse JSON-like output, but this approach is fragile. If the PowerShell command actually returns valid JSON, consider using proper JSON parsing.

+import "encoding/json"

 func parseWindowsGpuOutput(output string) []WindowsGpuInfo {
 	var gpuList []WindowsGpuInfo
 
-	// Try to parse as JSON array
 	if strings.TrimSpace(output) == "" {
 		return gpuList
 	}
 
-	// Simple JSON parsing for the PowerShell output
-	// The output should be a JSON array of GPU objects
-	lines := strings.Split(output, "\n")
-	for _, line := range lines {
-		line = strings.TrimSpace(line)
-		if line == "" {
-			continue
-		}
-
-		// Extract GPU name and memory info using regex
-		nameRe := regexp.MustCompile(`"Name":\s*"([^"]+)"`)
-		adapterRamRe := regexp.MustCompile(`"AdapterRAM":\s*(\d+)`)
-
-		nameMatches := nameRe.FindStringSubmatch(line)
-		ramMatches := adapterRamRe.FindStringSubmatch(line)
-
-		if len(nameMatches) >= 2 && len(ramMatches) >= 2 {
-			name := nameMatches[1]
-			if ramSize, err := strconv.ParseInt(ramMatches[1], 10, 64); err == nil {
-				// Convert bytes to GB
-				memTotal := float64(ramSize) / (1024 * 1024 * 1024)
-
-				gpuInfo := WindowsGpuInfo{
-					Name:     name,
-					MemTotal: memTotal,
-					MemUsed:  0,
-				}
-				gpuList = append(gpuList, gpuInfo)
-			}
-		}
-	}
+	// Try proper JSON parsing first
+	var rawGpus []map[string]interface{}
+	if err := json.Unmarshal([]byte(output), &rawGpus); err == nil {
+		for _, gpu := range rawGpus {
+			if name, ok := gpu["Name"].(string); ok {
+				if ramInterface, ok := gpu["AdapterRAM"]; ok {
+					var ramSize int64
+					switch v := ramInterface.(type) {
+					case float64:
+						ramSize = int64(v)
+					case string:
+						if parsed, err := strconv.ParseInt(v, 10, 64); err == nil {
+							ramSize = parsed
+						}
+					}
+					
+					if ramSize > 0 {
+						memTotal := float64(ramSize) / (1024 * 1024 * 1024)
+						gpuInfo := WindowsGpuInfo{
+							Name:     name,
+							MemTotal: memTotal,
+							MemUsed:  0,
+						}
+						gpuList = append(gpuList, gpuInfo)
+					}
+				}
+			}
+		}
+		return gpuList
+	}
+	
+	// Fallback to regex parsing if JSON parsing fails
+	// ... existing regex logic ...
 
 	return gpuList
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 59257cd and 1aebc5b.

📒 Files selected for processing (3)
  • docs/GPU_MONITORING.md (1 hunks)
  • pkg/wshrpc/wshremote/sysinfo.go (3 hunks)
  • pkg/wshrpc/wshremote/sysinfo_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/wshrpc/wshremote/sysinfo_test.go
  • docs/GPU_MONITORING.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: the getrpccontext() method in wshutil package is guaranteed to never return nil due to type constrai...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.

Applied to files:

  • pkg/wshrpc/wshremote/sysinfo.go
📚 Learning: the rpcclient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either...
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.

Applied to files:

  • pkg/wshrpc/wshremote/sysinfo.go
🧬 Code Graph Analysis (1)
pkg/wshrpc/wshremote/sysinfo.go (1)
pkg/wshrpc/wshrpctypes.go (1)
  • TimeSeries_Gpu (583-583)
🔇 Additional comments (5)
pkg/wshrpc/wshremote/sysinfo.go (5)

6-38: LGTM! Imports and constants are well-structured.

The new imports are appropriate for the GPU monitoring functionality, and the PowerShell command constant is properly formatted for Windows GPU detection.


40-47: LGTM! Well-designed GPU data structure.

The GpuData struct appropriately captures essential GPU metrics with proper JSON tags for serialization.


63-79: LGTM! Proper utility availability detection.

Both functions correctly use context with timeout and appropriate commands to detect GPU utility availability.


604-646: LGTM! Well-structured GPU data integration.

The main getGpuData function properly orchestrates platform-specific GPU data collection and integrates it cleanly with the existing metrics system. The aggregate utilization calculation and error handling are appropriate.


681-681: LGTM! Clean integration with existing metrics collection.

The addition of getGpuData(values) follows the established pattern and integrates seamlessly with the existing CPU and memory data collection.

Comment on lines +336 to +399
// Get memory pressure from vm_stat command
func getMemoryPressureFromVMStat() float64 {
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
defer cancel()

cmd := exec.CommandContext(ctx, "vm_stat")
output, err := cmd.Output()
if err != nil {
return 0
}

lines := strings.Split(string(output), "\n")
var pageSize int64 = 4096 // Default page size

// Find page size
for _, line := range lines {
if strings.Contains(line, "Mach Virtual Memory Statistics") {
re := regexp.MustCompile(`page size of (\d+)`)
matches := re.FindStringSubmatch(line)
if len(matches) >= 2 {
if size, err := strconv.ParseInt(matches[1], 10, 64); err == nil {
pageSize = size
}
}
break
}
}

// Parse memory statistics
var activePages, inactivePages, wiredPages int64

for _, line := range lines {
line = strings.TrimSpace(line)
if strings.Contains(line, "Pages active:") {
re := regexp.MustCompile(`Pages active:\s+(\d+)`)
matches := re.FindStringSubmatch(line)
if len(matches) >= 2 {
if pages, err := strconv.ParseInt(matches[1], 10, 64); err == nil {
activePages = pages
}
}
} else if strings.Contains(line, "Pages inactive:") {
re := regexp.MustCompile(`Pages inactive:\s+(\d+)`)
matches := re.FindStringSubmatch(line)
if len(matches) >= 2 {
if pages, err := strconv.ParseInt(matches[1], 10, 64); err == nil {
inactivePages = pages
}
}
} else if strings.Contains(line, "Pages wired down:") {
re := regexp.MustCompile(`Pages wired down:\s+(\d+)`)
matches := re.FindStringSubmatch(line)
if len(matches) >= 2 {
if pages, err := strconv.ParseInt(matches[1], 10, 64); err == nil {
wiredPages = pages
}
}
}
}

// Calculate used memory in GB
usedBytes := (activePages + inactivePages + wiredPages) * pageSize
return float64(usedBytes) / (1024 * 1024 * 1024) // Convert to GB
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Misleading GPU memory usage calculation.

The getMemoryPressureFromVMStat function calculates system memory pressure, not GPU memory usage. Using system memory statistics as a proxy for GPU memory usage is misleading and could confuse users.

Consider either:

  1. Returning 0 to indicate GPU memory usage is unavailable on macOS
  2. Clearly documenting that this represents system memory pressure, not GPU memory
  3. Finding a more accurate method to estimate GPU memory usage
 func getMemoryPressureFromVMStat() float64 {
-	// Complex vm_stat parsing logic...
+	// GPU memory usage is not directly available on macOS
+	// Return 0 to indicate unavailable data
+	return 0
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In pkg/wshrpc/wshremote/sysinfo.go lines 336 to 399, the function
getMemoryPressureFromVMStat currently calculates system memory pressure but is
misleadingly used or interpreted as GPU memory usage. To fix this, either modify
the function to return 0 to indicate GPU memory usage is unavailable on macOS,
or add clear documentation in the function comment stating it returns system
memory pressure, not GPU memory usage. Alternatively, if feasible, implement a
more accurate method to estimate GPU memory usage on macOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants