Align battery health reporting (#22569)

changes/19619-align-battery-health

@@ -0,0 +1 @@
+- battery health definitions now defined as cycle counts greater than 1000 or max capacity falling under 80% of designed capacity for macOS and Windows

docs/Contributing/Understanding-host-vitals.md

@@ -3,18 +3,9 @@
 
 Following is a summary of the detail queries hardcoded in Fleet used to populate the device details:
 
-## battery_macos
+## battery
 
-- Platforms: darwin
-
-- Query:
-```sql
-SELECT serial_number, cycle_count, health FROM battery;
-```
-
-## battery_windows
-
-- Platforms: windows
+- Platforms: windows, darwin
 
 - Discovery query:
 ```sql

server/service/hosts.go

@@ -1152,18 +1152,6 @@ func (svc *Service) getHostDetails(ctx context.Context, host *fleet.Host, opts f
 		nextMw.StartsAt = nextMw.StartsAt.In(gCalLoc)
 	}
 
-	// Due to a known osquery issue with M1 Macs, we are ignoring the stored value in the db
-	// and replacing it at the service layer with custom values determined by the cycle count.
-	// See https://github.com/fleetdm/fleet/issues/6763.
-	// TODO: Update once the underlying osquery issue has been resolved.
-	for _, b := range bats {
-		if b.CycleCount < 1000 {
-			b.Health = "Normal"
-		} else {
-			b.Health = "Replacement recommended"
-		}
-	}
-
 	var policies *[]*fleet.HostPolicy
 	if opts.IncludePolicies {
 		hp, err := svc.ds.ListPoliciesForHost(ctx, host)

server/service/hosts_test.go

@@ -65,7 +65,7 @@ func TestHostDetails(t *testing.T) {
 	ds.ListPoliciesForHostFunc = func(ctx context.Context, host *fleet.Host) ([]*fleet.HostPolicy, error) {
 		return nil, nil
 	}
-	dsBats := []*fleet.HostBattery{{HostID: host.ID, SerialNumber: "a", CycleCount: 999, Health: "Check Battery"}, {HostID: host.ID, SerialNumber: "b", CycleCount: 1001, Health: "Good"}}
+	dsBats := []*fleet.HostBattery{{HostID: host.ID, SerialNumber: "a", CycleCount: 999, Health: "Normal"}, {HostID: host.ID, SerialNumber: "b", CycleCount: 1001, Health: "Service recommended"}}
 	ds.ListHostBatteriesFunc = func(ctx context.Context, hostID uint) ([]*fleet.HostBattery, error) {
 		return dsBats, nil
 	}
@@ -75,8 +75,6 @@ func TestHostDetails(t *testing.T) {
 	ds.GetHostLockWipeStatusFunc = func(ctx context.Context, host *fleet.Host) (*fleet.HostLockWipeStatus, error) {
 		return &fleet.HostLockWipeStatus{}, nil
 	}
-	// Health should be replaced at the service layer with custom values determined by the cycle count. See https://github.com/fleetdm/fleet/issues/6763.
-	expectedBats := []*fleet.HostBattery{{HostID: host.ID, SerialNumber: "a", CycleCount: 999, Health: "Normal"}, {HostID: host.ID, SerialNumber: "b", CycleCount: 1001, Health: "Replacement recommended"}}
 
 	opts := fleet.HostDetailOptions{
 		IncludeCVEScores: false,
@@ -87,7 +85,7 @@ func TestHostDetails(t *testing.T) {
 	assert.Equal(t, expectedLabels, hostDetail.Labels)
 	assert.Equal(t, expectedPacks, hostDetail.Packs)
 	require.NotNil(t, hostDetail.Batteries)
-	assert.Equal(t, expectedBats, *hostDetail.Batteries)
+	assert.Equal(t, dsBats, *hostDetail.Batteries)
 	require.Nil(t, hostDetail.MDM.MacOSSettings)
 }
 

server/service/integration_core_test.go

@@ -8327,8 +8327,8 @@ func (s *integrationTestSuite) TestGetHostBatteries() {
 	require.NoError(t, err)
 
 	bats := []*fleet.HostBattery{
-		{HostID: host.ID, SerialNumber: "a", CycleCount: 1, Health: "Good"},
-		{HostID: host.ID, SerialNumber: "b", CycleCount: 1002, Health: "Poor"},
+		{HostID: host.ID, SerialNumber: "a", CycleCount: 1, Health: "Normal"},
+		{HostID: host.ID, SerialNumber: "b", CycleCount: 1002, Health: "Service recommended"},
 	}
 	require.NoError(t, s.ds.ReplaceHostBatteries(context.Background(), host.ID, bats))
 
@@ -8338,7 +8338,7 @@ func (s *integrationTestSuite) TestGetHostBatteries() {
 	// only cycle count and health are returned
 	require.ElementsMatch(t, []*fleet.HostBattery{
 		{CycleCount: 1, Health: "Normal"},
-		{CycleCount: 1002, Health: "Replacement recommended"},
+		{CycleCount: 1002, Health: "Service recommended"},
 	}, *getHostResp.Host.Batteries)
 
 	// same for get host by identifier
@@ -8347,7 +8347,7 @@ func (s *integrationTestSuite) TestGetHostBatteries() {
 	// only cycle count and health are returned
 	require.ElementsMatch(t, []*fleet.HostBattery{
 		{CycleCount: 1, Health: "Normal"},
-		{CycleCount: 1002, Health: "Replacement recommended"},
+		{CycleCount: 1002, Health: "Service recommended"},
 	}, *getHostResp.Host.Batteries)
 }
 

server/service/integration_desktop_test.go

@@ -38,7 +38,7 @@ func (s *integrationTestSuite) TestDeviceAuthenticatedEndpoints() {
 	require.NoError(t, s.ds.SetOrUpdateMunkiInfo(context.Background(), hosts[0].ID, "1.3.0", nil, nil))
 	// create a battery for hosts[0]
 	require.NoError(t, s.ds.ReplaceHostBatteries(context.Background(), hosts[0].ID, []*fleet.HostBattery{
-		{HostID: hosts[0].ID, SerialNumber: "a", CycleCount: 1, Health: "Good"},
+		{HostID: hosts[0].ID, SerialNumber: "a", CycleCount: 1, Health: "Normal"},
 	}))
 
 	// create an auth token for hosts[0]

server/service/osquery_test.go

@@ -1062,7 +1062,7 @@ func verifyDiscovery(t *testing.T, queries, discovery map[string]string) {
 		hostDetailQueryPrefix + "orbit_info":                 {},
 		hostDetailQueryPrefix + "software_vscode_extensions": {},
 		hostDetailQueryPrefix + "software_macos_firefox":     {},
-		hostDetailQueryPrefix + "battery_windows":            {},
+		hostDetailQueryPrefix + "battery":                    {},
 	}
 	for name := range queries {
 		require.NotEmpty(t, discovery[name])

server/service/osquery_utils/queries.go

@@ -555,17 +555,13 @@ var extraDetailQueries = map[string]DetailQuery{
 		DirectIngestFunc: directIngestChromeProfiles,
 		Discovery:        discoveryTable("google_chrome_profiles"),
 	},
-	"battery_macos": {
-		Query:            `SELECT serial_number, cycle_count, health FROM battery;`,
-		Platforms:        []string{"darwin"},
-		DirectIngestFunc: directIngestBattery,
-		// the "battery" table doesn't need a Discovery query as it is an official
-		// osquery table on darwin (https://osquery.io/schema/5.3.0#battery), it is
-		// always present.
-	},
-	"battery_windows": {
+	"battery": {
+		// This query is used to determine battery health of macOS and Windows hosts
+		// based on the cycle count, designed capacity, and max capacity of the battery.
+		// The `health` column is ommitted due to a known osquery issue with M1 Macs
+		// (https://github.com/fleetdm/fleet/issues/6763) and its absence on Windows.
 		Query:            `SELECT serial_number, cycle_count, designed_capacity, max_capacity FROM battery`,
-		Platforms:        []string{"windows"},
+		Platforms:        []string{"windows", "darwin"},
 		DirectIngestFunc: directIngestBattery,
 		Discovery:        discoveryTable("battery"), // added to Windows in v5.12.1 (https://github.com/osquery/osquery/releases/tag/5.12.1)
 	},
@@ -1300,71 +1296,79 @@ func directIngestChromeProfiles(ctx context.Context, logger log.Logger, host *fl
 	return ds.ReplaceHostDeviceMapping(ctx, host.ID, mapping, fleet.DeviceMappingGoogleChromeProfiles)
 }
 
+// directIngestBattery ingests battery data from a host on a Windows or macOS platform
+// and calculates the battery health based on cycle count and capacity.
+// Due to a known osquery issue with M1 Macs (https://github.com/fleetdm/fleet/issues/6763)
+// and the ommission of the `health` column on Windows, we are not leveraging the `health`
+// column in the query and instead aligning the definition of battery health between
+// macOS and Windows.
 func directIngestBattery(ctx context.Context, logger log.Logger, host *fleet.Host, ds fleet.Datastore, rows []map[string]string) error {
 	mapping := make([]*fleet.HostBattery, 0, len(rows))
+
 	for _, row := range rows {
-		cycleCount, err := strconv.Atoi(EmptyToZero(row["cycle_count"]))
+		health, cycleCount, err := generateBatteryHealth(row, logger)
 		if err != nil {
-			return err
+			level.Error(logger).Log("op", "directIngestBattery", "hostID", host.ID, "err", err)
 		}
 
-		switch host.Platform {
-		case "darwin":
-			mapping = append(mapping, &fleet.HostBattery{
-				HostID:       host.ID,
-				SerialNumber: row["serial_number"],
-				CycleCount:   cycleCount,
-				// database type is VARCHAR(40) and since there isn't a
-				// canonical list of strings we can get for health, we
-				// truncate the value just in case.
-				Health: fmt.Sprintf("%.40s", row["health"]),
-			})
-		case "windows":
-			health, err := generateWindowsBatteryHealth(row["designed_capacity"], row["max_capacity"])
-			if err != nil {
-				level.Error(logger).Log("op", "directIngestBattery", "hostID", host.ID, "err", err)
-			}
-
-			mapping = append(mapping, &fleet.HostBattery{
-				HostID:       host.ID,
-				SerialNumber: row["serial_number"],
-				CycleCount:   cycleCount,
-				Health:       health,
-			})
-		}
+		mapping = append(mapping, &fleet.HostBattery{
+			HostID:       host.ID,
+			SerialNumber: row["serial_number"],
+			CycleCount:   cycleCount,
+			Health:       health,
+		})
 	}
+
 	return ds.ReplaceHostBatteries(ctx, host.ID, mapping)
 }
 
 const (
-	batteryStatusUnknown     = "Unknown"
-	batteryStatusDegraded    = "Check Battery"
-	batteryStatusGood        = "Good"
-	batteryDegradedThreshold = 80
+	batteryStatusUnknown      = "Unknown"
+	batteryStatusDegraded     = "Service recommended"
+	batteryStatusGood         = "Normal"
+	batteryDegradedThreshold  = 80
+	batteryDegradedCycleCount = 1000
 )
 
-func generateWindowsBatteryHealth(designedCapacity, maxCapacity string) (string, error) {
+// generateBatteryHealth calculates the battery health based on the cycle count and capacity.
+func generateBatteryHealth(row map[string]string, logger log.Logger) (string, int, error) {
+	designedCapacity := row["designed_capacity"]
+	maxCapacity := row["max_capacity"]
+	cycleCount := row["cycle_count"]
+
+	count, err := strconv.Atoi(EmptyToZero(cycleCount))
+	if err != nil {
+		level.Error(logger).Log("op", "generateBatteryHealth", "err", err)
+		// If we can't parse the cycle count, we'll assume it's 0
+		// and continue with the rest of the battery health check.
+		count = 0
+	}
+
+	if count >= batteryDegradedCycleCount {
+		return batteryStatusDegraded, count, nil
+	}
+
 	if designedCapacity == "" || maxCapacity == "" {
-		return batteryStatusUnknown, fmt.Errorf("missing battery capacity values, designed: %s, max: %s", designedCapacity, maxCapacity)
+		return batteryStatusUnknown, count, fmt.Errorf("missing battery capacity values, designed: %s, max: %s", designedCapacity, maxCapacity)
 	}
 
 	designed, err := strconv.ParseInt(designedCapacity, 10, 64)
 	if err != nil {
-		return batteryStatusUnknown, err
+		return batteryStatusUnknown, count, fmt.Errorf("failed to parse designed capacity: %s", designedCapacity)
 	}
 
 	max, err := strconv.ParseInt(maxCapacity, 10, 64)
 	if err != nil {
-		return batteryStatusUnknown, err
+		return batteryStatusUnknown, count, fmt.Errorf("failed to parse max capacity: %s", maxCapacity)
 	}
 
 	health := float64(max) / float64(designed) * 100
 
 	if health < batteryDegradedThreshold {
-		return batteryStatusDegraded, nil
+		return batteryStatusDegraded, count, nil
 	}
 
-	return batteryStatusGood, nil
+	return batteryStatusGood, count, nil
 }
 
 func directIngestWindowsUpdateHistory(