Repository navigation
textfile: fix panic on a metric without HELP after inconsistent help texts - #3866
Open
vignesh2027 wants to merge 3 commits into
Open
vignesh2027 wants to merge 3 commits into
vignesh2027 wants to merge 3 commits into
Conversation
…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 <applemacbook6sep2004@gmail.com>
Member
|
Please do not |
Author
|
Sorry about that, I've removed the mention from the description. |
SuperQ
requested changes
Oct 28, 2026
Comment on lines
+222
to
+223
| // A family without a help text can still land here once | ||
| // the metric has been marked as inconsistent. |
Member
There was a problem hiding this comment.
Suggested change
| // A family without a help text can still land here once | |
| // the metric has been marked as inconsistent. |
Comment on lines
+197
to
+203
| 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", | ||
| } |
Member
There was a problem hiding this comment.
This should be added to the fixtures, not inlined with a WriteFile in the test.
Member
|
Any reason to not extend the lists of tests in |
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 <applemacbook6sep2004@gmail.com>
Author
|
No good reason, thanks. I moved the case into a new |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The textfile collector panics with a nil pointer dereference when the same metric appears in three or more
.promfiles, two of them with different HELP texts and a later one with no HELP line at all.After the first mismatch, the help text check treats every later family with that name as inconsistent because of the
|| helpTexts[1] != ""branch. That branch is also taken when the family has no HELP, and the code then reads*mf.Help, which is nil. The panic happens inside the per-collector goroutine started byNodeCollector.Collect, so nothing recovers it and node_exporter exits.This change logs the help text through
mf.GetHelp()and only overwrites the recorded help text when the new family has one. A family with no HELP is now dropped and reported throughnode_textfile_scrape_error, the same way other inconsistent families are.TestTextfileCollectorInconsistentHelpThenNoHelpwrites four files: HELP "First", HELP "Second", no HELP, and HELP "First" again. On main it fails with the recovered nil pointer panic. With this change only the first file's metric is exported and the scrape error is set. The fourth file checks that a family with no HELP does not clear the inconsistency marker.