feat: inline metrics with sparkline display

[yimsk]

Closes #32

Summary

Inline CloudWatch metrics with sparkline visualization in resource browser.

Supported Resources

Resource	Metric	Dimension
EC2 Instances	CPUUtilization	InstanceId
RDS Instances	CPUUtilization	DBInstanceIdentifier
Lambda Functions	Invocations	FunctionName

Features

• M key toggle: Enable/disable metrics (default OFF to save API calls)
• Ctrl+r reload: Refresh metrics when enabled
• Sparkline: Unicode blocks ▁▂▃▄▅▆▇█ (15m window, 1m resolution)
• 30s timeout: Prevents UI blocking on slow API responses
• Auto-reload aware: Metrics refresh on auto-reload when enabled

Implementation

• New internal/metrics/ package (cloudwatch.go, sparkline.go, types.go)
• MetricSpecProvider interface for extensible resource metrics
• CloudWatch GetMetricData batch API (500 queries/request max)

Follow-up

ECS/ELB support tracked in #47 (requires multi-dimension support)

[yimsk]

Thanks for the thorough review!

Addressed in 37f7c25

Critical Issues ✅

1. Unit display - Added Unit field to MetricSpec. EC2/RDS use "%", Lambda uses empty string.
2. Resource ID mapping - Verified safe: same resourceIDs slice passed to both buildQueries and processResults within Fetch(). No modification possible between calls.

Should Fix ✅

3. Error logging - Added service/resource context to metrics load failures.
4. Context handling - Already using r.ctx which respects parent cancellation (was correct before).

Nice to Have (Deferred)

• Configurable time windows → feat: extend inline metrics to ECS/ELB #47
• Metrics caching → feat: extend inline metrics to ECS/ELB #47
• Concurrent batch fetching → already have errgroup import, can add later

[yimsk]

Fixed the race condition (1224038). loadMetricsCmd() now captures resource IDs synchronously before returning the closure, avoiding the data race when r.resources is modified during goroutine execution.

Other items from review:

• Stale metrics after pagination: tracked in feat: extend inline metrics to ECS/ELB #47 (follow-up)
• Metric ID collision: safe within batch scope (i resets per batch, not per global call)
• Error sanitization: agree, can add in follow-up
• Caching: tracked in feat: extend inline metrics to ECS/ELB #47

[yimsk]

Addressed review suggestions (a787583):

• ✅ Added comprehensive tests for processResults with actual MetricDataResult data
• ✅ Added M key to README key bindings
• ✅ Created docs/iam-permissions.md with cloudwatch:GetMetricData requirement
• ✅ Linked IAM doc from README Configuration section

Ready for merge.

[yimsk]

Fixed stale metrics race condition (28fb33f). metricsLoadedMsg now includes resourceType, validated on receive to discard stale data when user switches tabs.

Re: nil cmd in tea.Batch - tea.Batch handles nil gracefully (filters them out), and line 380 is guarded by getMetricSpec() != nil check.

Re: CloudWatch retry - can add in follow-up if throttling becomes an issue.
Re: strings.Builder - negligible perf impact for 12-element loop.

Ready for merge.

[yimsk]

Addressed context cancellation check from review (2343503).

golang.org/x/sync note: Already used in custom/computeoptimizer/recommendations/dao.go for errgroup - not new from this PR.

[claude]

Pull Request Review: Inline CloudWatch Metrics with Sparkline Display

Summary

This PR adds inline CloudWatch metrics visualization with sparklines to the resource browser for EC2, RDS, and Lambda resources. The implementation is well-structured with good test coverage and thoughtful design choices.

✅ Strengths

1. Code Quality & Architecture

• Clean separation of concerns with the new internal/metrics/ package
• Good use of interfaces (MetricSpecProvider) for extensibility
• Proper error handling with context timeouts and graceful degradation
• Race condition prevention: loadMetricsCmd() captures resource IDs synchronously (line 699-703) before launching goroutine
• Thoughtful UX defaults: metrics disabled by default to save API calls

2. Performance Considerations

• Efficient batch processing: Uses CloudWatch's GetMetricData API with batching (500 queries/request max)
• 30-second timeout prevents UI blocking on slow API responses
• Context cancellation support for early termination
• No N+1 queries - fetches all metrics in batched API calls

3. Test Coverage

• Comprehensive unit tests for all three modules (304 lines of tests for ~211 lines of code)
• Tests cover edge cases: nil values, empty data, constant values, truncation
• Good test organization with table-driven tests

4. Documentation

• New docs/iam-permissions.md clearly documents required CloudWatch permissions
• PR description includes clear feature summary and implementation details
• Updated README with key binding documentation

🔍 Issues & Recommendations

High Priority

1. Potential Memory Issue in Batch Processing
Location: internal/metrics/cloudwatch.go:45-69

The Fetch() method could create very large batches when there are thousands of resources. Consider:

// Current: builds ALL queries upfront
queries := f.buildQueries(resourceIDs, spec)  // Could be 10,000+ queries

