Repository: grafana/dskit (go)

Issue:
Migrate metric test helpers from Mimir
Mimir has a couple of test helper methods for testing emitted metrics that would be good to migrate to dskit:

> I was wondering if we could reuse the utilities from the `metrics` package, but I don't think we can out of the box. Not high priority, but I think in Mimir we have most of the missing utilities ready and we could move them to dskit:
> - `findHistogramMatchingLabels()` from `pkg/frontend/querymiddleware/codec_json_test.go`
> - `getMetricsMatchingLabels()` from `pkg/storegateway/bucket_e2e_test.go`
> - `MatchesSelectors()` from `pkg/util`

From https://github.com/grafana/dskit/pull/344/files#r1285937024

Pull request:
Migrate metric test helpers from Mimir 
**What this PR does**:

This PR migrates three test helpers we use frequently in Mimir to test metrics emitted during a test.

**Which issue(s) this PR fixes**:

Fixes #351.

**Checklist**
- [x] Tests updated
- [x] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e1c56b1..78474c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -138,6 +138,7 @@
 * [ENHANCEMENT] Migrate `github.com/weaveworks/common/aws` and `github.com/weaveworks/common/test` packages to corresponding packages in `github.com/grafana/dskit`. #356
 * [ENHANCEMENT] Ring: optionally emit logs and trace events in `DoUntilQuorum`. #361
 * [ENHANCEMENT] metrics: Add `MetricFamilyMap.MinGauges` and `MetricFamiliesPerTenant.SendMinOfGauges` methods. #366
+* [ENHANCEMENT] metrics: add `MatchesLabels`, `FindMetricsInFamilyMatchingLabels` and `FindHistogramWithNameAndLabels` test helper methods. #365
 * [BUGFIX] spanlogger: Support multiple tenant IDs. #59
 * [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85
 * [BUGFIX] Ring: `ring_member_ownership_percent` and `ring_tokens_owned` metrics are not updated on scale down. #109
diff --git a/metrics/helpers.go b/metrics/helpers.go
new file mode 100644
index 0000000..873377f
--- /dev/null
+++ b/metrics/helpers.go
@@ -0,0 +1,64 @@
+package metrics
+
+import (
+	"errors"
+	"fmt"
+
+	dto "github.com/prometheus/client_model/go"
+)
+
+// MatchesLabels returns true if m has all the labels in l.
+func MatchesLabels(m *dto.Metric, labelNamesAndValues ...string) bool {
+	for _, l := range makeLabels(labelNamesAndValues...) {
+		found := false
+		for _, lp := range m.GetLabel() {
+			if l.GetName() != lp.GetName() || l.GetValue() != lp.GetValue() {
+				continue
+			}
+
+			found = true
+			break
+		}
+
+		if !found {
+			return false
+		}
+	}
+
+	return true
+}
+
+// FindMetricsInFamilyMatchingLabels returns all the metrics in mf that match the labels in labelNamesAndValues.
+func FindMetricsInFamilyMatchingLabels(mf *dto.MetricFamily, labelNamesAndValues ...string) []*dto.Metric {
+	var result []*dto.Metric
+	for _, m := range mf.GetMetric() {
+		if MatchesLabels(m, labelNamesAndValues...) {
+			result = append(result, m)
+		}
+	}
+	return result
+}
+
+// FindHistogramWithNameAndLabels returns the histogram in metrics with name that matches the labels in labelNamesAndValues.
+// If no histogram with name is present in metrics, if labelNamesAndValues matches anything other than exactly one metric,
+// or if the matching metric is not a histogram, an error is returned.
+func FindHistogramWithNameAndLabels(metrics MetricFamilyMap, name string, labelNamesAndValues ...string) (*dto.Histogram, error) {
+	metricFamily, ok := metrics[name]
+	if !ok {
+		return nil, fmt.Errorf("no metric with name '%v' found", name)
+	}
+
+	matchingMetrics := FindMetricsInFamilyMatchingLabels(metricFamily, labelNamesAndValues...)
+
+	if len(matchingMetrics) != 1 {
+		return nil, fmt.Errorf("wanted exactly one matching metric, but found %v", len(matchingMetrics))
+	}
+
+	metric := matchingMetrics[0]
+
+	if metric.Histogram == nil {
+		return nil, errors.New("found a single matching metric, but it is not a histogram")
+	}
+
+	return metric.Histogram, nil
+}
diff --git a/metrics/tenant_registries.go b/metrics/tenant_registries.go
index f0556a0..77a9846 100644
--- a/metrics/tenant_registries.go
+++ b/metrics/tenant_registries.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
+	"github.com/gogo/protobuf/proto"
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
 )
