feat: add cpu memory monitoring

[shrimalmadhur]

Motivation:

Modifications:

• Adds an option for users to enable memory monitoring
• Enable CPU monitoring by default

Info output

Upgrade / Create ask

Result:

Testing:

Open questions:

[claude]

Claude finished @shrimalmadhur's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of this PR that adds CPU/memory monitoring to the eigenx CLI.

Todo List:

• Read repository guidelines and understand context
• Review all 10 changed files systematically
• Check for bugs and security concerns
• Evaluate performance implications
• Assess code style and best practices
• Review test coverage needs
• Provide comprehensive feedback

Review Summary

This PR adds CPU/memory monitoring functionality to the eigenx CLI. The implementation is well-structured but has several areas for improvement:

🔧 Code Quality & Best Practices

✅ Strengths:

• Consistent with existing CLI patterns using urfave/cli/v2
• Good separation of concerns between interactive prompts, build logic, and Docker templates
• Proper error handling throughout the codebase
• Follows the repository's established flag naming conventions

⚠️ Areas for Improvement:

1. Interactive Prompt Clarity (pkg/commands/utils/interactive.go:914) - Addressing reviewer feedback:

   • Current prompt: "Show resource usage (CPU/memory) for your app?"
   • Consider being more specific about what enabling this means (e.g., performance impact, data collection)

2. Flag Value Validation (pkg/common/flags.go:73-76):

   ResourceUsageFlag = &cli.StringFlag{
       Name:  "resource-usage-monitoring",
       Usage: "Resource usage monitoring: enable or disable",
   }

   • Should specify valid values in the usage string: "Resource usage monitoring: 'enable' or 'disable'"

3. Template Field Naming (pkg/commands/utils/types.go:34):

   ResourceUsageAllow string

   • The name ResourceUsageAllow is unclear. Consider EnableResourceMonitoring or ResourceMonitoringEnabled

🐛 Potential Bugs

1. Missing Default Handling (pkg/commands/utils/interactive.go:898-907):

   if flagValue := cCtx.String("resource-usage-monitoring"); flagValue != "" {
       switch strings.ToLower(flagValue) {
       case "enable":
           return "always", nil
       case "disable":
           return "never", nil
       default:
           return "", fmt.Errorf("invalid --resource-usage-monitoring value: %s (must be enable or disable)", flagValue)
       }
   }

   • No handling for when the flag is provided but empty (--resource-usage-monitoring="")

2. Inconsistent Template Value (internal/templates/docker/Dockerfile.layered.tmpl:49):

   LABEL tee.launch_policy.monitoring_memory_allow={{.ResourceUsageAllow}}

   • Template expects a string but function returns "always" or "never" - verify this matches the expected label format

🔒 Security Concerns

✅ No significant security issues identified

• The monitoring feature appears to be read-only resource observation
• No user input is directly interpolated into shell commands or Docker builds
• Template escaping is handled by Go's text/template package

⚡ Performance Considerations

✅ Minimal Performance Impact:

• The interactive prompts and new flags don't add significant overhead
• CPU monitoring is enabled by default, which is reasonable for observability
• Memory monitoring is optional and user-controlled

💡 Suggestion: Consider documenting any performance impact of enabling memory monitoring in the user prompt or help text.

🧪 Test Coverage

⚠️ Missing Test Coverage:

1. No unit tests found for the new GetResourceUsageSetting() function
2. No tests for the updated template rendering with ResourceUsageAllow field
3. Consider adding tests for:
   • Interactive prompt behavior with various inputs
   • Flag value validation (valid/invalid cases)
   • Template rendering with new memory monitoring label

Recommended Test Cases:

func TestGetResourceUsageSetting(t *testing.T) {
    // Test flag values: "enable", "disable", "invalid", ""
    // Test interactive prompt behavior
    // Test error cases
}

func TestLayeredDockerfileTemplate(t *testing.T) {
    // Test template rendering with ResourceUsageAllow values
    // Verify correct label generation
}

🔧 Architecture & Design

✅ Good Design Choices:

• Maintains consistency with existing log visibility pattern
• CPU monitoring enabled by default (sensible default for observability)
• Memory monitoring is opt-in (good for performance-conscious users)
• Proper integration with existing build pipeline

💡 Enhancement Suggestions:

1. Consider adding a --monitoring flag that accepts cpu, memory, both, or none for more granular control
2. Add validation to ensure the Docker template labels match the expected TEE platform format

📝 Documentation & Usability

1. Command Help Text: The current flag usage text could be clearer about the implications
2. User Prompts: Following reviewer suggestion, make the resource usage prompt more descriptive
3. Error Messages: Good error messages with clear valid options

🚀 Recommendations

High Priority:

1. Update the interactive prompt text to be clearer (per reviewer feedback)
2. Add proper flag value validation and better usage text
3. Consider renaming ResourceUsageAllow template field for clarity

Medium Priority:

1. Add unit tests for the new functionality
2. Document any performance implications of memory monitoring
3. Consider more granular monitoring control options

Low Priority:

1. Add integration tests for the full deploy/upgrade flow with monitoring options
2. Consider adding metrics/telemetry for monitoring feature usage

✅ Overall Assessment

This is a solid implementation that follows the project's established patterns well. The main issues are around clarity of user-facing elements (prompts, help text) and missing test coverage. The core functionality appears sound and secure.

The changes are focused and don't introduce any breaking changes, making this a low-risk addition to the CLI.