metrics: copy tags list before serializing (#143)

Author: bmoylan

metrics/registry.go

@@ -283,7 +283,6 @@ func (r *rootRegistry) Each(f MetricVisitor) {
 			name = metricWithTags.name
 			tags = make(Tags, len(metricWithTags.tags))
 			copy(tags, metricWithTags.tags)
-			sort.Sort(tags)
 		} else {
 			// Metric was added to rcrowley registry outside of our registry.
 			// This is likely a go runtime metric (nothing else is exposed).
@@ -300,7 +299,7 @@ func (r *rootRegistry) Each(f MetricVisitor) {
 }
 
 func (r *rootRegistry) Unregister(name string, tags ...Tag) {
-	metricID := toMetricTagsID(name, tags)
+	metricID := toMetricTagsID(name, newSortedTags(tags))
 	r.registry.Unregister(string(metricID))
 }
 
@@ -341,11 +340,12 @@ func DefaultSample() metrics.Sample {
 }
 
 func (r *rootRegistry) registerMetric(name string, tags Tags) string {
-	metricID := toMetricTagsID(name, tags)
+	sortedTags := newSortedTags(tags)
+	metricID := toMetricTagsID(name, sortedTags)
 	r.idToMetricMutex.Lock()
 	r.idToMetricWithTags[metricID] = metricWithTags{
 		name: name,
-		tags: tags,
+		tags: sortedTags,
 	}
 	r.idToMetricMutex.Unlock()
 	return string(metricID)
@@ -364,10 +364,9 @@ type metricTagsID string
 
 // toMetricTagsID generates the metricTagsID identifier for the metricWithTags. A unique {metricName, set<Tag>} input will
 // generate a unique output. This implementation tries to minimize memory allocation and runtime.
+// The ID is created by adding the tags in the order they are provided (this means that, if the caller wants a specific set of tags to always
+// result in the same ID, they must sort the Tags before providing them to this function).
 func toMetricTagsID(name string, tags Tags) metricTagsID {
-	// TODO(maybe): Ensure tags is already sorted when it comes in so we can remove this.
-	sort.Sort(tags)
-
 	// calculate how large to make our byte buffer below
 	bufSize := len(name)
 	for _, t := range tags {
@@ -382,3 +381,10 @@ func toMetricTagsID(name string, tags Tags) metricTagsID {
 	}
 	return metricTagsID(buf.Bytes())
 }
+
+// newSortedTags copies the tag slice before sorting so that in-place mutation does not affect the input slice.
+func newSortedTags(tags Tags) Tags {
+	tagsCopy := append(tags[:0:0], tags...)
+	sort.Sort(tagsCopy)
+	return tagsCopy
+}

metrics/registry_test.go

@@ -73,6 +73,16 @@ func TestMetricsWithTags(t *testing.T) {
 	assert.Equal(t, wantTags, gotTags)
 }
 
+func TestMetricDoesNotMutateInputTagSlice(t *testing.T) {
+	root := metrics.NewRootMetricsRegistry()
+
+	unsortedTags := metrics.Tags{metrics.MustNewTag("b", "b"), metrics.MustNewTag("a", "a")}
+
+	root.Counter("my-counter", unsortedTags...).Inc(1)
+
+	assert.Equal(t, metrics.Tags{metrics.MustNewTag("b", "b"), metrics.MustNewTag("a", "a")}, unsortedTags)
+}
+
 // Prefix should be used as provided (no case conversion/normalization), while tags should always be converted to
 // lowercase.
 func TestMetricsCasing(t *testing.T) {