PMM-15198 Indexstats fix.

exporter/collstats_collector.go

@@ -26,7 +26,7 @@
 )
 
 type collstatsCollector struct {
 	ctx  context.Context
 	base *baseCollector
 
 	compatibleMode  bool
@@ -148,9 +148,7 @@
 		labels["collection"] = collection
 
 		for _, metrics := range stats {
-			if shard, ok := metrics["shard"].(string); ok {
-				labels["shard"] = shard
-			}
+			setShardLabel(labels, metrics)
 
 			for _, metric := range makeMetrics(prefix, metrics, labels, d.compatibleMode) {
 				ch <- metric

exporter/common.go

@@ -269,6 +269,25 @@ func splitNamespace(ns string) (string, string) {
 	return parts[0], strings.Join(parts[1:], ".")
 }
 
+// setShardLabel sets the optional "shard" label from a single $collStats or
+// $indexStats document. Callers may reuse one labels map across documents, so a
+// stale shard from a previous document is removed first, otherwise a document
+// without a shard would silently inherit it.
+//
+// Empty and absent shards are both treated as "no shard". Whether the label is
+// present has to be the same for every document of a metric name within one
+// scrape: the descriptors are built from these labels, and a registry rejects a
+// collector describing one fully-qualified name with two different label sets,
+// which makes the whole scrape fail. That holds in practice because $collStats
+// and $indexStats report a shard for every document through mongos and for none
+// of them otherwise.
+func setShardLabel(labels map[string]string, doc bson.M) {
+	delete(labels, "shard")
+	if shard, ok := doc["shard"].(string); ok && shard != "" {
+		labels["shard"] = shard
+	}
+}
+
 func fromMapToSlice(databases map[string][]string) []string {
 	var collections []string
 	for db, cols := range databases {

exporter/common_test.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.mongodb.org/mongo-driver/bson"
 	"go.mongodb.org/mongo-driver/mongo"
 
@@ -188,6 +189,44 @@ func TestSplitNamespace(t *testing.T) {
 	}
 }
 
+func TestSetShardLabel(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		doc  bson.M
+		want map[string]string
+	}{
+		{
+			name: "shard set",
+			doc:  bson.M{"shard": "shard-1"},
+			want: map[string]string{"database": "testdb", "shard": "shard-1"},
+		},
+		{
+			name: "empty shard",
+			doc:  bson.M{"shard": ""},
+			want: map[string]string{"database": "testdb"},
+		},
+		{
+			name: "shard field absent",
+			doc:  bson.M{},
+			want: map[string]string{"database": "testdb"},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			// A shard from a previous document must not leak into this one.
+			labels := map[string]string{"database": "testdb", "shard": "shard-0"}
+			setShardLabel(labels, test.doc)
+
+			require.Equal(t, test.want, labels)
+		})
+	}
+}
+
 //nolint:paralleltest
 func TestCheckNamespacesForViews(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)

exporter/indexstats_collector.go

@@ -131,10 +131,7 @@ func (d *indexstatsCollector) collect(ch chan<- prometheus.Metric) {
 			// prefix and labels are needed to avoid duplicated metric names since the metrics are the
 			// same, for different collections.
 			prefix := "indexstats"
-			labels := d.topologyInfo.baseLabels()
-			labels["database"] = database
-			labels["collection"] = collection
-			labels["key_name"] = indexName
+			labels := indexStatsLabels(d.topologyInfo.baseLabels(), database, collection, indexName, metric)
 
 			metrics := sanitizeMetrics(metric)
 			for _, metric := range makeMetrics(prefix, metrics, labels, false) {
@@ -144,6 +141,16 @@ func (d *indexstatsCollector) collect(ch chan<- prometheus.Metric) {
 	}
 }
 
+// indexStatsLabels builds the label set for a single $indexStats document.
+func indexStatsLabels(labels map[string]string, database, collection, indexName string, metric bson.M) map[string]string {
+	labels["database"] = database
+	labels["collection"] = collection
+	labels["key_name"] = indexName
+	setShardLabel(labels, metric)
+
+	return labels
+}
+
 // According to specs, we should expose only this 2 metrics. 'building' might not exist.
 func sanitizeMetrics(m bson.M) bson.M {
 	ops := float64(0)

exporter/indexstats_collector_test.go

@@ -25,7 +25,7 @@ import (
 	"github.com/AlekSi/pointer"
 	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/prometheus/common/promslog"
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.mongodb.org/mongo-driver/bson"
 	"go.mongodb.org/mongo-driver/bson/primitive"
 	"go.mongodb.org/mongo-driver/mongo"
@@ -50,7 +50,7 @@ func TestIndexStatsCollector(t *testing.T) {
 		collection := fmt.Sprintf("testcol_%02d", i)
 		for j := 0; j < 10; j++ {
 			_, err := database.Collection(collection).InsertOne(ctx, bson.M{"f1": j, "f2": "2"})
-			assert.NoError(t, err)
+			require.NoError(t, err)
 		}
 		mod := mongo.IndexModel{
 			Keys: bson.M{
@@ -60,7 +60,7 @@ func TestIndexStatsCollector(t *testing.T) {
 			},
 		}
 		_, err := database.Collection(collection).Indexes().CreateOne(ctx, mod)
-		assert.NoError(t, err)
+		require.NoError(t, err)
 	}
 
 	collection := []string{"testdb.testcol_00", "testdb.testcol_01", "testdb.testcol_02"}
@@ -82,7 +82,81 @@ mongodb_indexstats_accesses_ops{collection="testcol_02",database="testdb",key_na
 		"mongodb_indexstats_accesses_ops",
 	}
 	err := testutil.CollectAndCompare(c, expected, filter...)
-	assert.NoError(t, err)
+	require.NoError(t, err)
+}
+
+func TestIndexStatsLabels(t *testing.T) {
+	t.Parallel()
+
+	first := indexStatsLabels(
+		map[string]string{"cl_role": "mongos"},
+		"testdb",
+		"orders",
+		"_id_",
+		bson.M{"shard": "shard-0"},
+	)
+	second := indexStatsLabels(
+		map[string]string{"cl_role": "mongos"},
+		"testdb",
+		"orders",
+		"_id_",
+		bson.M{"shard": "shard-1"},
+	)
+
+	require.Equal(t, map[string]string{
+		"cl_role":    "mongos",
+		"database":   "testdb",
+		"collection": "orders",
+		"key_name":   "_id_",
+		"shard":      "shard-0",
+	}, first)
+	require.Equal(t, map[string]string{
+		"cl_role":    "mongos",
+		"database":   "testdb",
+		"collection": "orders",
+		"key_name":   "_id_",
+		"shard":      "shard-1",
+	}, second)
+}
+
+func TestIndexStatsLabelsWithoutShard(t *testing.T) {
+	t.Parallel()
+
+	t.Run("shard field absent", func(t *testing.T) {
+		t.Parallel()
+
+		labels := indexStatsLabels(
+			map[string]string{},
+			"testdb",
+			"orders",
+			"_id_",
+			bson.M{},
+		)
+
+		require.Equal(t, map[string]string{
+			"database":   "testdb",
+			"collection": "orders",
+			"key_name":   "_id_",
+		}, labels)
+	})
+
+	t.Run("shard field empty", func(t *testing.T) {
+		t.Parallel()
+
+		labels := indexStatsLabels(
+			map[string]string{},
+			"testdb",
+			"orders",
+			"_id_",
+			bson.M{"shard": ""},
+		)
+
+		require.Equal(t, map[string]string{
+			"database":   "testdb",
+			"collection": "orders",
+			"key_name":   "_id_",
+		}, labels)
+	})
 }
 
 func TestDescendingIndexOverride(t *testing.T) {
@@ -101,16 +175,16 @@ func TestDescendingIndexOverride(t *testing.T) {
 		collection := fmt.Sprintf("testcol_%02d", i)
 		for j := 0; j < 10; j++ {
 			_, err := database.Collection(collection).InsertOne(ctx, bson.M{"f1": j, "f2": "2"})
-			assert.NoError(t, err)
+			require.NoError(t, err)
 		}
 
 		descendingMod := mongo.IndexModel{Keys: bson.M{"f1": -1}}
 		_, err := database.Collection(collection).Indexes().CreateOne(ctx, descendingMod)
-		assert.NoError(t, err)
+		require.NoError(t, err)
 
 		ascendingMod := mongo.IndexModel{Keys: bson.M{"f1": 1}}
 		_, err = database.Collection(collection).Indexes().CreateOne(ctx, ascendingMod)
-		assert.NoError(t, err)
+		require.NoError(t, err)
 	}
 
 	collection := []string{"testdb.testcol_00", "testdb.testcol_01", "testdb.testcol_02"}
@@ -134,7 +208,7 @@ func TestDescendingIndexOverride(t *testing.T) {
 		"mongodb_indexstats_accesses_ops",
 	}
 	err := testutil.CollectAndCompare(c, expected, filter...)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 }
 
 func TestSanitize(t *testing.T) {
@@ -166,7 +240,7 @@ func TestSanitize(t *testing.T) {
 			"building": float64(1),
 		}
 		got := sanitizeMetrics(in)
-		assert.Equal(t, want, got)
+		require.Equal(t, want, got)
 	})
 
 	t.Run("Without building", func(t *testing.T) {
@@ -195,6 +269,6 @@ func TestSanitize(t *testing.T) {
 			},
 		}
 		got := sanitizeMetrics(in)
-		assert.Equal(t, want, got)
+		require.Equal(t, want, got)
 	})
 }