[datadogexporter] Fix cache key generation for cumulative metrics (#5417

)

* [datadogexporter] Fix cache key generation for cumulative metrics

* Lint

* Split test

exporter/datadogexporter/internal/translator/ttlcache.go

@@ -22,6 +22,10 @@ import (
 	gocache "github.com/patrickmn/go-cache"
 )
 
+const (
+	metricKeySeparator = string(byte(0))
+)
+
 type TTLCache struct {
 	cache *gocache.Cache
 }
@@ -38,13 +42,29 @@ func NewTTLCache(sweepInterval int64, deltaTTL int64) *TTLCache {
 	return &TTLCache{cache}
 }
 
+// Uses a logic similar to what is done in the span processor to build metric keys:
+// https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/b2327211df976e0a57ef0425493448988772a16b/processor/spanmetricsprocessor/processor.go#L353-L387
+// TODO: make this a public util function?
+func concatDimensionValue(metricKeyBuilder *strings.Builder, value string) {
+	metricKeyBuilder.WriteString(value)
+	metricKeyBuilder.WriteString(metricKeySeparator)
+}
+
 // metricDimensionsToMapKey maps name and tags to a string to use as an identifier
 // The tags order does not matter
 func (*TTLCache) metricDimensionsToMapKey(name string, tags []string) string {
-	const separator string = "}{" // These are invalid in tags
-	dimensions := append(tags, name)
+	var metricKeyBuilder strings.Builder
+
+	dimensions := make([]string, len(tags))
+	copy(dimensions, tags)
+
+	dimensions = append(dimensions, name)
 	sort.Strings(dimensions)
-	return strings.Join(dimensions, separator)
+
+	for _, dim := range dimensions {
+		concatDimensionValue(&metricKeyBuilder, dim)
+	}
+	return metricKeyBuilder.String()
 }
 
 // putAndGetDiff submits a new value for a given metric and returns the difference with the

exporter/datadogexporter/internal/translator/ttlcache_test.go

@@ -53,3 +53,23 @@ func TestMetricDimensionsToMapKey(t *testing.T) {
 	assert.NotEqual(t, someTags, diffTags)
 	assert.Equal(t, someTags, sameTags)
 }
+
+func TestMetricDimensionsToMapKeyNoTagsChange(t *testing.T) {
+	// The original metricDimensionsToMapKey had an issue where:
+	// - if the capacity of the tags array passed to it was higher than its length
+	// - and the metric name is earlier (in alphabetical order) than one of the tags
+	// then the original tag array would be modified (without a reallocation, since there is enough capacity),
+	// and would contain a tag labeled as the metric name, while the final tag (in alphabetical order)
+	// would get left out.
+	// This test checks that this doesn't happen anymore.
+
+	metricName := "a.metric.name"
+	c := newTestCache()
+
+	originalTags := make([]string, 2, 3)
+	originalTags[0] = "key1:val1"
+	originalTags[1] = "key2:val2"
+	c.metricDimensionsToMapKey(metricName, originalTags)
+	assert.Equal(t, []string{"key1:val1", "key2:val2"}, originalTags)
+
+}