From 5e1b0281304ca6db3c29f50881523bf8173ac793 Mon Sep 17 00:00:00 2001 From: henrygd Date: Sun, 8 Mar 2026 15:19:50 -0400 Subject: [PATCH] refactor(smart): improve perf by skipping ata_device_statistics parsing if unnecessary --- agent/smart.go | 45 ++++--- agent/smart_test.go | 211 +++++++++++++++++++++++++++++++ internal/entities/smart/smart.go | 4 +- 3 files changed, 237 insertions(+), 23 deletions(-) diff --git a/agent/smart.go b/agent/smart.go index c2598518..5fbb2c64 100644 --- a/agent/smart.go +++ b/agent/smart.go @@ -871,15 +871,18 @@ func (sm *SmartManager) parseSmartForSata(output []byte) (bool, int) { smartData.FirmwareVersion = data.FirmwareVersion smartData.Capacity = data.UserCapacity.Bytes smartData.Temperature = data.Temperature.Current - if smartData.Temperature == 0 { - if temp, ok := temperatureFromAtaDeviceStatistics(data.AtaDeviceStatistics); ok { - smartData.Temperature = temp - } - } smartData.SmartStatus = getSmartStatus(smartData.Temperature, data.SmartStatus.Passed) smartData.DiskName = data.Device.Name smartData.DiskType = data.Device.Type + // get values from ata_device_statistics if necessary + var ataDeviceStats smart.AtaDeviceStatistics + if smartData.Temperature == 0 { + if temp := findAtaDeviceStatisticsValue(&data, &ataDeviceStats, 5, "Current Temperature", 0, 255); temp != nil { + smartData.Temperature = uint8(*temp) + } + } + // update SmartAttributes smartData.Attributes = make([]*smart.SmartAttribute, 0, len(data.AtaSmartAttributes.Table)) for _, attr := range data.AtaSmartAttributes.Table { @@ -914,23 +917,20 @@ func getSmartStatus(temperature uint8, passed bool) string { } } -func temperatureFromAtaDeviceStatistics(stats smart.AtaDeviceStatistics) (uint8, bool) { - entry := findAtaDeviceStatisticsEntry(stats, 5, "Current Temperature") - if entry == nil || entry.Value == nil { - return 0, false - } - if *entry.Value < 0 || *entry.Value > 255 { - return 0, false - } - return uint8(*entry.Value), true -} - // findAtaDeviceStatisticsEntry centralizes ATA devstat lookups so additional // metrics can be pulled from the same structure in the future. -func findAtaDeviceStatisticsEntry(stats smart.AtaDeviceStatistics, pageNumber uint8, entryName string) *smart.AtaDeviceStatisticsEntry { - for pageIdx := range stats.Pages { - page := &stats.Pages[pageIdx] - if page.Number != pageNumber { +func findAtaDeviceStatisticsValue(data *smart.SmartInfoForSata, ataDeviceStats *smart.AtaDeviceStatistics, entryNumber uint8, entryName string, minValue, maxValue int64) *int64 { + if len(ataDeviceStats.Pages) == 0 { + if len(data.AtaDeviceStatistics) == 0 { + return nil + } + if err := json.Unmarshal(data.AtaDeviceStatistics, ataDeviceStats); err != nil { + return nil + } + } + for pageIdx := range ataDeviceStats.Pages { + page := &ataDeviceStats.Pages[pageIdx] + if page.Number != entryNumber { continue } for entryIdx := range page.Table { @@ -938,7 +938,10 @@ func findAtaDeviceStatisticsEntry(stats smart.AtaDeviceStatistics, pageNumber ui if !strings.EqualFold(entry.Name, entryName) { continue } - return entry + if entry.Value == nil || *entry.Value < minValue || *entry.Value > maxValue { + return nil + } + return entry.Value } } return nil diff --git a/agent/smart_test.go b/agent/smart_test.go index 8ff7c6df..478e96e0 100644 --- a/agent/smart_test.go +++ b/agent/smart_test.go @@ -121,6 +121,41 @@ func TestParseSmartForSataDeviceStatisticsTemperature(t *testing.T) { assert.Equal(t, uint8(22), deviceData.Temperature) } +func TestParseSmartForSataAtaDeviceStatistics(t *testing.T) { + // tests that ata_device_statistics values are parsed correctly + jsonPayload := []byte(`{ + "smartctl": {"exit_status": 0}, + "device": {"name": "/dev/sdb", "type": "sat"}, + "model_name": "SanDisk SSD U110 16GB", + "serial_number": "lksjfh23lhj", + "firmware_version": "U21B001", + "user_capacity": {"bytes": 16013942784}, + "smart_status": {"passed": true}, + "ata_smart_attributes": {"table": []}, + "ata_device_statistics": { + "pages": [ + { + "number": 5, + "name": "Temperature Statistics", + "table": [ + {"name": "Current Temperature", "value": 43, "flags": {"valid": true}}, + {"name": "Specified Minimum Operating Temperature", "value": -20, "flags": {"valid": true}} + ] + } + ] + } + }`) + + sm := &SmartManager{SmartDataMap: make(map[string]*smart.SmartData)} + hasData, exitStatus := sm.parseSmartForSata(jsonPayload) + require.True(t, hasData) + assert.Equal(t, 0, exitStatus) + + deviceData, ok := sm.SmartDataMap["lksjfh23lhj"] + require.True(t, ok, "expected smart data entry for serial lksjfh23lhj") + assert.Equal(t, uint8(43), deviceData.Temperature) +} + func TestParseSmartForSataNegativeDeviceStatistics(t *testing.T) { // Tests that negative values in ata_device_statistics (e.g. min operating temp) // do not cause the entire SAT parser to fail. @@ -764,6 +799,182 @@ func TestIsVirtualDeviceScsi(t *testing.T) { } } +func TestFindAtaDeviceStatisticsValue(t *testing.T) { + val42 := int64(42) + val100 := int64(100) + valMinus20 := int64(-20) + + tests := []struct { + name string + data smart.SmartInfoForSata + ataDeviceStats smart.AtaDeviceStatistics + entryNumber uint8 + entryName string + minValue int64 + maxValue int64 + expectedValue *int64 + }{ + { + name: "value in ataDeviceStats", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 5, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "Current Temperature", Value: &val42}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 100, + expectedValue: &val42, + }, + { + name: "value unmarshaled from data", + data: smart.SmartInfoForSata{ + AtaDeviceStatistics: []byte(`{"pages":[{"number":5,"table":[{"name":"Current Temperature","value":100}]}]}`), + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 255, + expectedValue: &val100, + }, + { + name: "value out of range (too high)", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 5, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "Current Temperature", Value: &val100}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 50, + expectedValue: nil, + }, + { + name: "value out of range (too low)", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 5, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "Min Temp", Value: &valMinus20}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Min Temp", + minValue: 0, + maxValue: 100, + expectedValue: nil, + }, + { + name: "no statistics available", + data: smart.SmartInfoForSata{}, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 255, + expectedValue: nil, + }, + { + name: "wrong page number", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 1, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "Current Temperature", Value: &val42}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 100, + expectedValue: nil, + }, + { + name: "wrong entry name", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 5, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "Other Stat", Value: &val42}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 100, + expectedValue: nil, + }, + { + name: "case insensitive name match", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 5, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "CURRENT TEMPERATURE", Value: &val42}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 100, + expectedValue: &val42, + }, + { + name: "entry value is nil", + ataDeviceStats: smart.AtaDeviceStatistics{ + Pages: []smart.AtaDeviceStatisticsPage{ + { + Number: 5, + Table: []smart.AtaDeviceStatisticsEntry{ + {Name: "Current Temperature", Value: nil}, + }, + }, + }, + }, + entryNumber: 5, + entryName: "Current Temperature", + minValue: 0, + maxValue: 100, + expectedValue: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := findAtaDeviceStatisticsValue(&tt.data, &tt.ataDeviceStats, tt.entryNumber, tt.entryName, tt.minValue, tt.maxValue) + if tt.expectedValue == nil { + assert.Nil(t, result) + } else { + require.NotNil(t, result) + assert.Equal(t, *tt.expectedValue, *result) + } + }) + } +} + func TestRefreshExcludedDevices(t *testing.T) { tests := []struct { name string diff --git a/internal/entities/smart/smart.go b/internal/entities/smart/smart.go index cda462e4..42a64df2 100644 --- a/internal/entities/smart/smart.go +++ b/internal/entities/smart/smart.go @@ -356,8 +356,8 @@ type SmartInfoForSata struct { SmartStatus SmartStatusInfo `json:"smart_status"` // AtaSmartData AtaSmartData `json:"ata_smart_data"` // AtaSctCapabilities AtaSctCapabilities `json:"ata_sct_capabilities"` - AtaSmartAttributes AtaSmartAttributes `json:"ata_smart_attributes"` - AtaDeviceStatistics AtaDeviceStatistics `json:"ata_device_statistics"` + AtaSmartAttributes AtaSmartAttributes `json:"ata_smart_attributes"` + AtaDeviceStatistics json.RawMessage `json:"ata_device_statistics"` // PowerOnTime PowerOnTimeInfo `json:"power_on_time"` // PowerCycleCount uint16 `json:"power_cycle_count"` Temperature TemperatureInfo `json:"temperature"`