@@ -822,3 +823,16 @@ type metricOptions struct {
 	skipZeroValueMetrics bool
 	labelNames           []string
 }
+
+func makeLabels(namesAndValues ...string) []*dto.LabelPair {
+	out := []*dto.LabelPair(nil)
+
+	for i := 0; i+1 < len(namesAndValues); i = i + 2 {
+		out = append(out, &dto.LabelPair{
+			Name:  proto.String(namesAndValues[i]),
+			Value: proto.String(namesAndValues[i+1]),
+		})
+	}
+
+	return out
+}


Test patch:
diff --git a/metrics/helpers_test.go b/metrics/helpers_test.go
new file mode 100644
index 0000000..140d8ae
--- /dev/null
+++ b/metrics/helpers_test.go
@@ -0,0 +1,161 @@
+package metrics
+
+import (
+	"testing"
+
+	dto "github.com/prometheus/client_model/go"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMatchesLabels(t *testing.T) {
+	m := dto.Metric{Label: makeLabels(
+		"label-1", "value-1",
+		"label-2", "value-2",
+	)}
+
+	testCases := map[string]struct {
+		labels        []string
+		expectedMatch bool
+	}{
+		"no labels": {
+			expectedMatch: true,
+		},
+		"one matching label": {
+			labels:        []string{"label-1", "value-1"},
+			expectedMatch: true,
+		},
+		"all matching labels": {
+			labels: []string{
+				"label-1", "value-1",
+				"label-2", "value-2",
+			},
+			expectedMatch: true,
+		},
+		"different value for label in metric": {
+			labels:        []string{"label-1", "value-2"},
+			expectedMatch: false,
+		},
+		"label not on metric": {
+			labels:        []string{"label-3", "value-2"},
+			expectedMatch: false,
+		},
+		"one value matches, one does not": {
+			labels: []string{
+				"label-1", "value-1",
+				"label-2", "value-3",
+			},
+			expectedMatch: false,
+		},
+		"one value matches, one label not on metric": {
+			labels: []string{
+				"label-1", "value-1",
+				"label-3", "value-2",
+			},
+			expectedMatch: false,
+		},
+	}
+
+	for name, testCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			actual := MatchesLabels(&m, testCase.labels...)
+			require.Equal(t, testCase.expectedMatch, actual)
+		})
+	}
+}
+
+func TestFindMetricsInFamilyMatchingLabels(t *testing.T) {
+	m1 := &dto.Metric{
+		Label: makeLabels(
+			"unique-label", "metric-1",
+			"shared-label", "shared-value",
+		),
+	}
+	m2 := &dto.Metric{
+		Label: makeLabels(
+			"unique-label", "metric-2",
+			"shared-label", "shared-value",
+		),
+	}
+	mf := &dto.MetricFamily{
+		Metric: []*dto.Metric{m1, m2},
+	}
+
+	require.Empty(t, FindMetricsInFamilyMatchingLabels(mf, "another-label", "some-value"))
+	require.Empty(t, FindMetricsInFamilyMatchingLabels(mf, "unique-label", "another-value"))
+	require.ElementsMatch(t, FindMetricsInFamilyMatchingLabels(mf, "unique-label", "metric-1"), []*dto.Metric{m1})
+	require.ElementsMatch(t, FindMetricsInFamilyMatchingLabels(mf, "shared-label", "shared-value"), []*dto.Metric{m1, m2})
+}
+
+func TestFindHistogramWithNameAndLabels(t *testing.T) {
+	metricName := "the_metric"
+
+	histogramMetric := &dto.Metric{
+		Label: makeLabels(
+			"unique-label", "histogram-metric",
+			"shared-label", "shared-value",
+		),
+		Histogram: &dto.Histogram{},
+	}
+
+	nonHistogramMetric := &dto.Metric{
+		Label: makeLabels(
+			"unique-label", "non-histogram-metric",
+			"shared-label", "shared-value",
+		),
+	}
+
+	mf := &dto.MetricFamily{
+		Name:   &metricName,
+		Metric: []*dto.Metric{histogramMetric, nonHistogramMetric},
+	}
+
+	mfm := MetricFamilyMap{
+		metricName: mf,
+	}
+
+	testCases := map[string]struct {
+		metricName        string
+		labels            []string
+		expectedHistogram *dto.Histogram
+		expectedError     string
+	}{
+		"no matching metric family": {
+			metricName:    "another_metric",
+			expectedError: "no metric with name 'another_metric' found",
+		},
+		"matching metric family, no matching metrics": {
+			metricName:    metricName,
+			labels:        []string{"unique-label", "another-value"},
+			expectedError: "wanted exactly one matching metric, but found 0",
+		},
+		"matching metric family, multiple matching metrics": {
+			metricName:    metricName,
+			labels:        []string{"shared-label", "shared-value"},
+			expectedError: "wanted exactly one matching metric, but found 2",
+		},
+		"matching metric family, one matching metric, metric is not a histogram": {
+			metricName:    metricName,
+			labels:        []string{"unique-label", "non-histogram-metric"},
+			expectedError: "found a single matching metric, but it is not a histogram",
+		},
+		"matching metric family, one matching metric, metric is a histogram": {
+			metricName:        metricName,
+			labels:            []string{"unique-label", "histogram-metric"},
+			expectedHistogram: histogramMetric.Histogram,
+		},
+	}
+
+	for name, testCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			actualHistogram, err := FindHistogramWithNameAndLabels(mfm, testCase.metricName, testCase.labels...)
+
+			if testCase.expectedError == "" {
+				require.NoError(t, err)
+				require.Equal(t, testCase.expectedHistogram, actualHistogram)
+			} else {
+				require.EqualError(t, err, testCase.expectedError)
+				require.Nil(t, actualHistogram)
+			}
+		})
+	}
+}
diff --git a/metrics/tenant_registries_test.go b/metrics/tenant_registries_test.go
index 457a2e9..c31fe81 100644
--- a/metrics/tenant_registries_test.go
+++ b/metrics/tenant_registries_test.go
@@ -134,19 +134,6 @@ func BenchmarkGetMetricsWithLabelNames(b *testing.B) {
 	}
 }
 
