From d4b4358900fb900c07e254efdf773d3400c3278c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9C=8F=E6=89=BF?= Date: Sun, 13 Sep 2026 21:26:01 +0800 Subject: [PATCH] filesystem: Deduplicate on the labels each metric carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mount table deduplication keys on the whole filesystemLabels struct, but no metric carries all of those fields. Two entries that differ only in a field no metric exposes are treated as distinct and then emit byte-identical series, which client_golang rejects. That fails the whole scrape rather than just dropping the repeated filesystem. filesystemLabels is wider than either emitted label set: every metric except node_filesystem_mount_info carries device, mountpoint, fstype and device_error, while mount_info carries device, major, minor and mountpoint. Key each set on the labels it actually emits so an entry is skipped per metric set instead of as a whole. Blanking the mount options in #3376 removed two of the extra fields, but major and minor are still part of the key and still absent from every metric but mount_info, so the bug remains reachable. A multihomed NFS export reaches it: one mountinfo line per server address, same device, mount point and fstype, and a superblock of its own per address. That matches the reports, which all fail with exactly seven duplicated metrics and never mention mount_info. Had the entries differed only in their options the device numbers would match too and mount_info would collide as well, giving eight. Emission moves into collectStats because Update calls the platform-specific GetStats and offered no way to feed in a mount table. The filesystem collector is disabled in the end-to-end tests, so no golden output changes. Fixes #2514 Signed-off-by: 霏承 --- collector/filesystem_common.go | 122 ++++++++++++++------ collector/filesystem_common_test.go | 173 ++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+), 38 deletions(-) create mode 100644 collector/filesystem_common_test.go diff --git a/collector/filesystem_common.go b/collector/filesystem_common.go index 1cd4f3dc71..c6b6a9fa84 100644 --- a/collector/filesystem_common.go +++ b/collector/filesystem_common.go @@ -83,6 +83,24 @@ type filesystemLabels struct { device, mountPoint, fsType, mountOptions, superOptions, deviceError, major, minor string } +// filesystemMetricKey holds the labels carried by every filesystem metric +// except node_filesystem_mount_info. filesystemLabels is wider than any label +// set actually emitted: it also carries the device numbers, which only +// node_filesystem_mount_info exposes, and the mount options, which no metric +// exposes. Deduplicating on the wider struct lets two mount table entries for +// the same filesystem through, and the identical series they emit are rejected +// by client_golang, which fails the whole scrape. +type filesystemMetricKey struct { + device, mountPoint, fsType, deviceError string +} + +// mountInfoKey holds the labels carried by node_filesystem_mount_info. They are +// not a subset of filesystemMetricKey: mount_info reports the device numbers +// instead of the filesystem type, so it needs a key of its own. +type mountInfoKey struct { + device, major, minor, mountPoint string +} + type filesystemStats struct { labels filesystemLabels size, free, avail float64 @@ -185,59 +203,87 @@ func (c *filesystemCollector) Update(ch chan<- prometheus.Metric) error { if err != nil { return err } - // Make sure we expose a metric once, even if there are multiple mounts - seen := map[filesystemLabels]bool{} + c.collectStats(ch, stats) + return nil +} + +// collectStats emits the filesystem metrics for stats. The same filesystem can +// appear more than once in the mount table, so each metric is emitted once per +// distinct set of labels it actually carries. +func (c *filesystemCollector) collectStats(ch chan<- prometheus.Metric, stats []filesystemStats) { + seenMetric := map[filesystemMetricKey]bool{} + seenMountInfo := map[mountInfoKey]bool{} for _, s := range stats { - if seen[s.labels] { + metricKey := filesystemMetricKey{ + device: s.labels.device, + mountPoint: s.labels.mountPoint, + fsType: s.labels.fsType, + deviceError: s.labels.deviceError, + } + infoKey := mountInfoKey{ + device: s.labels.device, + major: s.labels.major, + minor: s.labels.minor, + mountPoint: s.labels.mountPoint, + } + firstMetric := !seenMetric[metricKey] + firstMountInfo := !seenMountInfo[infoKey] + if !firstMetric && !firstMountInfo { continue } - seen[s.labels] = true + seenMetric[metricKey] = true + seenMountInfo[infoKey] = true - ch <- prometheus.MustNewConstMetric( - c.deviceErrorDesc, prometheus.GaugeValue, - s.deviceError, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) - ch <- prometheus.MustNewConstMetric( - c.roDesc, prometheus.GaugeValue, - s.ro, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) + if firstMetric { + ch <- prometheus.MustNewConstMetric( + c.deviceErrorDesc, prometheus.GaugeValue, + s.deviceError, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + ch <- prometheus.MustNewConstMetric( + c.roDesc, prometheus.GaugeValue, + s.ro, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + } if s.deviceError > 0 { continue } - ch <- prometheus.MustNewConstMetric( - c.sizeDesc, prometheus.GaugeValue, - s.size, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) - ch <- prometheus.MustNewConstMetric( - c.freeDesc, prometheus.GaugeValue, - s.free, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) - ch <- prometheus.MustNewConstMetric( - c.availDesc, prometheus.GaugeValue, - s.avail, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) - ch <- prometheus.MustNewConstMetric( - c.filesDesc, prometheus.GaugeValue, - s.files, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) - ch <- prometheus.MustNewConstMetric( - c.filesFreeDesc, prometheus.GaugeValue, - s.filesFree, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, - ) - ch <- prometheus.MustNewConstMetric( - c.mountInfoDesc, prometheus.GaugeValue, - 1.0, s.labels.device, s.labels.major, s.labels.minor, s.labels.mountPoint, - ) - if s.purgeable >= 0 { + if firstMetric { + ch <- prometheus.MustNewConstMetric( + c.sizeDesc, prometheus.GaugeValue, + s.size, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + ch <- prometheus.MustNewConstMetric( + c.freeDesc, prometheus.GaugeValue, + s.free, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + ch <- prometheus.MustNewConstMetric( + c.availDesc, prometheus.GaugeValue, + s.avail, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + ch <- prometheus.MustNewConstMetric( + c.filesDesc, prometheus.GaugeValue, + s.files, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + ch <- prometheus.MustNewConstMetric( + c.filesFreeDesc, prometheus.GaugeValue, + s.filesFree, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, + ) + } + if firstMountInfo { + ch <- prometheus.MustNewConstMetric( + c.mountInfoDesc, prometheus.GaugeValue, + 1.0, s.labels.device, s.labels.major, s.labels.minor, s.labels.mountPoint, + ) + } + if firstMetric && s.purgeable >= 0 { ch <- prometheus.MustNewConstMetric( c.purgeableDesc, prometheus.GaugeValue, s.purgeable, s.labels.device, s.labels.mountPoint, s.labels.fsType, s.labels.deviceError, ) } } - return nil } func newMountPointsFilter(logger *slog.Logger) (deviceFilter, error) { diff --git a/collector/filesystem_common_test.go b/collector/filesystem_common_test.go new file mode 100644 index 0000000000..b63742360a --- /dev/null +++ b/collector/filesystem_common_test.go @@ -0,0 +1,173 @@ +// 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 !nofilesystem && (linux || freebsd || netbsd || openbsd || darwin || dragonfly || aix) + +package collector + +import ( + "io" + "log/slog" + "testing" + + "github.com/prometheus/client_golang/prometheus" +) + +// testFilesystemCollector exposes collectStats through the Collector interface +// so metric emission can be checked without reading a real mount table. +type testFilesystemCollector struct { + collector *filesystemCollector + stats []filesystemStats +} + +func (c testFilesystemCollector) Collect(ch chan<- prometheus.Metric) { + c.collector.collectStats(ch, c.stats) +} + +func (c testFilesystemCollector) Describe(ch chan<- *prometheus.Desc) { + prometheus.DescribeByCollect(c, ch) +} + +func newFilesystemCollectorForStats(t *testing.T) *filesystemCollector { + t.Helper() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + coll, err := NewFilesystemCollector(logger) + if err != nil { + t.Fatal(err) + } + return coll.(*filesystemCollector) +} + +// gatherFilesystemStats gathers stats through a pedantic registry, which +// rejects a label set emitted twice exactly the way a scrape does. It returns +// the number of series gathered per metric name. +func gatherFilesystemStats(t *testing.T, c *filesystemCollector, stats []filesystemStats) map[string]int { + t.Helper() + registry := prometheus.NewPedanticRegistry() + if err := registry.Register(testFilesystemCollector{collector: c, stats: stats}); err != nil { + t.Fatal(err) + } + families, err := registry.Gather() + if err != nil { + t.Fatalf("gathering filesystem metrics: %s", err) + } + seriesPerMetric := make(map[string]int, len(families)) + for _, family := range families { + seriesPerMetric[family.GetName()] = len(family.GetMetric()) + } + return seriesPerMetric +} + +func nfsMountStat(fsType, minor string) filesystemStats { + return filesystemStats{ + labels: filesystemLabels{ + device: "storagesystem:/exports/home", + mountPoint: "/home", + fsType: fsType, + major: "0", + minor: minor, + }, + size: 536870912000, + free: 525147832320, + avail: 525147832320, + files: 33554432, + filesFree: 33216512, + purgeable: -1, + } +} + +// everyFilesystemMetric lists the metrics emitted for a mount that reports no +// device error. purgeable is left out because it is only emitted on platforms +// that report it. +var everyFilesystemMetric = []string{ + "node_filesystem_avail_bytes", + "node_filesystem_device_error", + "node_filesystem_files", + "node_filesystem_files_free", + "node_filesystem_free_bytes", + "node_filesystem_mount_info", + "node_filesystem_readonly", + "node_filesystem_size_bytes", +} + +func checkFilesystemSeries(t *testing.T, stats []filesystemStats, expected map[string]int) { + t.Helper() + c := newFilesystemCollectorForStats(t) + series := gatherFilesystemStats(t, c, stats) + for _, name := range everyFilesystemMetric { + want, tracked := expected[name] + if !tracked { + t.Errorf("%s: not accounted for in the test expectations", name) + continue + } + if series[name] != want { + t.Errorf("%s: expected %d series, got %d", name, want, series[name]) + } + } + for name := range series { + if _, tracked := expected[name]; !tracked { + t.Errorf("%s: unexpected metric emitted with %d series", name, series[name]) + } + } +} + +// A multihomed NFS export is listed once per server address, so the same device +// at the same mount point shows up several times with the same fstype but a +// device number of its own per address. Only node_filesystem_mount_info carries +// major and minor; every other filesystem metric describes the same series each +// time, which used to fail the whole scrape. +func TestFilesystemDeduplicatesRepeatMountWithDifferentDeviceNumbers(t *testing.T) { + expected := map[string]int{"node_filesystem_mount_info": 2} + for _, name := range everyFilesystemMetric { + if _, ok := expected[name]; !ok { + expected[name] = 1 + } + } + + checkFilesystemSeries(t, []filesystemStats{ + nfsMountStat("nfs", "41"), + nfsMountStat("nfs", "42"), + }, expected) +} + +// The same device numbers reported under two filesystem types are two series +// for the per-fstype metrics but a single series for mount_info, which does not +// carry fstype. This is the mirror image of the case above and needs its own +// deduplication key. +func TestFilesystemDeduplicatesMountInfoAcrossFilesystemTypes(t *testing.T) { + expected := map[string]int{"node_filesystem_mount_info": 1} + for _, name := range everyFilesystemMetric { + if _, ok := expected[name]; !ok { + expected[name] = 2 + } + } + + checkFilesystemSeries(t, []filesystemStats{ + nfsMountStat("nfs", "41"), + nfsMountStat("nfs4", "41"), + }, expected) +} + +// Mount table entries that repeat a filesystem without adding anything a metric +// carries are reported once, which is what the existing deduplication intended. +func TestFilesystemDeduplicatesIdenticalRepeatMount(t *testing.T) { + expected := map[string]int{} + for _, name := range everyFilesystemMetric { + expected[name] = 1 + } + + checkFilesystemSeries(t, []filesystemStats{ + nfsMountStat("nfs", "41"), + nfsMountStat("nfs", "41"), + }, expected) +}