veneur-prometheus bugfix: Specifying multiple tags/labels with -a causes sporadic incorrect metric emission

CHANGELOG.md

@@ -19,6 +19,7 @@
 * A fix for forwarding metrics with gRPC using the kubernetes discoverer. Thanks, [androohan](https://github.com/androohan)!
 * Regenerate testing certs/CA that have expired and have broken tests. Thanks, [randallm](https://github.com/randallm)
 * The config field `trace_lightstep_access_token` is redacted if printed. Thanks [arnavdugar](https://github.com/arnavdugar)!
+* A fix for the -a flag in veneur-prometheus. Thanks, [dmarriner](https://github.com/dmarriner)
 
 # 14.1.0, 2021-03-16
 

cmd/veneur-prometheus/main_test.go

@@ -301,6 +301,51 @@ func TestHistogramsNewBucketsAreTranslatedToDiffs(t *testing.T) {
 	assert.False(t, hasbucket5)
 }
 
+// This test verifies a fix for a previous bug that caused diff calculations to sporadically break when multiple labels
+// were added via the -a flag.  Labels were stored as key/values in a map and returned in a potentially different order
+// each time since map iteration order is random.  This caused the translator to emit the cumulative value instead of
+// the diffed value, because it would seem as though the metric was new when in fact it had been previously cached
+// with a different label ordering.  For example, the translator might look for "counter-key2:value2-key1:value1" when
+// it had been previously cached as "counter-key1:value1-key2:value2"
+func TestCountsWithAddingLabels(t *testing.T) {
+	counter := prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "counter",
+		Help: "A typical counter.",
+	})
+
+	ts, err := testPrometheusEndpoint(
+		counter,
+	)
+	require.NoError(t, err)
+	defer ts.Close()
+
+	cache := new(countCache)
+	cfg := testConfig(t, ts.URL)
+	cfg.addedLabels = map[string]string{
+		"key1": "value1",
+		"key2": "value2",
+	}
+	statsClient, err := statsd.New("localhost:8125", statsd.WithoutTelemetry())
+	assert.NoError(t, err)
+
+	stats, err := collect(context.Background(), cfg, statsClient, cache)
+	assert.NoError(t, err)
+	splitStats(stats)
+
+	// Run 100 time to ensure we aren't just passing the test we got lucky
+	for i := 0; i < 100; i++ {
+		counter.Inc()
+		counter.Inc()
+		counter.Inc()
+
+		stats, err = collect(context.Background(), cfg, statsClient, cache)
+		assert.NoError(t, err)
+		counts, _ := splitStats(stats)
+		count, _ := countValue(counts, "counter", "key1:value1", "key2:value2")
+		assert.Equal(t, 3, count)
+	}
+}
+
 func TestHistogramsAreTranslatedToDiffs(t *testing.T) {
 	histogram := prometheus.NewHistogram(prometheus.HistogramOpts{
 		Name:    "histogram",

cmd/veneur-prometheus/translate.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"math"
 	"regexp"
+	"sort"
 
 	dto "github.com/prometheus/client_model/go"
 	"github.com/stripe/veneur/v14/sources/openmetrics"
@@ -201,8 +202,16 @@ func (t translator) Tags(labels []*dto.LabelPair) []string {
 		tags = append(tags, fmt.Sprintf("%s:%s", labelName, labelValue))
 	}
 
-	for name, value := range t.added {
-		tags = append(tags, fmt.Sprintf("%s:%s", name, value))
+	// Tags need to be returned in sorted order so that metric cache keys are consistent across metric observation
+	// sweeps
+	names := make([]string, 0, len(t.added))
+	for k := range t.added {
+		names = append(names, k)
+	}
+	sort.Strings(names)
+
+	for _, name := range names {
+		tags = append(tags, fmt.Sprintf("%s:%s", name, t.added[name]))
 	}
 
 	return tags

cmd/veneur-prometheus/translate_test.go

@@ -70,7 +70,7 @@ func TestAddTags(t *testing.T) {
 		"so:good",
 	}
 
-	assert.ElementsMatch(t, expectedTags, tags)
+	assert.Equal(t, expectedTags, tags)
 }
 
 func TestReplaceTags(t *testing.T) {