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.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_arm64.go b/collector/thermal_darwin_arm64.go index 24558a1c9d..7bc0bf1317 100644 --- a/collector/thermal_darwin_arm64.go +++ b/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 } diff --git a/collector/thermal_darwin_test.go b/collector/thermal_darwin_test.go new file mode 100644 index 0000000000..4d8624ea1b --- /dev/null +++ b/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{}{} + } +} diff --git a/end-to-end-test.sh b/end-to-end-test.sh index b439cea556..a4473a3809 100755 --- a/end-to-end-test.sh +++ b/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