collector/slabinfo: index duplicate slab entries - #3784
Conversation
|
Everything else looks good, including the |
|
/workflow-approve |
39d28bc to
0b94860
Compare
|
Fixed, thanks. The test file now uses the year-less header; |
|
/workflow-approve |
1 similar comment
|
/workflow-approve |
nicolastakashi
left a comment
nicolastakashi
left a comment
There was a problem hiding this comment.
Thanks for this, and for digging up the kmem_cache_sanity_check() detail.
One thing I'd change before it lands: only attach index where a slab name actually repeats. Right now every series on every host picks up index="0", so people with no duplicates still have to go fix their dashboards for a bug they never had. We hit this in hwmon (#3646) and disambiguated only on collision, with TestHwmonUniqueChipNamesAreUnchanged to keep it that way.
Details inline. writeSlabinfo is fine as it is, and pre-sizing seen is good.
| fs: fs, | ||
| subsystem: "slabinfo", | ||
| labels: []string{"slab"}, | ||
| labels: []string{"slab", "index"}, |
There was a problem hiding this comment.
Keep the label called index, it matches rapl and sysctl and there's no *_index anywhere in the tree.
What I'd change is when it gets attached. Emitting it unconditionally means every existing node_slabinfo_* series changes identity, including on the many hosts that have no duplicate names at all. Two descriptor variants per column, one {slab} and one {slab,index}, picked per entry, keeps those hosts exactly as they are.
| ch <- c.objectSizeBytes(slab.Name, slab.ObjSize) | ||
| ch <- c.objectsPerSlab(slab.Name, slab.ObjPerSlab) | ||
| ch <- c.pagesPerSlab(slab.Name, slab.PagesPerSlab) | ||
| index := strconv.Itoa(seen[slab.Name]) |
There was a problem hiding this comment.
For the collision-only version this becomes two passes: count occurrences of each name first, then emit, attaching index only where the count is above one.
Worth knowing while you restructure: the filter matches on name, so every entry sharing a name is kept or dropped together. Counting before or after filtering gives the same answer.
| t.Fatal(err) | ||
| } | ||
| reg := prometheus.NewPedanticRegistry() | ||
| if err := reg.Register(prometheus.CollectorFunc(func(ch chan<- prometheus.Metric) { |
There was a problem hiding this comment.
Fair warning on something that took me a while to pin down, since it will bite you as soon as the label becomes conditional.
Mixed label sets within one metric family do work. A registry accepts them and GatherAndCompare matches them. But Registry.Register rejects two descriptors sharing an fqName when their label names differ:
descriptors reported by collector have inconsistent label names or help strings
for the same fully-qualified name, offender is Desc{fqName: "node_slabinfo_objects", ...}
Production never hits it, because NodeCollector.Describe only emits the two scrape descriptors. This does, because CollectorFunc describes itself through DescribeByCollect. A small test collector with an empty Describe fixes it. Worth a comment saying why, or someone will helpfully put it back.
| return got | ||
| } | ||
|
|
||
| func checkSlabinfo(t *testing.T, got map[string]float64, want map[string]float64) { |
There was a problem hiding this comment.
This only checks that the expected keys exist, so extra or wrong series slide through. testutil.GatherAndCompare against a full exposition string would catch them, and pins HELP and TYPE at the same time.
Two cases I'd add: unique names carrying no index label at all, which is the guarantee the whole change rests on, and a no-data case pointing *procPath at an empty t.TempDir().
|
|
||
| // Excluded slabs must not consume an index, so the ordinals of the remaining | ||
| // entries stay contiguous. | ||
| func TestSlabinfoIndexIgnoresFilteredSlabs(t *testing.T) { |
There was a problem hiding this comment.
I'd rework or drop this one. The filter matches on name, so excluding drop_me could never have shifted dup_cache's numbering. It passes, but not for the reason the name claims.
| node_slabinfo_active_objects{slab="kmalloc-8192"} 132 | ||
| node_slabinfo_active_objects{slab="kmem_cache"} 320 | ||
| node_slabinfo_active_objects{slab="tw_sock_TCP"} 704 | ||
| node_slabinfo_active_objects{index="0",slab="dmaengine-unmap-128"} 1206 |
There was a problem hiding this comment.
Both golden files should come out unchanged under the collision-only approach, since this fixture has no duplicate slab names. Please drop them from the diff.
If you're up for it, adding one duplicated row to collector/fixtures/proc/slabinfo and re-running ./end-to-end-test.sh -u would put both shapes in the golden side by side.
/proc/slabinfo can list the same slab name more than once, for example one cache per device instance. Labelling only by slab name therefore produces duplicate label sets, causing the registry to reject the scrape. Count the entries sharing each name first, then add an index label carrying each entry's position among them. Only names that actually repeat are disambiguated, so a host whose slab names are all unique keeps exactly the series it has today and does not have to change dashboards for a bug it never hit. Where a name does repeat, every kernel cache keeps its own series with its own counts and geometry. The label name matches the ordinal index convention already used by the rapl and sysctl collectors. The ordinal follows /proc/slabinfo ordering and is not a stable identity: if a cache is created or destroyed, subsequent entries shift. The e2e fixture gains a duplicated mlx5_fs_ftes entry so both label shapes appear side by side in the golden output. Fixes prometheus#3506 Signed-off-by: Kyle Mason <kyle@kylemason.org>
0b94860 to
3f6d151
Compare
|
Thanks for the detailed review. I’ve pushed 3f6d151 addressing the points you raised. The collector now uses collision-only disambiguation: unique slab names retain the existing {slab} label set with no index, while only repeated names receive index="0", index="1", etc. The implementation uses the two-pass count/emission approach you suggested, with counting before filtering since the filter operates on slab name. I also reworked the tests to use full testutil.GatherAndCompare exposition comparisons. The test collector intentionally implements an empty Describe() and now has a comment explaining why that is necessary for the mixed {slab} / {slab,index} descriptor shapes. The tests now explicitly cover unique names remaining unchanged, duplicate names, mixed unique/duplicate names, differing per-entry geometry, filtering, and the missing-slabinfo case. I also rewrote the previous filter test so it no longer makes the incorrect claim about unrelated names consuming indices. I took your suggestion on the e2e fixture as well and added the duplicated mlx5_fs_ftes case from #3506. The existing unique slab metrics remain unchanged; both golden diffs are additive only. One note on e2e-64k-page-output.txt: I can’t generate that variant directly on this x86_64 host because the e2e script selects it on aarch64/ppc64le. I derived the slabinfo additions from the regenerated normal golden and verified that the two goldens still differ by exactly the same pre-existing architecture-specific CPU-info block and nothing else. CI on the relevant architecture will be the authoritative check. Local validation on the pushed commit is green: gofmt, go vet ./..., go build ./..., go test -short ./..., collector tests, the repository-pinned golangci-lint, license checks, e2e, and amd64/386/arm64 builds. Thanks again for the review. |
2 participants
Fixes #3506.
/proc/slabinfomay contain repeated slab names, for example one cache perdevice instance. The collector currently labels series only by slab name, so
duplicates produce identical label sets and cause
Gather()to fail for theentire scrape.
This change disambiguates only the names that actually collide.
Behaviour
A slab name that occurs once keeps exactly the identity it has today, with no
indexlabel:A slab name that occurs more than once gains an ordinal, one series per cache:
Hosts with no duplicate slab names therefore see no change at all, and nobody
has to update dashboards or recording rules for a problem they never had. This
follows the collision-only approach already used for chip names in the hwmon
collector (#3646).
Implementation
Updatenow runs two passes. The first counts how many entries share each slabname. The second applies the include/exclude filter and emits, attaching the
ordinal only where the count is greater than one. The filter matches on name, so
every entry sharing a name is kept or dropped together and counting ahead of the
filter gives the same answer.
Each column has two descriptor variants,
{slab}and{slab,index}, selectedper entry. Mixed label sets inside one metric family are intentional and are
what allows unique names to keep their existing identity while duplicates are
disambiguated in the same scrape.
The label is spelled
index, matching the ordinal convention already used byrapl_linux.goandsysctl_linux.go. There is no<subsystem>_indexconvention in the collector tree.
Other notes:
disappears, subsequent indices can shift.
caches share
object_size, so the exporter does not choose a representativevalue and does not aggregate duplicates.
kmem_cache_sanity_check()only WARNs and iscompiled out without
CONFIG_DEBUG_VM, which is why the kernel emits theseentries at all.
Tests
This collector previously had no dedicated test file. The new tests compare a
complete Prometheus exposition with
testutil.GatherAndCompare, so extraseries, missing series, wrong labels, wrong values and changed HELP or TYPE
lines all fail:
TestSlabinfoUniqueSlabNamesAreUnchanged: unique names carry noindexlabelTestSlabinfoDuplicateSlabNames: three entries sharing a name get 0, 1, 2TestSlabinfoMixedUniqueAndDuplicateSlabNames: both label shapes in one familyTestSlabinfoDuplicateSlabNamesDifferentGeometry: per-entry geometry preservedTestSlabinfoExcludedSlabsAreNotCollected: filtering, and that excluding onename leaves collision handling of the remaining names untouched
TestSlabinfoNoSlabinfoFile: missing/proc/slabinforeturns an error andemits nothing
The test collector deliberately implements an empty
Describe. Because theindexlabel is now conditional, one family can carry both descriptor shapes,and
Registry.Registerrejects two descriptors that share a fully-qualifiedname but declare different label names. Gathering mixed label sets is fine, only
the Describe-time check objects, and production never reaches it because
NodeCollector.Describereports just the two scrape descriptors. There is acomment on the type recording this so it is not "fixed" back to
DescribeByCollect.e2e fixtures
collector/fixtures/proc/slabinfonow contains the real duplicate-name casefrom the issue, two
mlx5_fs_ftesentries, so the golden output demonstratesboth metric shapes side by side.
Both golden files are
+10 / -0. Every pre-existing unique slab series isuntouched, which is the backwards-compatibility guarantee made visible in the
diff. Before adding the duplicate rows I ran the e2e suite against the unmodified
upstream goldens and the output was byte-identical.
Validation
gofmt,go vet ./...,go build ./...go test -short ./...and the full collector test suitegolangci-lint run ./...using the repository-pinned v2.12.2, 0 issues./end-to-end-test.sh