PMM-9652 Fixed anti-pattern metric names (#450)

percona-csalguero · web-flow · commit 572870c79c71 · 2022-03-28T09:19:29.000-03:00
* PMM-9652 WIP initial test * PMM-9652 Updated top exporter Now metric names are properly built and the namespace is in a label. * PMM-9652 Fixed anti-pattern metric names Metric names shouldn't vahe variable data like database or collection name as part of the name. The namespace and other information like the index nane should be set in a label. This patch fixes the metric names in collstas, indexstats and topstats collectors. * Ran format * PMM-9652 Updated labels to have database+collection * Updated labels for top collector * Removed old comment * Format
diff --git a/exporter/collstats_collector.go b/exporter/collstats_collector.go
@@ -120,11 +120,7 @@ func (d *collstatsCollector) collect(ch chan<- prometheus.Metric) {
 		logger.Debugf("$collStats metrics for %s.%s", database, collection)
 		debugResult(logger, stats)
 
-		// Since all collections will have the same fields, we need to use a metric prefix (db+col)
-		// to differentiate metrics between collection. Labels are being set only to matke it easier
-		// to filter
-		prefix := database + "." + collection
-
+		prefix := "collstats"
 		labels := d.topologyInfo.baseLabels()
 		labels["database"] = database
 		labels["collection"] = collection
diff --git a/exporter/collstats_collector_test.go b/exporter/collstats_collector_test.go
@@ -58,24 +58,31 @@ func TestCollStatsCollector(t *testing.T) {
 
 	// The last \n at the end of this string is important
 	expected := strings.NewReader(`
-	# HELP mongodb_testdb_testcol_00_latencyStats_commands_latency testdb.testcol_00.latencyStats.commands.
-	# TYPE mongodb_testdb_testcol_00_latencyStats_commands_latency untyped
-	mongodb_testdb_testcol_00_latencyStats_commands_latency{collection="testcol_00",database="testdb"} 0
-	# HELP mongodb_testdb_testcol_01_latencyStats_commands_latency testdb.testcol_01.latencyStats.commands.
-	# TYPE mongodb_testdb_testcol_01_latencyStats_commands_latency untyped
-	mongodb_testdb_testcol_01_latencyStats_commands_latency{collection="testcol_01",database="testdb"} 0
-	# HELP mongodb_testdb_testcol_02_latencyStats_commands_latency testdb.testcol_02.latencyStats.commands.
-	# TYPE mongodb_testdb_testcol_02_latencyStats_commands_latency untyped
-	mongodb_testdb_testcol_02_latencyStats_commands_latency{collection="testcol_02",database="testdb"} 0` + "\n")
+# HELP mongodb_collstats_latencyStats_commands_latency collstats.latencyStats.commands.
+# TYPE mongodb_collstats_latencyStats_commands_latency untyped
+mongodb_collstats_latencyStats_commands_latency{collection="testcol_00",database="testdb"} 0
+mongodb_collstats_latencyStats_commands_latency{collection="testcol_01",database="testdb"} 0
+mongodb_collstats_latencyStats_commands_latency{collection="testcol_02",database="testdb"} 0
+# HELP mongodb_collstats_latencyStats_transactions_ops collstats.latencyStats.transactions.
+# TYPE mongodb_collstats_latencyStats_transactions_ops untyped
+mongodb_collstats_latencyStats_transactions_ops{collection="testcol_00",database="testdb"} 0
+mongodb_collstats_latencyStats_transactions_ops{collection="testcol_01",database="testdb"} 0
+mongodb_collstats_latencyStats_transactions_ops{collection="testcol_02",database="testdb"} 0
+# HELP mongodb_collstats_storageStats_capped collstats.storageStats.
+# TYPE mongodb_collstats_storageStats_capped untyped
+mongodb_collstats_storageStats_capped{collection="testcol_00",database="testdb"} 0
+mongodb_collstats_storageStats_capped{collection="testcol_01",database="testdb"} 0
+mongodb_collstats_storageStats_capped{collection="testcol_02",database="testdb"} 0` +
+		"\n")
 
 	// Filter metrics for 2 reasons:
 	// 1. The result is huge
 	// 2. We need to check against know values. Don't use metrics that return counters like uptime
 	//    or counters like the number of transactions because they won't return a known value to compare
 	filter := []string{
-		"mongodb_testdb_testcol_00_latencyStats_commands_latency",
-		"mongodb_testdb_testcol_01_latencyStats_commands_latency",
-		"mongodb_testdb_testcol_02_latencyStats_commands_latency",
+		"mongodb_collstats_latencyStats_commands_latency",
+		"mongodb_collstats_storageStats_capped",
+		"mongodb_collstats_latencyStats_transactions_ops",
 	}
 	err := testutil.CollectAndCompare(c, expected, filter...)
 	assert.NoError(t, err)
diff --git a/exporter/common.go b/exporter/common.go
@@ -217,3 +217,12 @@ func nonSystemCollectionsCount(ctx context.Context, client *mongo.Client, includ
 
 	return count, nil
 }
+
+func splitNamespace(ns string) (database, collection string) {
+	parts := strings.Split(ns, ".")
+	if len(parts) < 2 { // there is no collection?
+		return parts[0], ""
+	}
+
+	return parts[0], strings.Join(parts[1:], ".")
+}
diff --git a/exporter/common_test.go b/exporter/common_test.go
@@ -148,3 +148,28 @@ func TestListCollections(t *testing.T) {
 		assert.Equal(t, 6, count)
 	})
 }
+
+func TestSplitNamespace(t *testing.T) {
+	testCases := []struct {
+		namespace      string
+		wantDatabase   string
+		wantCollection string
+	}{
+		{
+			namespace:      "db.collection",
+			wantDatabase:   "db",
+			wantCollection: "collection",
+		},
+		{
+			namespace:      "db.this.is.a.valid.collection.name",
+			wantDatabase:   "db",
+			wantCollection: "this.is.a.valid.collection.name",
+		},
+	}
+
+	for _, tc := range testCases {
+		db, coll := splitNamespace(tc.namespace)
+		assert.Equal(t, tc.wantDatabase, db)
+		assert.Equal(t, tc.wantCollection, coll)
+	}
+}
diff --git a/exporter/indexstats_collector.go b/exporter/indexstats_collector.go
@@ -101,15 +101,17 @@ func (d *indexstatsCollector) collect(ch chan<- prometheus.Metric) {
 			continue
 		}
 
-		logger.Debugf("indexStats for %s.%s", database, collection)
-		debugResult(logger, stats)
+		d.base.logger.Debugf("indexStats for %s.%s", database, collection)
+
+		debugResult(d.base.logger, stats)
 
 		for _, metric := range stats {
 			// prefix and labels are needed to avoid duplicated metric names since the metrics are the
 			// same, for different collections.
-			prefix := fmt.Sprintf("%s_%s_%s", database, collection, metric["name"])
+			prefix := "indexstats"
 			labels := d.topologyInfo.baseLabels()
-			labels["namespace"] = database + "." + collection
+			labels["database"] = database
+			labels["collection"] = collection
 			labels["key_name"] = fmt.Sprintf("%s", metric["name"])
 
 			metrics := sanitizeMetrics(metric)
diff --git a/exporter/indexstats_collector_test.go b/exporter/indexstats_collector_test.go
@@ -69,24 +69,15 @@ func TestIndexStatsCollector(t *testing.T) {
 
 	// The last \n at the end of this string is important
 	expected := strings.NewReader(`
-	# HELP mongodb_testdb_testcol_00_id_accesses_ops testdb_testcol_00__id_.accesses.
-	# TYPE mongodb_testdb_testcol_00_id_accesses_ops untyped
-	mongodb_testdb_testcol_00_id_accesses_ops{key_name="_id_",namespace="testdb.testcol_00"} 0
-	# HELP mongodb_testdb_testcol_00_idx_01_accesses_ops testdb_testcol_00_idx_01.accesses.
-	# TYPE mongodb_testdb_testcol_00_idx_01_accesses_ops untyped
-	mongodb_testdb_testcol_00_idx_01_accesses_ops{key_name="idx_01",namespace="testdb.testcol_00"} 0
-	# HELP mongodb_testdb_testcol_01_id_accesses_ops testdb_testcol_01__id_.accesses.
-	# TYPE mongodb_testdb_testcol_01_id_accesses_ops untyped
-	mongodb_testdb_testcol_01_id_accesses_ops{key_name="_id_",namespace="testdb.testcol_01"} 0
-	# HELP mongodb_testdb_testcol_01_idx_01_accesses_ops testdb_testcol_01_idx_01.accesses.
-	# TYPE mongodb_testdb_testcol_01_idx_01_accesses_ops untyped
-	mongodb_testdb_testcol_01_idx_01_accesses_ops{key_name="idx_01",namespace="testdb.testcol_01"} 0
-	# HELP mongodb_testdb_testcol_02_id_accesses_ops testdb_testcol_02__id_.accesses.
-	# TYPE mongodb_testdb_testcol_02_id_accesses_ops untyped
-	mongodb_testdb_testcol_02_id_accesses_ops{key_name="_id_",namespace="testdb.testcol_02"} 0
-	# HELP mongodb_testdb_testcol_02_idx_01_accesses_ops testdb_testcol_02_idx_01.accesses.
-	# TYPE mongodb_testdb_testcol_02_idx_01_accesses_ops untyped
-mongodb_testdb_testcol_02_idx_01_accesses_ops{key_name="idx_01",namespace="testdb.testcol_02"} 0` + "\n")
+# HELP mongodb_indexstats_accesses_ops indexstats.accesses.
+# TYPE mongodb_indexstats_accesses_ops untyped
+mongodb_indexstats_accesses_ops{collection="testcol_00",database="testdb",key_name="_id_"} 0
+mongodb_indexstats_accesses_ops{collection="testcol_00",database="testdb",key_name="idx_01"} 0
+mongodb_indexstats_accesses_ops{collection="testcol_01",database="testdb",key_name="_id_"} 0
+mongodb_indexstats_accesses_ops{collection="testcol_01",database="testdb",key_name="idx_01"} 0
+mongodb_indexstats_accesses_ops{collection="testcol_02",database="testdb",key_name="_id_"} 0
+mongodb_indexstats_accesses_ops{collection="testcol_02",database="testdb",key_name="idx_01"} 0` +
+		"\n")
 
 	err := testutil.CollectAndCompare(c, expected)
 	assert.NoError(t, err)
diff --git a/exporter/metrics.go b/exporter/metrics.go
@@ -161,11 +161,16 @@ func makeRawMetric(prefix, name string, value interface{}, labels map[string]str
 
 	fqName, label := nameAndLabel(prefix, name)
 
+	metricType := prometheus.UntypedValue
+	if strings.HasSuffix(strings.ToLower(name), "count") {
+		metricType = prometheus.CounterValue
+	}
+
 	rm := &rawMetric{
 		fqName: fqName,
 		help:   help,
 		val:    *f,
-		vt:     prometheus.UntypedValue,
+		vt:     metricType,
 		ln:     make([]string, 0, len(labels)),
 		lv:     make([]string, 0, len(labels)),
 	}
diff --git a/exporter/metrics_test.go b/exporter/metrics_test.go
@@ -163,7 +163,7 @@ func TestMakeRawMetric(t *testing.T) {
 				ln:     ln,
 				lv:     lv,
 				val:    *tc.wantVal,
-				vt:     3,
+				vt:     prometheus.CounterValue,
 			}
 		}
 
diff --git a/exporter/top_collector.go b/exporter/top_collector.go
@@ -18,10 +18,12 @@ package exporter
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/sirupsen/logrus"
 	"go.mongodb.org/mongo-driver/bson"
+	"go.mongodb.org/mongo-driver/bson/primitive"
 	"go.mongodb.org/mongo-driver/mongo"
 )
 
@@ -33,12 +35,14 @@ type topCollector struct {
 	topologyInfo   labelsGetter
 }
 
-// newTopCollector creates a collector for statistics on collection usage.
-func newTopCollector(ctx context.Context, client *mongo.Client, logger *logrus.Logger, compatible bool, topology labelsGetter) *topCollector {
-	return &topCollector{
-		ctx:  ctx,
-		base: newBaseCollector(client, logger),
+var ErrInvalidOrMissingTotalsEntry = fmt.Errorf("Invalid or misssing totals entry in top results")
 
+func newTopCollector(ctx context.Context, client *mongo.Client, logger *logrus.Logger, compatible bool,
+	topology labelsGetter,
+) *topCollector {
+	return &topCollector{
+		ctx:            ctx,
+		base:           newBaseCollector(client, logger),
 		compatibleMode: compatible,
 		topologyInfo:   topology,
 	}
@@ -59,7 +63,7 @@ func (d *topCollector) collect(ch chan<- prometheus.Metric) {
 	cmd := bson.D{{Key: "top", Value: "1"}}
 	res := client.Database("admin").RunCommand(d.ctx, cmd)
 
-	var m bson.M
+	var m primitive.M
 	if err := res.Decode(&m); err != nil {
 		ch <- prometheus.NewInvalidMetric(prometheus.NewInvalidDesc(err), err)
 		return
@@ -68,7 +72,85 @@ func (d *topCollector) collect(ch chan<- prometheus.Metric) {
 	logrus.Debug("top result:")
 	debugResult(logger, m)
 
-	for _, metric := range makeMetrics("top", m, d.topologyInfo.baseLabels(), d.compatibleMode) {
-		ch <- metric
+	totals, ok := m["totals"].(primitive.M)
+	if !ok {
+		ch <- prometheus.NewInvalidMetric(prometheus.NewInvalidDesc(ErrInvalidOrMissingTotalsEntry),
+			ErrInvalidOrMissingTotalsEntry)
+	}
+
+	/*
+		      The top command will return a structure with a key named totals and it is a map
+			  where the key is the collection namespace and for each collection there are per
+			  collection usage statistics.
+			  Example: 	rs1:SECONDARY> db.adminCommand({"top": 1});
+
+		      {
+		        "totals" : {
+		                "note" : "all times in microseconds",
+		                "admin.system.roles" : {
+		                        "total" : {
+		                                "time" : 41,
+		                                "count" : 1
+		                        },
+		                        "readLock" : {
+		                                "time" : 41,
+		                                "count" : 1
+		                        },
+		                        "writeLock" : {
+		                                "time" : 0,
+		                                "count" : 0
+		                        },
+		                        "queries" : {
+		                                "time" : 41,
+		                                "count" : 1
+		                        },
+		                        "getmore" : {
+		                                "time" : 0,
+		                                "count" : 0
+		                        },
+		                        "insert" : {
+		                                "time" : 0,
+		                                "count" : 0
+		                        },
+		                        "update" : {
+		                                "time" : 0,
+		                                "count" : 0
+		                        },
+		                        "remove" : {
+		                                "time" : 0,
+		                                "count" : 0
+		                        },
+		                        "commands" : {
+		                                "time" : 0,
+		                                "count" : 0
+		                        }
+		                },
+		                "admin.system.version" : {
+		                        "total" : {
+		                                "time" : 63541,
+		                                "count" : 218
+		                        },
+
+		      If we pass this structure to the makeMetrics function, we will have metric names with the form of
+			  prefix + namespace + metric like mongodb_top_totals_admin.system.role_readlock_count.
+			  Having the namespace as part of the metric is a Prometheus anti pattern and diffucults grouping
+			  metrics in Grafana. For this reason, we need to manually loop through the metric in the totals key
+			  and pass the namespace as a label to the makeMetrics function.
+	*/
+
+	for namespace, metrics := range totals {
+		labels := d.topologyInfo.baseLabels()
+		db, coll := splitNamespace(namespace)
+		labels["database"] = db
+		labels["collection"] = coll
+
+		mm, ok := metrics.(primitive.M) // ingore entries like -> "note" : "all times in microseconds"
+		if !ok {
+			continue
+		}
+
+		for _, metric := range makeMetrics("top", mm, labels, d.compatibleMode) {
+			ch <- metric
+		}
 	}
 }
diff --git a/exporter/top_collector_test.go b/exporter/top_collector_test.go
@@ -18,7 +18,6 @@ package exporter
 
 import (
 	"context"
-	"strings"
 	"testing"
 	"time"
 
@@ -39,18 +38,38 @@ func TestTopCollector(t *testing.T) {
 
 	c := newTopCollector(ctx, client, logrus.New(), false, ti)
 
-	// The last \n at the end of this string is important
-	expected := strings.NewReader(`
-	# HELP mongodb_top_ok top.
-	# TYPE mongodb_top_ok untyped
-	mongodb_top_ok 1` + "\n")
 	// Filter metrics for 2 reasons:
 	// 1. The result is huge
 	// 2. We need to check against know values. Don't use metrics that return counters like uptime
 	//    or counters like the number of transactions because they won't return a known value to compare
 	filter := []string{
-		"mongodb_top_ok",
+		"mongodb_top_update_count",
 	}
-	err := testutil.CollectAndCompare(c, expected, filter...)
-	assert.NoError(t, err)
+	filter = nil
+	count := testutil.CollectAndCount(c, filter...)
+
+	/*
+		      The number of metrics is not a constant. It depends on the number of collections in the db.
+			  It looks like:
+
+		      # HELP mongodb_top_update_count top.update.
+		      # TYPE mongodb_top_update_count untyped
+		      mongodb_top_update_count{namespace="admin.system.roles"} 0
+		      mongodb_top_update_count{namespace="admin.system.version"} 3
+		      mongodb_top_update_count{namespace="config.cache.chunks.config.system.sessions"} 0
+		      mongodb_top_update_count{namespace="config.cache.collections"} 1540
+		      mongodb_top_update_count{namespace="config.image_collection"} 0
+		      mongodb_top_update_count{namespace="config.system.sessions"} 12
+		      mongodb_top_update_count{namespace="config.transaction_coordinators"} 0
+		      mongodb_top_update_count{namespace="config.transactions"} 0
+		      mongodb_top_update_count{namespace="local.oplog.rs"} 0
+		      mongodb_top_update_count{namespace="local.replset.election"} 0
+		      mongodb_top_update_count{namespace="local.replset.minvalid"} 0
+		      mongodb_top_update_count{namespace="local.replset.oplogTruncateAfterPoint"} 0
+		      mongodb_top_update_count{namespace="local.startup_log"} 0
+		      mongodb_top_update_count{namespace="local.system.replset"} 0
+		      mongodb_top_update_count{namespace="local.system.rollback.id"} 0
+
+	*/
+	assert.True(t, count > 0)
 }

Original file line number	Diff line number	Diff line change
`@@ -163,7 +163,7 @@ func TestMakeRawMetric(t *testing.T) {`
`163`	`163`	`ln: ln,`
`164`	`164`	`lv: lv,`
`165`	`165`	`val: *tc.wantVal,`
`166`		`- vt: 3,`
	`166`	`+ vt: prometheus.CounterValue,`
`167`	`167`	`}`
`168`	`168`	`}`
`169`	`169`