From b1366d037e512ae7fce75ee0da93ce8127e7112f Mon Sep 17 00:00:00 2001 From: vigneshwar Date: Thu, 22 Oct 2026 21:40:54 +0530 Subject: [PATCH 1/2] textfile: fix panic on a metric without HELP after inconsistent help texts Once a metric has been seen with two different help texts, every later family with the same name is treated as inconsistent. If such a later family has no HELP line, the collector dereferenced its nil help text and panicked. The panic happens in the per-collector goroutine, so it takes down node_exporter. Use the getter for the log line and keep the recorded help text when the new family has none. Signed-off-by: vigneshwar --- collector/textfile.go | 8 ++++-- collector/textfile_test.go | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/collector/textfile.go b/collector/textfile.go index 6629eb9d8e..50aae4ccee 100644 --- a/collector/textfile.go +++ b/collector/textfile.go @@ -219,12 +219,16 @@ func (c *textFileCollector) Update(ch chan<- prometheus.Metric) error { // Check for metrics with inconsistent help texts and take the first help text occurrence. if helpTexts, seen := metricsNamesToHelpTexts[*mf.Name]; seen { if mf.Help != nil && helpTexts[0] != *mf.Help || helpTexts[1] != "" { - metricsNamesToHelpTexts[*mf.Name] = [2]string{helpTexts[0], *mf.Help} + // A family without a help text can still land here once + // the metric has been marked as inconsistent. + if mf.Help != nil { + metricsNamesToHelpTexts[*mf.Name] = [2]string{helpTexts[0], *mf.Help} + } errored = true c.logger.Error("inconsistent metric help text", "metric", *mf.Name, "original_help_text", helpTexts[0], - "new_help_text", *mf.Help, + "new_help_text", mf.GetHelp(), // Only the first file path will be recorded in case of two or more inconsistent help texts. "file", metricsNamesToFiles[*mf.Name][0]) continue diff --git a/collector/textfile_test.go b/collector/textfile_test.go index 222ffa8fee..5f970a5aea 100644 --- a/collector/textfile_test.go +++ b/collector/textfile_test.go @@ -191,3 +191,53 @@ func TestTextfileCollectorSupportsUTF8Names(t *testing.T) { } t.Fatalf("expected dotted metric name, got %v", families) } + +func TestTextfileCollectorInconsistentHelpThenNoHelp(t *testing.T) { + dir := t.TempDir() + files := map[string]string{ + // Files are read in lexical order. + "a.prom": "# HELP foo First help text.\n# TYPE foo gauge\nfoo{file=\"a\"} 1\n", + "b.prom": "# HELP foo Second help text.\n# TYPE foo gauge\nfoo{file=\"b\"} 2\n", + "c.prom": "# TYPE foo gauge\nfoo{file=\"c\"} 3\n", + "d.prom": "# HELP foo First help text.\n# TYPE foo gauge\nfoo{file=\"d\"} 4\n", + } + for name, content := range files { + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + c := &textFileCollector{ + paths: []string{dir}, + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + registry := prometheus.NewRegistry() + registry.MustRegister(collectorAdapter{c}) + + families, err := registry.Gather() + if err != nil { + t.Fatalf("failed to gather textfile metrics: %v", err) + } + + var scrapeError float64 = -1 + var fooFiles []string + for _, family := range families { + switch family.GetName() { + case "node_textfile_scrape_error": + scrapeError = family.Metric[0].GetGauge().GetValue() + case "foo": + if got := family.GetHelp(); got != "First help text." { + t.Errorf("expected help text of the first file, got %q", got) + } + for _, m := range family.Metric { + fooFiles = append(fooFiles, m.Label[0].GetValue()) + } + } + } + if scrapeError != 1 { + t.Errorf("expected node_textfile_scrape_error to be 1, got %v", scrapeError) + } + if len(fooFiles) != 1 || fooFiles[0] != "a" { + t.Errorf("expected only the metric from a.prom, got %v", fooFiles) + } +} From a5218edf4c307ce7e08a7ea5fbb9c303606dfe83 Mon Sep 17 00:00:00 2001 From: vigneshwar Date: Thu, 29 Oct 2026 15:05:23 +0530 Subject: [PATCH 2/2] textfile: move the missing help test into the fixtures Cover the inconsistent help case with a fixture in TestTextfileCollector instead of a separate test that writes files, and drop the extra comment. Signed-off-by: vigneshwar --- ...etrics_merge_inconsistent_then_no_help.out | 12 +++++ .../a.prom | 3 ++ .../b.prom | 3 ++ .../c.prom | 2 + .../d.prom | 3 ++ collector/textfile.go | 2 - collector/textfile_test.go | 54 ++----------------- 7 files changed, 27 insertions(+), 52 deletions(-) create mode 100644 collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help.out create mode 100644 collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/a.prom create mode 100644 collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/b.prom create mode 100644 collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/c.prom create mode 100644 collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/d.prom diff --git a/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help.out b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help.out new file mode 100644 index 0000000000..38c23a044a --- /dev/null +++ b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help.out @@ -0,0 +1,12 @@ +# HELP foo First help text. +# TYPE foo gauge +foo{file="a"} 1 +# HELP node_textfile_mtime_seconds Unixtime mtime of textfiles successfully read. +# TYPE node_textfile_mtime_seconds gauge +node_textfile_mtime_seconds{file="fixtures/textfile/metrics_merge_inconsistent_then_no_help/a.prom"} 1 +node_textfile_mtime_seconds{file="fixtures/textfile/metrics_merge_inconsistent_then_no_help/b.prom"} 1 +node_textfile_mtime_seconds{file="fixtures/textfile/metrics_merge_inconsistent_then_no_help/c.prom"} 1 +node_textfile_mtime_seconds{file="fixtures/textfile/metrics_merge_inconsistent_then_no_help/d.prom"} 1 +# HELP node_textfile_scrape_error 1 if there was an error opening or reading a file, 0 otherwise +# TYPE node_textfile_scrape_error gauge +node_textfile_scrape_error 1 diff --git a/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/a.prom b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/a.prom new file mode 100644 index 0000000000..4e727fd6d5 --- /dev/null +++ b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/a.prom @@ -0,0 +1,3 @@ +# HELP foo First help text. +# TYPE foo gauge +foo{file="a"} 1 diff --git a/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/b.prom b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/b.prom new file mode 100644 index 0000000000..b094d1087e --- /dev/null +++ b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/b.prom @@ -0,0 +1,3 @@ +# HELP foo Second help text. +# TYPE foo gauge +foo{file="b"} 2 diff --git a/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/c.prom b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/c.prom new file mode 100644 index 0000000000..7e6cf1efa5 --- /dev/null +++ b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/c.prom @@ -0,0 +1,2 @@ +# TYPE foo gauge +foo{file="c"} 3 diff --git a/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/d.prom b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/d.prom new file mode 100644 index 0000000000..3569ccf392 --- /dev/null +++ b/collector/fixtures/textfile/metrics_merge_inconsistent_then_no_help/d.prom @@ -0,0 +1,3 @@ +# HELP foo First help text. +# TYPE foo gauge +foo{file="d"} 4 diff --git a/collector/textfile.go b/collector/textfile.go index 50aae4ccee..8e4055f297 100644 --- a/collector/textfile.go +++ b/collector/textfile.go @@ -219,8 +219,6 @@ func (c *textFileCollector) Update(ch chan<- prometheus.Metric) error { // Check for metrics with inconsistent help texts and take the first help text occurrence. if helpTexts, seen := metricsNamesToHelpTexts[*mf.Name]; seen { if mf.Help != nil && helpTexts[0] != *mf.Help || helpTexts[1] != "" { - // A family without a help text can still land here once - // the metric has been marked as inconsistent. if mf.Help != nil { metricsNamesToHelpTexts[*mf.Name] = [2]string{helpTexts[0], *mf.Help} } diff --git a/collector/textfile_test.go b/collector/textfile_test.go index 5f970a5aea..1412377c66 100644 --- a/collector/textfile_test.go +++ b/collector/textfile_test.go @@ -122,6 +122,10 @@ func TestTextfileCollector(t *testing.T) { paths: []string{"fixtures/textfile/metrics_merge_different_help"}, out: "fixtures/textfile/metrics_merge_different_help.out", }, + { + paths: []string{"fixtures/textfile/metrics_merge_inconsistent_then_no_help"}, + out: "fixtures/textfile/metrics_merge_inconsistent_then_no_help.out", + }, } for i, test := range tests { @@ -191,53 +195,3 @@ func TestTextfileCollectorSupportsUTF8Names(t *testing.T) { } t.Fatalf("expected dotted metric name, got %v", families) } - -func TestTextfileCollectorInconsistentHelpThenNoHelp(t *testing.T) { - dir := t.TempDir() - files := map[string]string{ - // Files are read in lexical order. - "a.prom": "# HELP foo First help text.\n# TYPE foo gauge\nfoo{file=\"a\"} 1\n", - "b.prom": "# HELP foo Second help text.\n# TYPE foo gauge\nfoo{file=\"b\"} 2\n", - "c.prom": "# TYPE foo gauge\nfoo{file=\"c\"} 3\n", - "d.prom": "# HELP foo First help text.\n# TYPE foo gauge\nfoo{file=\"d\"} 4\n", - } - for name, content := range files { - if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o600); err != nil { - t.Fatal(err) - } - } - - c := &textFileCollector{ - paths: []string{dir}, - logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - registry := prometheus.NewRegistry() - registry.MustRegister(collectorAdapter{c}) - - families, err := registry.Gather() - if err != nil { - t.Fatalf("failed to gather textfile metrics: %v", err) - } - - var scrapeError float64 = -1 - var fooFiles []string - for _, family := range families { - switch family.GetName() { - case "node_textfile_scrape_error": - scrapeError = family.Metric[0].GetGauge().GetValue() - case "foo": - if got := family.GetHelp(); got != "First help text." { - t.Errorf("expected help text of the first file, got %q", got) - } - for _, m := range family.Metric { - fooFiles = append(fooFiles, m.Label[0].GetValue()) - } - } - } - if scrapeError != 1 { - t.Errorf("expected node_textfile_scrape_error to be 1, got %v", scrapeError) - } - if len(fooFiles) != 1 || fooFiles[0] != "a" { - t.Errorf("expected only the metric from a.prom, got %v", fooFiles) - } -}