-func makeLabels(namesAndValues ...string) []*dto.LabelPair {
-	out := []*dto.LabelPair(nil)
-
-	for i := 0; i+1 < len(namesAndValues); i = i + 2 {
-		out = append(out, &dto.LabelPair{
-			Name:  proto.String(namesAndValues[i]),
-			Value: proto.String(namesAndValues[i+1]),
-		})
-	}
-
-	return out
-}
-
 // TestSendSumOfGaugesPerTenantWithLabels tests to ensure multiple metrics for the same tenant with a matching label are
 // summed correctly.
 func TestSendSumOfGaugesPerTenantWithLabels(t *testing.T) {
diff --git a/middleware/grpc_instrumentation_test.go b/middleware/grpc_instrumentation_test.go
index bf01292..65098f3 100644
--- a/middleware/grpc_instrumentation_test.go
+++ b/middleware/grpc_instrumentation_test.go
@@ -25,6 +25,7 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/grafana/dskit/httpgrpc"
+	"github.com/grafana/dskit/metrics"
 )
 
 func TestErrorCode_NoError(t *testing.T) {
@@ -246,18 +247,19 @@ func requireRequestMetrics(t *testing.T, g prometheus.Gatherer, result string, c
 }
 
 func gatherRequestMetrics(t *testing.T, g prometheus.Gatherer) string {
-	got, err := g.Gather()
+	metricMap, err := metrics.NewMetricFamilyMapFromGatherer(g)
 	require.NoError(t, err)
 
-	actual := ""
+	mf := metricMap["client_request_duration_seconds"]
+	if mf == nil {
+		return ""
+	}
 
-	for _, mf := range got {
-		require.Equal(t, "client_request_duration_seconds", *mf.Name)
+	actual := ""
 
-		for _, m := range mf.Metric {
-			require.NotNil(t, m.Histogram)
-			actual += fmt.Sprintf(`%v_count%v %v`, *mf.Name, formatLabelPairs(m.Label), *m.Histogram.SampleCount)
-		}
+	for _, m := range mf.Metric {
+		require.NotNil(t, m.Histogram)
+		actual += fmt.Sprintf(`%v_count%v %v`, *mf.Name, formatLabelPairs(m.Label), *m.Histogram.SampleCount)
 	}
 
 	return actual