PMM-15198 Indexstats fix.#1321
Conversation
|
@copilot review |
Reviewed the current PR changes in the indexstats area; I don’t see additional actionable fixes to apply right now. I also re-ran targeted tests for the updated helper logic ( |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@copilot review |
ademidoff
left a comment
ademidoff
left a comment
There was a problem hiding this comment.
A few minor suggestions, nothing blocking.
| labels["database"] = database | ||
| labels["collection"] = collection | ||
| labels["key_name"] = indexName | ||
| if shard, ok := metric["shard"].(string); ok && shard != "" { |
There was a problem hiding this comment.
Minor: the empty-string guard makes indexstats behave differently from collstats, which adds the label on ok alone (collstats_collector.go:151). Either the guard matters — then collstats should get it too — or it can't happen and it's dead code here. Since it's a two-line change under the same ticket, consider applying the same guard to collstats in this PR.
Context: label dimensions must be consistent per metric name within a single scrape, so if a mongos ever returned shard: "" for some documents but real names for others, dropping the label on only those series would make the whole mongodb_indexstats_* family fail to gather. Theoretical, but worth keeping the two collectors aligned.
There was a problem hiding this comment.
Good catch. Applied your primary suggestion: kept the && shard != "" guard in indexstats and added the same guard to collstats (collstats_collector.go:151), so both collectors now behave identically.
| "key_name": "_id_", | ||
| "shard": "shard-1", | ||
| }, second) | ||
| assert.NotEqual(t, first, second) |
There was a problem hiding this comment.
Minor: this assertion is redundant — the two exact-map assertions above already guarantee first != second.
| "testdb", | ||
| "orders", | ||
| "_id_", | ||
| bson.M{"shard": ""}, |
There was a problem hiding this comment.
This only covers shard: "". The more common real case is the field being entirely absent (any non-mongos node) — consider adding a bson.M{} input as well.
There was a problem hiding this comment.
Added test case.
|
@copilot review |
| } | ||
| } | ||
|
|
||
| func setShardLabel(labels map[string]string, metrics bson.M) { |
There was a problem hiding this comment.
Although PMM-15198 primarily targets indexstats, collstats uses the same optional shard label and was updated to keep both collectors consistent. Since collstats reuses the labels map across results, an empty or absent shard could otherwise retain the previous shard value and mislabel metrics. The added tests cover both cases.
PMM-15198
Adds a
shardlabel to indexstats metrics. When scraping through mongos,$indexStatsreturns one document per shard for the same index, so without the label the collector emitted duplicate series (same name and labels) and the per-scrape registry rejected them.Also raises the
go testtimeout from 30s to 1m in the Makefile. The 30s limit applies to the wholeexporterpackage, whose integration suite takes ~29s in CI under-racewith coverage instrumentation (recent successful runs: 29.250s and 28.698s). With under a second of headroom, any runner jitter makes the test binary panic withtest timed out after 30s, blaming whatever test happens to be running at that moment — see https://github.com/percona/mongodb_exporter/actions/runs/30005948435/job/89201852367 for an example. Individual tests remain bounded by their own context timeouts, so genuine hangs are still caught quickly.