diff --git a/internal/rds/cloudwatch.go b/internal/rds/cloudwatch.go index 51e19bc..2690243 100644 --- a/internal/rds/cloudwatch.go +++ b/internal/rds/cloudwatch.go @@ -27,6 +27,10 @@ type MetricStats struct { // write I/O is a countersignal against downsizing even when CPU is low, // because burstable instance classes scale EBS bandwidth with size. AvgWriteIOPS float64 + // WO-18: AvgReadIOPS is average read IOPS over the window. Combined with + // AvgWriteIOPS, total IOPS is the activity signal that distinguishes a + // genuinely idle instance (pooled connections but no I/O) from an active one. + AvgReadIOPS float64 } // WO-7: enables tag-based exclusion in rds/scanner.go. @@ -70,7 +74,8 @@ func FetchInstanceMetrics(ctx context.Context, cw CloudWatchAPI, instanceID stri return nil, err } - // Fetch connection count + // WO-19: use Average for DatabaseConnections so TotalConns is the average + // connection count over the window, not a meaningless sum-of-daily-sums. connOut, err := cw.GetMetricStatistics(ctx, &cloudwatch.GetMetricStatisticsInput{ Namespace: aws.String("AWS/RDS"), MetricName: aws.String("DatabaseConnections"), @@ -80,7 +85,7 @@ func FetchInstanceMetrics(ctx context.Context, cw CloudWatchAPI, instanceID stri StartTime: aws.Time(start), EndTime: aws.Time(now), Period: aws.Int32(period), - Statistics: []cwtypes.Statistic{cwtypes.StatisticSum}, + Statistics: []cwtypes.Statistic{cwtypes.StatisticAverage}, }) if err != nil { return nil, err @@ -120,6 +125,23 @@ func FetchInstanceMetrics(ctx context.Context, cw CloudWatchAPI, instanceID stri return nil, err } + // WO-18: fetch read IOPS; combined with write IOPS, total IOPS is the + // activity signal for pooled-connection idle detection. + readOut, err := cw.GetMetricStatistics(ctx, &cloudwatch.GetMetricStatisticsInput{ + Namespace: aws.String("AWS/RDS"), + MetricName: aws.String("ReadIOPS"), + Dimensions: []cwtypes.Dimension{ + {Name: aws.String("DBInstanceIdentifier"), Value: aws.String(instanceID)}, + }, + StartTime: aws.Time(start), + EndTime: aws.Time(now), + Period: aws.Int32(period), + Statistics: []cwtypes.Statistic{cwtypes.StatisticAverage}, + }) + if err != nil { + return nil, err + } + stats := &MetricStats{} if len(cpuOut.Datapoints) > 0 { @@ -138,11 +160,18 @@ func FetchInstanceMetrics(ctx context.Context, cw CloudWatchAPI, instanceID stri stats.MaxCPU = maxVal } + // WO-19: average the Average datapoints, not sum the Sum datapoints. + var connSum float64 + var connCount int for _, dp := range connOut.Datapoints { - if dp.Sum != nil { - stats.TotalConns += *dp.Sum + if dp.Average != nil { + connSum += *dp.Average + connCount++ } } + if connCount > 0 { + stats.TotalConns = connSum / float64(connCount) + } // WO-17@v2: swap GROWTH, not presence. CloudWatch does not guarantee datapoint // ordering, so sort by timestamp before taking the first/last delta. @@ -161,6 +190,19 @@ func FetchInstanceMetrics(ctx context.Context, cw CloudWatchAPI, instanceID stri stats.AvgWriteIOPS = writeSum / float64(writeCount) } + // WO-18: average read IOPS across the window. + var readSum float64 + var readCount int + for _, dp := range readOut.Datapoints { + if dp.Average != nil { + readSum += *dp.Average + readCount++ + } + } + if readCount > 0 { + stats.AvgReadIOPS = readSum / float64(readCount) + } + return stats, nil } diff --git a/internal/rds/cloudwatch_test.go b/internal/rds/cloudwatch_test.go index 2811416..fa1ec75 100644 --- a/internal/rds/cloudwatch_test.go +++ b/internal/rds/cloudwatch_test.go @@ -124,7 +124,7 @@ func TestFetchInstanceMetricsMaxCPU(t *testing.T) { } // WO-17@v2: a flat swap profile is parked-page noise and must report zero growth. -// This is the exact live-account shape (media-view-prod, 8.38MB flat over 14d) +// This is the exact live-account shape (a write-heavy instance, 8.38MB flat over 14d) // that WO-16 wrongly read as memory pressure. func TestFetchInstanceMetricsFlatSwapIsNotGrowth(t *testing.T) { cw := newMockCWClient() @@ -158,7 +158,7 @@ func TestFetchInstanceMetricsDecliningSwap(t *testing.T) { } // WO-17@v2: genuinely growing swap reports the positive delta (live-account shape -// of saga-service-prod: 0.5MB -> 6.24MB). +// of a memory-pressured instance: 0.5MB -> 6.24MB). func TestFetchInstanceMetricsGrowingSwap(t *testing.T) { cw := newMockCWClient() cw.metrics["CPUUtilization"] = makeCPUDatapoints(8.0, 15.0, 14) @@ -230,3 +230,23 @@ func TestFetchInstanceMetricsWriteIOPS(t *testing.T) { t.Errorf("AvgWriteIOPS = %.2f, want ~138.88", stats.AvgWriteIOPS) } } + +// WO-18: average read IOPS is surfaced alongside write IOPS. +func TestFetchInstanceMetricsReadIOPS(t *testing.T) { + cw := newMockCWClient() + cw.metrics["CPUUtilization"] = makeCPUDatapoints(8.0, 15.0, 14) + cw.metrics["DatabaseConnections"] = makeConnDatapoints(50) + cw.metrics["WriteIOPS"] = makeWriteIOPSDatapoints(1.49, 14) + cw.metrics["ReadIOPS"] = makeWriteIOPSDatapoints(0.33, 14) + + stats, err := FetchInstanceMetrics(context.Background(), cw, "mydb", now, 14) + if err != nil { + t.Fatalf("error: %v", err) + } + if stats.AvgReadIOPS < 0.2 || stats.AvgReadIOPS > 0.4 { + t.Errorf("AvgReadIOPS = %.2f, want ~0.33", stats.AvgReadIOPS) + } + if stats.AvgWriteIOPS < 1.3 || stats.AvgWriteIOPS > 1.6 { + t.Errorf("AvgWriteIOPS = %.2f, want ~1.49", stats.AvgWriteIOPS) + } +} diff --git a/internal/rds/mock_test.go b/internal/rds/mock_test.go index 4ddd708..4a64832 100644 --- a/internal/rds/mock_test.go +++ b/internal/rds/mock_test.go @@ -157,10 +157,11 @@ func makeCPUDatapoints(avgCPU, maxCPU float64, count int) *cloudwatch.GetMetricS return &cloudwatch.GetMetricStatisticsOutput{Datapoints: dps} } -func makeConnDatapoints(totalConns float64) *cloudwatch.GetMetricStatisticsOutput { +// WO-19: DatabaseConnections now uses Average, not Sum. +func makeConnDatapoints(avgConns float64) *cloudwatch.GetMetricStatisticsOutput { return &cloudwatch.GetMetricStatisticsOutput{ Datapoints: []cwtypes.Datapoint{ - {Sum: &totalConns}, + {Average: &avgConns}, }, } } diff --git a/internal/rds/scanner.go b/internal/rds/scanner.go index 94c55ba..6be8806 100644 --- a/internal/rds/scanner.go +++ b/internal/rds/scanner.go @@ -25,6 +25,13 @@ const ( writeIOPSBusyThreshold = 50.0 ) +// WO-18: idleIOPSThreshold is the total average IOPS (read + write) below which +// an instance is treated as effectively idle even when connection pools hold +// connections open. Calibrated from live data: a genuinely dead instance +// (a dead-app instance) averages ~1.8 IOPS; the lowest-activity live instance +// (a low-traffic instance) averages ~4.0 IOPS. 5.0 sits between those two clusters. +const idleIOPSThreshold = 5.0 + // WO-17@v2: gradeOversized converts corroborating metrics into a confidence grade // plus human-readable countersignals. It never decides whether to emit — that // is the caller's job — it only reports how much the evidence agrees. @@ -229,19 +236,39 @@ func (s *RDSScanner) analyzeInstance(ctx context.Context, cfg database.ScanConfi if s.cw != nil && cfg.MetricDays > 0 { metrics, err := FetchInstanceMetrics(ctx, s.cw, inst.ID, s.now, cfg.MetricDays) if err == nil && metrics.HasData { - // Idle check: avg CPU < idle threshold AND zero connections - if metrics.AvgCPU < cfg.IdleCPU && metrics.TotalConns == 0 { + // WO-18: idle check — low CPU AND (zero connections OR near-zero IOPS). + totalIOPS := metrics.AvgReadIOPS + metrics.AvgWriteIOPS + // Connection pools hold connections open on dead apps, so TotalConns==0 + // alone misses them. Total IOPS below the threshold catches a pooled + // but effectively dead instance (e.g. a dead-app instance: 1.8 IOPS, 6.7 conns). + if metrics.AvgCPU < cfg.IdleCPU && (metrics.TotalConns == 0 || totalIOPS < idleIOPSThreshold) { + // WO-18: a zero-connection idle is confident; a pooled-connection + // idle is graded needs-review because a warm pool does not prove + // the app is permanently dead. + confidence := database.ConfidenceConfident + var countersignals []string + if metrics.TotalConns > 0 { + confidence = database.ConfidenceNeedsReview + countersignals = append(countersignals, fmt.Sprintf( + "%.0f pooled connections but %.1f total IOPS (verify app is decommissioned)", + metrics.TotalConns, totalIOPS)) + } findings = append(findings, database.Finding{ ID: database.FindingIdleInstance, Severity: database.SeverityHigh, ResourceType: database.ResourceInstance, ResourceID: inst.ID, Region: s.region, - Message: fmt.Sprintf("Instance idle for %d days (avg CPU %.1f%%, 0 connections)", cfg.MetricDays, metrics.AvgCPU), + Message: fmt.Sprintf("Instance idle for %d days (avg CPU %.1f%%, %.1f total IOPS)", cfg.MetricDays, metrics.AvgCPU, totalIOPS), EstimatedMonthlyWaste: monthlyCost, + Confidence: confidence, + Countersignals: countersignals, Metadata: map[string]any{ "avg_cpu": metrics.AvgCPU, "total_conns": metrics.TotalConns, + "total_iops": totalIOPS, + "read_iops": metrics.AvgReadIOPS, + "write_iops": metrics.AvgWriteIOPS, "metric_days": cfg.MetricDays, "instance_class": inst.Class, "engine": inst.Engine, diff --git a/internal/rds/scanner_test.go b/internal/rds/scanner_test.go index 0cb65e1..bdd1ce8 100644 --- a/internal/rds/scanner_test.go +++ b/internal/rds/scanner_test.go @@ -249,6 +249,80 @@ func TestScanIdleInstance(t *testing.T) { } } +// WO-18: zero-connection idle is graded confident. +func TestScanIdleInstanceZeroConnsIsConfident(t *testing.T) { + mock := newMockRDSClient() + mock.instances = []rdstypes.DBInstance{ + makeInstance("idle-db", "db.t3.small", "postgres", "17.2"), + } + cw := newMockCWClient() + cw.metrics["CPUUtilization"] = makeCPUDatapoints(2.0, 4.0, 14) + cw.metrics["DatabaseConnections"] = makeConnDatapoints(0) + + s := newTestScanner(mock, cw) + result := s.Scan(context.Background(), defaultCfg(), nil) + + findings := findByID(result.Findings, database.FindingIdleInstance) + if len(findings) != 1 { + t.Fatalf("expected 1 IDLE_INSTANCE, got %d", len(findings)) + } + if findings[0].Confidence != database.ConfidenceConfident { + t.Errorf("Confidence = %q, want %q for zero-connection idle", findings[0].Confidence, database.ConfidenceConfident) + } +} + +// WO-18: a pooled-connection instance with near-zero IOPS is flagged idle. +// Shape observed on a real account: ~1.8 total IOPS with ~7 pooled connections. +func TestScanIdleInstancePooledButDead(t *testing.T) { + mock := newMockRDSClient() + mock.instances = []rdstypes.DBInstance{ + makeInstance("dead-app-db", "db.t4g.small", "postgres", "13.4"), + } + cw := newMockCWClient() + cw.metrics["CPUUtilization"] = makeCPUDatapoints(2.0, 4.0, 14) + cw.metrics["DatabaseConnections"] = makeConnDatapoints(7) + cw.metrics["ReadIOPS"] = makeWriteIOPSDatapoints(0.33, 14) + cw.metrics["WriteIOPS"] = makeWriteIOPSDatapoints(1.49, 14) + + s := newTestScanner(mock, cw) + result := s.Scan(context.Background(), defaultCfg(), nil) + + findings := findByID(result.Findings, database.FindingIdleInstance) + if len(findings) != 1 { + t.Fatalf("expected 1 IDLE_INSTANCE for pooled-but-dead instance, got %d (TotalConns==0 rule alone would have missed this)", len(findings)) + } + if findings[0].Confidence != database.ConfidenceNeedsReview { + t.Errorf("Confidence = %q, want %q for pooled-connection idle", findings[0].Confidence, database.ConfidenceNeedsReview) + } + if len(findings[0].Countersignals) != 1 { + t.Fatalf("expected 1 countersignal, got %v", findings[0].Countersignals) + } + if !strings.Contains(findings[0].Countersignals[0], "pooled connections") { + t.Errorf("countersignal = %q, want it to name the pooled connections", findings[0].Countersignals[0]) + } +} + +// WO-18: a pooled-connection instance with active IOPS is NOT flagged idle. +// It should fall through to the OVERSIZED_INSTANCE check instead. +func TestScanIdleInstancePooledAndActiveNotIdle(t *testing.T) { + mock := newMockRDSClient() + mock.instances = []rdstypes.DBInstance{ + makeInstance("active-db", "db.t4g.small", "postgres", "17.2"), + } + cw := newMockCWClient() + cw.metrics["CPUUtilization"] = makeCPUDatapoints(8.0, 15.0, 14) + cw.metrics["DatabaseConnections"] = makeConnDatapoints(50) + cw.metrics["ReadIOPS"] = makeWriteIOPSDatapoints(106.0, 14) + cw.metrics["WriteIOPS"] = makeWriteIOPSDatapoints(8.0, 14) + + s := newTestScanner(mock, cw) + result := s.Scan(context.Background(), defaultCfg(), nil) + + if len(findByID(result.Findings, database.FindingIdleInstance)) != 0 { + t.Error("should not flag an active instance as idle just because CPU is low") + } +} + func TestScanOversizedInstance(t *testing.T) { mock := newMockRDSClient() mock.instances = []rdstypes.DBInstance{ @@ -347,7 +421,7 @@ func TestScanOversizedGrowingSwapNeedsReview(t *testing.T) { } // WO-17@v2: sustained write IOPS downgrades confidence (live-account shape of -// media-view-prod: 14.6% max CPU but 138.88 avg write IOPS). +// a write-heavy instance: 14.6% max CPU but 138.88 avg write IOPS). func TestScanOversizedHighWriteIOPSNeedsReview(t *testing.T) { mock := newMockRDSClient() mock.instances = []rdstypes.DBInstance{