From 1a1b86069a0a1895138d099d58d7d365c7ff385e Mon Sep 17 00:00:00 2001 From: Ying WANG Date: Sun, 15 Sep 2024 22:40:58 +0200 Subject: [PATCH 1/3] process_collector: Add Platform-Specific Describe for processCollector Signed-off-by: Ying WANG --- prometheus/process_collector.go | 31 +++++++------ prometheus/process_collector_darwin.go | 22 ++++++++- prometheus/process_collector_js.go | 11 ++++- prometheus/process_collector_other.go | 16 +++++++ prometheus/process_collector_test.go | 49 ++++++++++++++++++++ prometheus/process_collector_wasip1.go | 13 ++++-- prometheus/process_collector_windows.go | 21 ++++++--- prometheus/process_collector_windows_test.go | 49 ++++++++++++++++++++ 8 files changed, 186 insertions(+), 26 deletions(-) diff --git a/prometheus/process_collector.go b/prometheus/process_collector.go index 62a4e7ad9..40542f616 100644 --- a/prometheus/process_collector.go +++ b/prometheus/process_collector.go @@ -23,6 +23,7 @@ import ( type processCollector struct { collectFn func(chan<- Metric) + describeFn func(chan<- *Desc) pidFn func() (int, error) reportErrors bool cpuTotal *Desc @@ -122,26 +123,23 @@ func NewProcessCollector(opts ProcessCollectorOpts) Collector { // Set up process metric collection if supported by the runtime. if canCollectProcess() { c.collectFn = c.processCollect + c.describeFn = c.describe } else { - c.collectFn = func(ch chan<- Metric) { - c.reportError(ch, nil, errors.New("process metrics not supported on this platform")) - } + c.collectFn = c.defaultCollectFn + c.describeFn = c.defaultDescribeFn } return c } -// Describe returns all descriptions of the collector. -func (c *processCollector) Describe(ch chan<- *Desc) { - ch <- c.cpuTotal - ch <- c.openFDs - ch <- c.maxFDs - ch <- c.vsize - ch <- c.maxVsize - ch <- c.rss - ch <- c.startTime - ch <- c.inBytes - ch <- c.outBytes +func (c *processCollector) defaultCollectFn(ch chan<- Metric) { + c.reportError(ch, nil, errors.New("process metrics not supported on this platform")) +} + +func (c *processCollector) defaultDescribeFn(ch chan<- *Desc) { + if c.reportErrors { + ch <- NewInvalidDesc(errors.New("process metrics not supported on this platform")) + } } // Collect returns the current state of all metrics of the collector. @@ -149,6 +147,11 @@ func (c *processCollector) Collect(ch chan<- Metric) { c.collectFn(ch) } +// Describe returns all descriptions of the collector. +func (c *processCollector) Describe(ch chan<- *Desc) { + c.describeFn(ch) +} + func (c *processCollector) reportError(ch chan<- Metric, desc *Desc, err error) { if !c.reportErrors { return diff --git a/prometheus/process_collector_darwin.go b/prometheus/process_collector_darwin.go index 4d9314c3d..b8b5a5526 100644 --- a/prometheus/process_collector_darwin.go +++ b/prometheus/process_collector_darwin.go @@ -15,10 +15,11 @@ package prometheus import ( "fmt" - "golang.org/x/sys/unix" "os" "syscall" "time" + + "golang.org/x/sys/unix" ) func canCollectProcess() bool { @@ -58,6 +59,25 @@ func getOpenFileCount() (float64, error) { } } +// describe returns all descriptions of the collector for Darwin. +// Ensure that this list of descriptors is kept in sync with the metrics collected +// in the processCollect method. Any changes to the metrics in processCollect +// (such as adding or removing metrics) should be reflected in this list of descriptors. +func (c *processCollector) describe(ch chan<- *Desc) { + ch <- c.cpuTotal + ch <- c.openFDs + ch <- c.maxFDs + ch <- c.maxVsize + ch <- c.startTime + + /* the process could be collected but not implemented yet + ch <- c.rss + ch <- c.vsize + ch <- c.inBytes + ch <- c.outBytes + */ +} + func (c *processCollector) processCollect(ch chan<- Metric) { if procs, err := unix.SysctlKinfoProcSlice("kern.proc.pid", os.Getpid()); err == nil { if len(procs) == 1 { diff --git a/prometheus/process_collector_js.go b/prometheus/process_collector_js.go index b1e363d6c..805624021 100644 --- a/prometheus/process_collector_js.go +++ b/prometheus/process_collector_js.go @@ -20,7 +20,14 @@ func canCollectProcess() bool { return false } +// describe returns all descriptions of the collector for js. +// Ensure that this list of descriptors is kept in sync with the metrics collected +// in the processCollect method. Any changes to the metrics in processCollect +// (such as adding or removing metrics) should be reflected in this list of descriptors. func (c *processCollector) processCollect(ch chan<- Metric) { - // noop on this platform - return + c.defaultCollect(ch) +} + +func (c *processCollector) describe(ch chan<- *Desc) { + c.defaultDescribe(ch) } diff --git a/prometheus/process_collector_other.go b/prometheus/process_collector_other.go index a05029c1d..9f4b130be 100644 --- a/prometheus/process_collector_other.go +++ b/prometheus/process_collector_other.go @@ -78,3 +78,19 @@ func (c *processCollector) processCollect(ch chan<- Metric) { c.reportError(ch, nil, err) } } + +// describe returns all descriptions of the collector for others than windows, js, wasip1 and darwin. +// Ensure that this list of descriptors is kept in sync with the metrics collected +// in the processCollect method. Any changes to the metrics in processCollect +// (such as adding or removing metrics) should be reflected in this list of descriptors. +func (c *processCollector) describe(ch chan<- *Desc) { + ch <- c.cpuTotal + ch <- c.openFDs + ch <- c.maxFDs + ch <- c.vsize + ch <- c.maxVsize + ch <- c.rss + ch <- c.startTime + ch <- c.inBytes + ch <- c.outBytes +} diff --git a/prometheus/process_collector_test.go b/prometheus/process_collector_test.go index 0d62d41e0..9bead32a9 100644 --- a/prometheus/process_collector_test.go +++ b/prometheus/process_collector_test.go @@ -170,3 +170,52 @@ func TestNewPidFileFn(t *testing.T) { } } } + +func TestDescribeAndCollectAlignment(t *testing.T) { + collector := &processCollector{ + pidFn: getPIDFn(), + cpuTotal: NewDesc("cpu_total", "Total CPU usage", nil, nil), + openFDs: NewDesc("open_fds", "Number of open file descriptors", nil, nil), + maxFDs: NewDesc("max_fds", "Maximum file descriptors", nil, nil), + vsize: NewDesc("vsize", "Virtual memory size", nil, nil), + maxVsize: NewDesc("max_vsize", "Maximum virtual memory size", nil, nil), + rss: NewDesc("rss", "Resident Set Size", nil, nil), + startTime: NewDesc("start_time", "Process start time", nil, nil), + inBytes: NewDesc("in_bytes", "Input bytes", nil, nil), + outBytes: NewDesc("out_bytes", "Output bytes", nil, nil), + } + + // Collect and get descriptors + descCh := make(chan *Desc, 15) + collector.describe(descCh) + close(descCh) + + definedDescs := make(map[string]bool) + for desc := range descCh { + definedDescs[desc.String()] = true + } + + // Collect and get metrics + metricsCh := make(chan Metric, 15) + collector.processCollect(metricsCh) + close(metricsCh) + + collectedMetrics := make(map[string]bool) + for metric := range metricsCh { + collectedMetrics[metric.Desc().String()] = true + } + + // Verify that all described metrics are collected + for desc := range definedDescs { + if !collectedMetrics[desc] { + t.Errorf("Metric %s described but not collected", desc) + } + } + + // Verify that no extra metrics are collected + for desc := range collectedMetrics { + if !definedDescs[desc] { + t.Errorf("Metric %s collected but not described", desc) + } + } +} diff --git a/prometheus/process_collector_wasip1.go b/prometheus/process_collector_wasip1.go index d8d9a6d7a..509b3b4bd 100644 --- a/prometheus/process_collector_wasip1.go +++ b/prometheus/process_collector_wasip1.go @@ -20,7 +20,14 @@ func canCollectProcess() bool { return false } -func (*processCollector) processCollect(chan<- Metric) { - // noop on this platform - return +func (c *processCollector) processCollect(ch chan<- Metric) { + c.defaultCollect(ch) +} + +// describe returns all descriptions of the collector for wasip1. +// Ensure that this list of descriptors is kept in sync with the metrics collected +// in the processCollect method. Any changes to the metrics in processCollect +// (such as adding or removing metrics) should be reflected in this list of descriptors. +func (c *processCollector) describe(ch chan<- *Desc) { + c.defaultDescribe(ch) } diff --git a/prometheus/process_collector_windows.go b/prometheus/process_collector_windows.go index f973398df..fa474289e 100644 --- a/prometheus/process_collector_windows.go +++ b/prometheus/process_collector_windows.go @@ -79,14 +79,10 @@ func getProcessHandleCount(handle windows.Handle) (uint32, error) { } func (c *processCollector) processCollect(ch chan<- Metric) { - h, err := windows.GetCurrentProcess() - if err != nil { - c.reportError(ch, nil, err) - return - } + h := windows.CurrentProcess() var startTime, exitTime, kernelTime, userTime windows.Filetime - err = windows.GetProcessTimes(h, &startTime, &exitTime, &kernelTime, &userTime) + err := windows.GetProcessTimes(h, &startTime, &exitTime, &kernelTime, &userTime) if err != nil { c.reportError(ch, nil, err) return @@ -111,6 +107,19 @@ func (c *processCollector) processCollect(ch chan<- Metric) { ch <- MustNewConstMetric(c.maxFDs, GaugeValue, float64(16*1024*1024)) // Windows has a hard-coded max limit, not per-process. } +// describe returns all descriptions of the collector for windows. +// Ensure that this list of descriptors is kept in sync with the metrics collected +// in the processCollect method. Any changes to the metrics in processCollect +// (such as adding or removing metrics) should be reflected in this list of descriptors. +func (c *processCollector) describe(ch chan<- *Desc) { + ch <- c.cpuTotal + ch <- c.openFDs + ch <- c.maxFDs + ch <- c.vsize + ch <- c.rss + ch <- c.startTime +} + func fileTimeToSeconds(ft windows.Filetime) float64 { return float64(uint64(ft.HighDateTime)<<32+uint64(ft.LowDateTime)) / 1e7 } diff --git a/prometheus/process_collector_windows_test.go b/prometheus/process_collector_windows_test.go index 6f687b9c3..670c0fc53 100644 --- a/prometheus/process_collector_windows_test.go +++ b/prometheus/process_collector_windows_test.go @@ -68,3 +68,52 @@ func TestWindowsProcessCollector(t *testing.T) { } } } + +func TestWindowsDescribeAndCollectAlignment(t *testing.T) { + collector := &processCollector{ + pidFn: getPIDFn(), + cpuTotal: NewDesc("cpu_total", "Total CPU usage", nil, nil), + openFDs: NewDesc("open_fds", "Number of open file descriptors", nil, nil), + maxFDs: NewDesc("max_fds", "Maximum file descriptors", nil, nil), + vsize: NewDesc("vsize", "Virtual memory size", nil, nil), + maxVsize: NewDesc("max_vsize", "Maximum virtual memory size", nil, nil), + rss: NewDesc("rss", "Resident Set Size", nil, nil), + startTime: NewDesc("start_time", "Process start time", nil, nil), + inBytes: NewDesc("in_bytes", "Input bytes", nil, nil), + outBytes: NewDesc("out_bytes", "Output bytes", nil, nil), + } + + // Collect and get descriptors + descCh := make(chan *Desc, 15) + collector.describe(descCh) + close(descCh) + + definedDescs := make(map[string]bool) + for desc := range descCh { + definedDescs[desc.String()] = true + } + + // Collect and get metrics + metricsCh := make(chan Metric, 15) + collector.processCollect(metricsCh) + close(metricsCh) + + collectedMetrics := make(map[string]bool) + for metric := range metricsCh { + collectedMetrics[metric.Desc().String()] = true + } + + // Verify that all described metrics are collected + for desc := range definedDescs { + if !collectedMetrics[desc] { + t.Errorf("Metric %s described but not collected", desc) + } + } + + // Verify that no extra metrics are collected + for desc := range collectedMetrics { + if !definedDescs[desc] { + t.Errorf("Metric %s collected but not described", desc) + } + } +} From b8300bc65b903ab443cb9a0cd465968cde5eb30c Mon Sep 17 00:00:00 2001 From: Ying WANG Date: Sun, 15 Sep 2024 23:41:24 +0200 Subject: [PATCH 2/3] add changelog entry Signed-off-by: Ying WANG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f83a02761..79318dd02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased +* [ENHANCEMENT] process_collector: Add Platform-Specific Describe for processCollector. #1625 + ## 1.20.2 / 2024-08-23 * [BUGFIX] promhttp: Unset Content-Encoding header when data is uncompressed. #1596 From 0bfbc373ba7089a5714d03525468e173a7f42520 Mon Sep 17 00:00:00 2001 From: Ying WANG Date: Sun, 29 Sep 2024 22:26:28 +0200 Subject: [PATCH 3/3] Address comments Signed-off-by: Ying WANG --- CHANGELOG.md | 2 -- prometheus/process_collector.go | 8 ++++---- prometheus/process_collector_js.go | 4 ++-- prometheus/process_collector_wasip1.go | 4 ++-- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79318dd02..f83a02761 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,5 @@ ## Unreleased -* [ENHANCEMENT] process_collector: Add Platform-Specific Describe for processCollector. #1625 - ## 1.20.2 / 2024-08-23 * [BUGFIX] promhttp: Unset Content-Encoding header when data is uncompressed. #1596 diff --git a/prometheus/process_collector.go b/prometheus/process_collector.go index 40542f616..e7bce8b58 100644 --- a/prometheus/process_collector.go +++ b/prometheus/process_collector.go @@ -125,18 +125,18 @@ func NewProcessCollector(opts ProcessCollectorOpts) Collector { c.collectFn = c.processCollect c.describeFn = c.describe } else { - c.collectFn = c.defaultCollectFn - c.describeFn = c.defaultDescribeFn + c.collectFn = c.errorCollectFn + c.describeFn = c.errorDescribeFn } return c } -func (c *processCollector) defaultCollectFn(ch chan<- Metric) { +func (c *processCollector) errorCollectFn(ch chan<- Metric) { c.reportError(ch, nil, errors.New("process metrics not supported on this platform")) } -func (c *processCollector) defaultDescribeFn(ch chan<- *Desc) { +func (c *processCollector) errorDescribeFn(ch chan<- *Desc) { if c.reportErrors { ch <- NewInvalidDesc(errors.New("process metrics not supported on this platform")) } diff --git a/prometheus/process_collector_js.go b/prometheus/process_collector_js.go index 805624021..0b42a6f1c 100644 --- a/prometheus/process_collector_js.go +++ b/prometheus/process_collector_js.go @@ -25,9 +25,9 @@ func canCollectProcess() bool { // in the processCollect method. Any changes to the metrics in processCollect // (such as adding or removing metrics) should be reflected in this list of descriptors. func (c *processCollector) processCollect(ch chan<- Metric) { - c.defaultCollect(ch) + c.errorCollectFn(ch) } func (c *processCollector) describe(ch chan<- *Desc) { - c.defaultDescribe(ch) + c.errorDescribeFn(ch) } diff --git a/prometheus/process_collector_wasip1.go b/prometheus/process_collector_wasip1.go index 509b3b4bd..5c74c2c48 100644 --- a/prometheus/process_collector_wasip1.go +++ b/prometheus/process_collector_wasip1.go @@ -21,7 +21,7 @@ func canCollectProcess() bool { } func (c *processCollector) processCollect(ch chan<- Metric) { - c.defaultCollect(ch) + c.errorCollectFn(ch) } // describe returns all descriptions of the collector for wasip1. @@ -29,5 +29,5 @@ func (c *processCollector) processCollect(ch chan<- Metric) { // in the processCollect method. Any changes to the metrics in processCollect // (such as adding or removing metrics) should be reflected in this list of descriptors. func (c *processCollector) describe(ch chan<- *Desc) { - c.defaultDescribe(ch) + c.errorDescribeFn(ch) }