From 3279e73b6c582baaca674ae7e32bb1c02b076561 Mon Sep 17 00:00:00 2001 From: SaiPisey2 Date: Fri, 31 Jul 2026 20:41:26 +0530 Subject: [PATCH 1/2] fix(collector): keep collecting temperatures without CPU power status Apple Silicon does not implement IOPMCopyCPUPowerStatus, so fetchCPUPowerStatus returns kIOReturnNotFound there. Update returned that error straight away, which aborted the collector before updateTemperatures ran, so no temperature metrics were collected at all. The error was also not ErrNoData, so it was logged at error level on every scrape. On an M5 Pro, Update emitted 0 metrics and failed, while updateTemperatures on its own returned 52 temperature metrics. Treat kIOReturnNotFound as a system that does not report CPU power status: skip the three CPU power metrics, log at debug level and carry on to the temperature sensors. Any other non-success return code is still returned as an error. Systems that do report CPU power status are unaffected. This does not make the CPU power metrics available on Apple Silicon, since the underlying API provides no data. It only stops their absence from suppressing the temperature metrics. Adds a regression test covering the case. Signed-off-by: SaiPisey2 --- collector/thermal_darwin.go | 34 ++++++++++++++------- collector/thermal_darwin_test.go | 52 ++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 collector/thermal_darwin_test.go diff --git a/collector/thermal_darwin.go b/collector/thermal_darwin.go index c55b9b2845..0a126cf2e3 100644 --- a/collector/thermal_darwin.go +++ b/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 { diff --git a/collector/thermal_darwin_test.go b/collector/thermal_darwin_test.go new file mode 100644 index 0000000000..511fb98cc4 --- /dev/null +++ b/collector/thermal_darwin_test.go @@ -0,0 +1,52 @@ +// 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" +) + +// 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 { + } +} From 1d0ecba155f5f82eed1c8eb61befe5a7c6753b9e Mon Sep 17 00:00:00 2001 From: SaiPisey2 Date: Wed, 26 Aug 2026 23:13:47 +0530 Subject: [PATCH 2/2] fix(collector): disambiguate colliding thermal sensor names Collecting temperatures on Apple Silicon surfaced a second problem that was previously unreachable, because Update returned before updateTemperatures ever ran. A sensor is labelled with its IOHID "Product" property, which is not unique. On an M5 Pro, 52 temperature-reporting services carry only 17 distinct product names, so the registry rejects the colliding samples and logs an error on every scrape. The collisions are of two kinds. Six "gas gauge battery" services are distinct sensors that merely share a name; they have distinct LocationIDs. The "PMU tdie*" services come in threes that share both the name and the LocationID, and report readings that differ by a few tenths of a degree, so they are separate sensing elements rather than the same one listed repeatedly. Dropping either kind loses readings. Following the approach taken for hwmon in #3646, read all sensors first, then qualify a name shared by several services with its location, falling back to the service registry ID when the location does not separate them either. Registry IDs are unique per service, which the OS guarantees and which held for every service observed here. Names that do not collide are left untouched. The endpoint now reports all 52 readings with unique label sets and no gather errors, where before the change 35 of them were rejected. Also update collector/fixtures/e2e-output-darwin.txt for node_scrape_collector_success{collector="thermal"}, which is now 1 because the collector no longer fails. resolveSensorNames is kept free of cgo so that the labelling rules are covered by table-driven tests, which matters because the CI runner reports no thermal sensors at all. Signed-off-by: SaiPisey2 --- collector/fixtures/e2e-output-darwin.txt | 2 +- collector/thermal_darwin_arm64.go | 92 ++++++++++++++++- collector/thermal_darwin_arm64_test.go | 121 +++++++++++++++++++++++ collector/thermal_darwin_test.go | 37 +++++++ 4 files changed, 247 insertions(+), 5 deletions(-) create mode 100644 collector/thermal_darwin_arm64_test.go diff --git a/collector/fixtures/e2e-output-darwin.txt b/collector/fixtures/e2e-output-darwin.txt index cf29fb473c..a968780066 100644 --- a/collector/fixtures/e2e-output-darwin.txt +++ b/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. diff --git a/collector/thermal_darwin_arm64.go b/collector/thermal_darwin_arm64.go index 24558a1c9d..7722a0df4a 100644 --- a/collector/thermal_darwin_arm64.go +++ b/collector/thermal_darwin_arm64.go @@ -39,10 +39,12 @@ CFArrayRef IOHIDEventSystemClientCopyServices(IOHIDEventSystemClientRef client); IOHIDEventRef IOHIDServiceClientCopyEvent(IOHIDServiceClientRef service, int64_t type, int32_t options, int64_t timestamp); double IOHIDEventGetFloatValue(IOHIDEventRef event, int32_t field); CFTypeRef IOHIDServiceClientCopyProperty(IOHIDServiceClientRef service, CFStringRef key); +uint64_t IOHIDServiceClientGetRegistryID(IOHIDServiceClientRef service); */ import "C" import ( + "strconv" "unsafe" "github.com/prometheus/client_golang/prometheus" @@ -50,6 +52,18 @@ import ( const absoluteZeroCelsius = -273.15 +// A thermal sensor is labelled with its IOHID "Product" property, which is not +// unique. Apple Silicon reports several services under one product name: some +// are distinct sensors that only share the name and can be told apart by their +// location, while others share the location too and differ only by the registry +// ID of the service. +type thermalSensor struct { + name string + location string + registryID string + temp float64 +} + func (c *thermCollector) updateTemperatures(ch chan<- prometheus.Metric) error { client := C.IOHIDEventSystemClientCreate(C.kCFAllocatorDefault) if client == nil { @@ -101,10 +115,18 @@ func (c *thermCollector) updateTemperatures(ch chan<- prometheus.Metric) error { cfProdKey := C.CFStringCreateWithCString(C.kCFAllocatorDefault, prodKey, C.kCFStringEncodingUTF8) defer C.CFRelease(C.CFTypeRef(cfProdKey)) + locKey := C.CString("LocationID") + defer C.free(unsafe.Pointer(locKey)) + cfLocKey := C.CFStringCreateWithCString(C.kCFAllocatorDefault, locKey, C.kCFStringEncodingUTF8) + defer C.CFRelease(C.CFTypeRef(cfLocKey)) + + // Read every sensor first, so that colliding product names can be detected + // before any metric is emitted. + sensors := make([]thermalSensor, 0, int(count)) for i := 0; i < int(count); i++ { - service := C.CFArrayGetValueAtIndex(services, C.CFIndex(i)) + service := (C.IOHIDServiceClientRef)(C.CFArrayGetValueAtIndex(services, C.CFIndex(i))) - event := C.IOHIDServiceClientCopyEvent((C.IOHIDServiceClientRef)(service), C.kIOHIDEventTypeTemperature, 0, 0) + event := C.IOHIDServiceClientCopyEvent(service, C.kIOHIDEventTypeTemperature, 0, 0) if event == nil { continue } @@ -118,18 +140,80 @@ func (c *thermCollector) updateTemperatures(ch chan<- prometheus.Metric) error { continue } - nameRef := C.IOHIDServiceClientCopyProperty((C.IOHIDServiceClientRef)(service), cfProdKey) + nameRef := C.IOHIDServiceClientCopyProperty(service, cfProdKey) name := "Unknown" if nameRef != 0 { name = cfStringToString((C.CFStringRef)(nameRef)) C.CFRelease(C.CFTypeRef(nameRef)) } - ch <- c.temperature.mustNewConstMetric(float64(temp), name) + sensors = append(sensors, thermalSensor{ + name: name, + location: serviceNumberProperty(service, cfLocKey), + registryID: strconv.FormatUint(uint64(C.IOHIDServiceClientGetRegistryID(service)), 10), + temp: float64(temp), + }) } + + for i, label := range resolveSensorNames(sensors) { + ch <- c.temperature.mustNewConstMetric(sensors[i].temp, label) + } + return nil } +// resolveSensorNames returns the sensor label to report for each sensor, in the +// order the sensors were given. +// +// Emitting the same label set twice makes the registry reject the samples and +// fail the whole scrape. A product name shared by several services is therefore +// qualified with the service location, falling back to the registry ID, which is +// unique per service, when the location does not tell them apart either. +func resolveSensorNames(sensors []thermalSensor) []string { + nameCount := make(map[string]int, len(sensors)) + nameLocationCount := make(map[string]int, len(sensors)) + for _, s := range sensors { + nameCount[s.name]++ + nameLocationCount[s.name+"\x00"+s.location]++ + } + + labels := make([]string, 0, len(sensors)) + for _, s := range sensors { + label := s.name + if nameCount[s.name] > 1 { + suffix := s.location + if suffix == "" || nameLocationCount[s.name+"\x00"+s.location] > 1 { + suffix = s.registryID + } + label = s.name + "_" + suffix + } + labels = append(labels, label) + } + + return labels +} + +// serviceNumberProperty reads a numeric IOHID service property, returning an +// empty string when it is absent or not a number. +func serviceNumberProperty(service C.IOHIDServiceClientRef, key C.CFStringRef) string { + ref := C.IOHIDServiceClientCopyProperty(service, key) + if ref == 0 { + return "" + } + defer C.CFRelease(ref) + + if C.CFGetTypeID(ref) != C.CFNumberGetTypeID() { + return "" + } + + var value C.longlong + if C.CFNumberGetValue(C.CFNumberRef(ref), C.kCFNumberLongLongType, unsafe.Pointer(&value)) == 0 { + return "" + } + + return strconv.FormatInt(int64(value), 10) +} + func cfStringToString(s C.CFStringRef) string { p := C.CFStringGetCStringPtr(s, C.kCFStringEncodingUTF8) if p != nil { diff --git a/collector/thermal_darwin_arm64_test.go b/collector/thermal_darwin_arm64_test.go new file mode 100644 index 0000000000..ba3e867a8b --- /dev/null +++ b/collector/thermal_darwin_arm64_test.go @@ -0,0 +1,121 @@ +// 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 && arm64 && cgo + +package collector + +import "testing" + +func TestResolveSensorNames(t *testing.T) { + for _, tc := range []struct { + name string + sensors []thermalSensor + want []string + }{ + { + name: "unique names are left alone", + sensors: []thermalSensor{{name: "NAND CH0 temp", location: "1", registryID: "10"}}, + want: []string{"NAND CH0 temp"}, + }, + { + // Several "gas gauge battery" services are distinct sensors that + // only share a product name. + name: "a shared name is qualified with the location", + sensors: []thermalSensor{ + {name: "gas gauge battery", location: "1413951555", registryID: "10"}, + {name: "gas gauge battery", location: "1413951574", registryID: "11"}, + }, + want: []string{"gas gauge battery_1413951555", "gas gauge battery_1413951574"}, + }, + { + // The PMU sensors report the same product name and the same + // location, so only the registry ID separates them. + name: "a shared location falls back to the registry ID", + sensors: []thermalSensor{ + {name: "PMU tdie2", location: "1414541922", registryID: "10"}, + {name: "PMU tdie2", location: "1414541922", registryID: "11"}, + {name: "PMU tdie2", location: "1414541922", registryID: "12"}, + }, + want: []string{"PMU tdie2_10", "PMU tdie2_11", "PMU tdie2_12"}, + }, + { + name: "a missing location falls back to the registry ID", + sensors: []thermalSensor{ + {name: "sensor", location: "", registryID: "10"}, + {name: "sensor", location: "", registryID: "11"}, + }, + want: []string{"sensor_10", "sensor_11"}, + }, + { + // Only the services that cannot be told apart by location fall back + // to the registry ID. + name: "location and registry ID are mixed within one name", + sensors: []thermalSensor{ + {name: "sensor", location: "1", registryID: "10"}, + {name: "sensor", location: "2", registryID: "11"}, + {name: "sensor", location: "2", registryID: "12"}, + }, + want: []string{"sensor_1", "sensor_11", "sensor_12"}, + }, + { + name: "no sensors", + sensors: nil, + want: []string{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := resolveSensorNames(tc.sensors) + if len(got) != len(tc.want) { + t.Fatalf("got %d labels, want %d: %q", len(got), len(tc.want), got) + } + for i := range tc.want { + if got[i] != tc.want[i] { + t.Errorf("label %d: got %q, want %q", i, got[i], tc.want[i]) + } + } + + seen := make(map[string]struct{}, len(got)) + for _, l := range got { + if _, dup := seen[l]; dup { + t.Errorf("duplicate label %q", l) + } + seen[l] = struct{}{} + } + }) + } +} + +// Every reading must be reported. Sensors sharing a product name are +// disambiguated rather than dropped. +func TestResolveSensorNamesKeepsEveryReading(t *testing.T) { + sensors := []thermalSensor{ + {name: "a", location: "1", registryID: "10"}, + {name: "a", location: "1", registryID: "11"}, + {name: "a", location: "2", registryID: "12"}, + {name: "b", location: "1", registryID: "13"}, + } + + got := resolveSensorNames(sensors) + if len(got) != len(sensors) { + t.Fatalf("got %d labels for %d sensors", len(got), len(sensors)) + } + + seen := make(map[string]struct{}, len(got)) + for _, l := range got { + if _, dup := seen[l]; dup { + t.Errorf("duplicate label %q", l) + } + seen[l] = struct{}{} + } +} diff --git a/collector/thermal_darwin_test.go b/collector/thermal_darwin_test.go index 511fb98cc4..5b736169f0 100644 --- a/collector/thermal_darwin_test.go +++ b/collector/thermal_darwin_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" ) // Apple Silicon does not implement IOPMCopyCPUPowerStatus, so fetchCPUPowerStatus @@ -50,3 +51,39 @@ func TestThermalUpdateWithoutCPUPowerStatus(t *testing.T) { for range ch { } } + +// Several IOHID services report the same product name, so the collector must +// qualify the colliding ones. 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{}{} + } +}