fix(collector): keep collecting temperatures without CPU power status

collector/fixtures/e2e-output-darwin.txt

@@ -159,7 +159,7 @@ node_scrape_collector_success{collector="netdev"} 1
 node_scrape_collector_success{collector="os"} 1
 node_scrape_collector_success{collector="powersupplyclass"} 1
 node_scrape_collector_success{collector="textfile"} 1
-node_scrape_collector_success{collector="thermal"} 0
+node_scrape_collector_success{collector="thermal"} 1
 node_scrape_collector_success{collector="time"} 1
 node_scrape_collector_success{collector="xfrm"} 1
 # HELP node_textfile_mtime_seconds Unixtime mtime of textfiles successfully read.

collector/thermal_darwin.go

@@ -62,6 +62,11 @@ type thermCollector struct {
 
 const thermal = "thermal"
 
+// errNoCPUPowerStatus is returned when the system does not report any CPU power
+// status. Apple Silicon does not implement IOPMCopyCPUPowerStatus, so this is an
+// expected condition on those systems rather than a failure.
+var errNoCPUPowerStatus = errors.New("no CPU power status has been recorded")
+
 func init() {
 	registerCollector(thermal, defaultEnabled, NewThermCollector)
 }
@@ -110,18 +115,25 @@ func NewThermCollector(logger *slog.Logger) (Collector, error) {
 
 func (c *thermCollector) Update(ch chan<- prometheus.Metric) error {
 	cpuPowerStatus, err := fetchCPUPowerStatus()
-	if err != nil {
+	switch {
+	case err == nil:
+		if value, ok := cpuPowerStatus[(string(C.kIOPMCPUPowerLimitSchedulerTimeKey))]; ok {
+			ch <- c.cpuSchedulerLimit.mustNewConstMetric(float64(value) / 100.0)
+		}
+		if value, ok := cpuPowerStatus[(string(C.kIOPMCPUPowerLimitProcessorCountKey))]; ok {
+			ch <- c.cpuAvailableCPU.mustNewConstMetric(float64(value))
+		}
+		if value, ok := cpuPowerStatus[(string(C.kIOPMCPUPowerLimitProcessorSpeedKey))]; ok {
+			ch <- c.cpuSpeedLimit.mustNewConstMetric(float64(value) / 100.0)
+		}
+	case errors.Is(err, errNoCPUPowerStatus):
+		// Apple Silicon does not report CPU power status. The temperature
+		// sensors collected below are still available, so this must not abort
+		// the collector.
+		c.logger.Debug("No CPU power status reported by the system, skipping CPU power metrics")
+	default:
 		return err
 	}
-	if value, ok := cpuPowerStatus[(string(C.kIOPMCPUPowerLimitSchedulerTimeKey))]; ok {
-		ch <- c.cpuSchedulerLimit.mustNewConstMetric(float64(value) / 100.0)
-	}
-	if value, ok := cpuPowerStatus[(string(C.kIOPMCPUPowerLimitProcessorCountKey))]; ok {
-		ch <- c.cpuAvailableCPU.mustNewConstMetric(float64(value))
-	}
-	if value, ok := cpuPowerStatus[(string(C.kIOPMCPUPowerLimitProcessorSpeedKey))]; ok {
-		ch <- c.cpuSpeedLimit.mustNewConstMetric(float64(value) / 100.0)
-	}
 
 	return c.updateTemperatures(ch)
 }
@@ -135,7 +147,7 @@ func fetchCPUPowerStatus() (map[string]int, error) {
 	}()
 
 	if C.kIOReturnNotFound == cfDictRef.ret {
-		return nil, errors.New("no CPU power status has been recorded")
+		return nil, errNoCPUPowerStatus
 	}
 
 	if C.kIOReturnSuccess != cfDictRef.ret {

collector/thermal_darwin_arm64.go

@@ -101,6 +101,16 @@ func (c *thermCollector) updateTemperatures(ch chan<- prometheus.Metric) error {
 	cfProdKey := C.CFStringCreateWithCString(C.kCFAllocatorDefault, prodKey, C.kCFStringEncodingUTF8)
 	defer C.CFRelease(C.CFTypeRef(cfProdKey))
 
+	// A sensor is identified only by its product name. Several services report
+	// the same name, either because the same sensor is listed more than once or
+	// because two distinct sensors share a name, and the services carry no
+	// property that tells them apart: RegistryID, UniqueID and SerialNumber are
+	// all unset here. Emitting the same label set twice makes the registry
+	// reject those samples and fail the whole scrape, so only the first reading
+	// for a name is reported.
+	seen := make(map[string]struct{}, int(count))
+	skipped := 0
+
 	for i := 0; i < int(count); i++ {
 		service := C.CFArrayGetValueAtIndex(services, C.CFIndex(i))
 
@@ -125,8 +135,19 @@ func (c *thermCollector) updateTemperatures(ch chan<- prometheus.Metric) error {
 			C.CFRelease(C.CFTypeRef(nameRef))
 		}
 
+		if _, duplicate := seen[name]; duplicate {
+			skipped++
+			continue
+		}
+		seen[name] = struct{}{}
+
 		ch <- c.temperature.mustNewConstMetric(float64(temp), name)
 	}
+
+	if skipped > 0 {
+		c.logger.Debug("Skipped thermal sensors reporting a duplicate name", "skipped", skipped, "reported", len(seen))
+	}
+
 	return nil
 }
 

collector/thermal_darwin_test.go

@@ -0,0 +1,89 @@
+// Copyright The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build !notherm && darwin && cgo
+
+package collector
+
+import (
+	"errors"
+	"io"
+	"log/slog"
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus"
+	dto "github.com/prometheus/client_model/go"
+)
+
+// Apple Silicon does not implement IOPMCopyCPUPowerStatus, so fetchCPUPowerStatus
+// reports errNoCPUPowerStatus there. That is an expected condition and must not
+// abort the collector, otherwise the temperature sensors, which are read after
+// the CPU power status, are never collected.
+func TestThermalUpdateWithoutCPUPowerStatus(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+
+	c, err := NewThermCollector(logger)
+	if err != nil {
+		t.Fatalf("failed to create collector: %v", err)
+	}
+
+	ch := make(chan prometheus.Metric, 1024)
+	err = c.Update(ch)
+	close(ch)
+
+	if errors.Is(err, errNoCPUPowerStatus) {
+		t.Fatal("Update returned errNoCPUPowerStatus; a system without CPU power status must still collect temperatures")
+	}
+	if err != nil {
+		t.Fatalf("Update failed: %v", err)
+	}
+
+	for range ch {
+	}
+}
+
+// Several IOHID services report the same sensor name and carry no property that
+// tells them apart, so the collector must report each name once. Duplicate label
+// sets are rejected by the registry and fail the whole scrape.
+func TestThermalTemperaturesAreUnique(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+
+	c, err := NewThermCollector(logger)
+	if err != nil {
+		t.Fatalf("failed to create collector: %v", err)
+	}
+
+	ch := make(chan prometheus.Metric, 4096)
+	if err := c.Update(ch); err != nil {
+		t.Fatalf("Update failed: %v", err)
+	}
+	close(ch)
+
+	seen := make(map[string]struct{})
+	for m := range ch {
+		var pb dto.Metric
+		if err := m.Write(&pb); err != nil {
+			t.Fatalf("cannot read metric: %v", err)
+		}
+
+		key := m.Desc().String()
+		for _, l := range pb.GetLabel() {
+			key += "," + l.GetName() + "=" + l.GetValue()
+		}
+
+		if _, duplicate := seen[key]; duplicate {
+			t.Errorf("duplicate metric collected: %s", key)
+		}
+		seen[key] = struct{}{}
+	}
+}

end-to-end-test.sh

@@ -279,7 +279,10 @@ generated_metrics="${tmpdir}/e2e-output.txt"
 for os in freebsd openbsd netbsd solaris dragonfly darwin; do
   if [ "${GOHOSTOS}" = "${os}" ]; then
     generated_metrics="${tmpdir}/e2e-output-${GOHOSTOS}.txt"
-    fixture_metrics="${fixture_metrics::-4}-${GOHOSTOS}.txt"
+    # Not "${fixture_metrics::-4}": a negative length needs bash 4.2, and macOS
+    # still ships bash 3.2, where the expansion fails and the Linux fixture is
+    # used instead.
+    fixture_metrics="${fixture_metrics%.txt}-${GOHOSTOS}.txt"
   fi
 done
 
@@ -390,6 +393,7 @@ non_deterministic_metrics=$(cat << METRICS
   node_network_receive_bytes_total
   node_network_receive_multicast_total
   node_network_transmit_multicast_total
+  node_thermal_temperature_celsius
   node_zfs_abdstats_linear_count_total
   node_zfs_abdstats_linear_data_bytes
   node_zfs_abdstats_scatter_chunk_waste_bytes