// Recommendation: Build queries per batch to reduce memory footprint
for i := 0; i < len(resourceIDs); i += maxQueriesPerRequest {
    end := min(i+maxQueriesPerRequest, len(resourceIDs))
    batch := f.buildQueries(resourceIDs[i:end], spec)
    // ... process batch
}

2. Query ID Collision Risk
Location: internal/metrics/cloudwatch.go:78

Using simple index-based IDs (m0, m1, etc.) could collide across batches:

Id: aws.String(fmt.Sprintf("m%d", i))  // Resets to m0 for each batch

Fix: Include batch offset in ID generation:

Id: aws.String(fmt.Sprintf("m%d", batchOffset+i))

Medium Priority

3. Sparkline Rendering: Padding Logic
Location: internal/metrics/sparkline.go:54-56

The left-padding with · when len(spark) < SparklineWidth will never execute because the loop at line 39 always produces exactly len(values) characters. If len(values) < SparklineWidth, this padding is needed, but the condition should be checked before the loop.

4. Missing Validation in buildQueries
Location: internal/metrics/cloudwatch.go:74-95

No validation that spec fields are non-empty. If a renderer returns a MetricSpec with empty Namespace or MetricName, CloudWatch API will fail. Add validation:

func (f *Fetcher) buildQueries(resourceIDs []string, spec *render.MetricSpec) ([]types.MetricDataQuery, error) {
    if spec.Namespace == "" || spec.MetricName == "" {
        return nil, fmt.Errorf("invalid metric spec: namespace and metric name required")
    }
    // ...
}

5. Error Handling in Auto-Reload
Location: internal/view/resource_browser.go:379-381

When auto-reload triggers metric refresh, errors are logged but not surfaced to the user. Consider showing a subtle indicator when metrics fail to load, especially for permission issues.

Low Priority

6. go.mod Organization
Location: go.mod:86-87, 99

The PR moves cloudwatch from indirect to direct dependencies and adds golang.org/x/sync as direct, but golang.org/x/sync doesn't appear to be used in the new code. If it's not needed, remove it to keep dependencies clean.

7. Test: processResults Edge Case
Location: internal/metrics/cloudwatch_test.go:98-112

The test TestProcessResults with nil results is good, but missing a test for partial failures where some metric queries succeed and others return empty results (StatusCode != "Complete").

8. Column Width Calculation
Location: internal/view/resource_browser.go:762-767

The extra width distribution logic doesn't account for the metrics column when enabled. The last regular column gets extraWidth only when metrics are disabled, but when metrics are enabled, the metrics column gets all extraWidth. This is fine but could be more evenly distributed.

🔒 Security

No significant security concerns identified.

• Proper context timeout prevents resource exhaustion
• No credential handling in new code (relies on AWS SDK config)
• IAM documentation correctly specifies minimum permissions

📊 Performance Analysis

API Call Efficiency:

• 15-minute window with 1-minute resolution = 15 data points per resource
• With 100 resources: 100 queries, 1 API call (batch)
• With 1000 resources: 1000 queries, 2 API calls (500 each batch)
• Well-optimized for typical use cases

Potential Concern: Large accounts with thousands of instances could see:

• 10,000 instances = 20 API calls
• CloudWatch GetMetricData cost: ~$0.01 per 1,000 metrics
• With frequent refreshes, costs could accumulate

Recommendation: Consider adding a warning or limit when resource count > 1000.

🧪 Test Coverage Assessment

Coverage: Excellent (~94% estimated for new code)

Covered:

• ✅ Edge cases (nil, empty, single values)
• ✅ Batching logic
• ✅ Sparkline rendering with various data patterns
• ✅ Type conversions and data structures

Missing:

• ❌ Integration test with actual CloudWatch API (could use AWS SDK mocks)
• ❌ Test for metrics loading with concurrent resource updates
• ❌ Test for resource type switching while metrics are loading

📝 Suggestions for Follow-up

1. Caching: Consider caching metric data for 60 seconds to reduce API calls when toggling metrics on/off
2. Metric Selection: Allow users to choose which metric to display (e.g., CPU vs Network)
3. Color Coding: Add color to sparklines based on thresholds (green/yellow/red)
4. ECS/ELB Support: As mentioned in feat: extend inline metrics to ECS/ELB #47, multi-dimension support would be valuable

✨ Overall Assessment

Status: Approve with Minor Changes Recommended

This is a well-implemented feature that adds significant value to the tool. The code quality is high, test coverage is comprehensive, and the implementation shows careful consideration of performance and UX. The issues identified are mostly minor improvements rather than blockers.

Recommended Actions Before Merge:

1. Fix the query ID collision issue (High Priority GIFデモ #2)
2. Add spec validation in buildQueries (Medium Priority S3オブジェクトの作成・参照・更新 #4)
3. Remove unused golang.org/x/sync dependency if not needed (Low Priority EventBridgeルールスケジュール更新 #6)

Nice to Have:

• Address the memory optimization in batch processing
• Add integration tests with AWS SDK mocks

Great work! This feature will significantly enhance the resource browsing experience. 🎉