stats: sanitize metric names (#481)

* stats: sanitize metric names If the configuration contains a key value with a `:` or `|`, e.g. ``` - key: path value: "/foo:*" rate_limit: unit: minute requests_per_unit: 20 ``` the reported statsd metrics are malformed. Signed-off-by: Lukasz Szczesny <[email protected]> * Add tests Signed-off-by: Lukasz Szczesny <[email protected]> --------- Signed-off-by: Lukasz Szczesny <[email protected]>
envoyproxy · Jan 15, 2024 · 763fd8e · 763fd8e
1 parent d87236e
commit 763fd8e
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 0 deletions.
diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go
@@ -5,6 +5,7 @@ import (
 	logger "github.com/sirupsen/logrus"
 
 	"github.com/envoyproxy/ratelimit/src/settings"
+	"github.com/envoyproxy/ratelimit/src/utils"
 )
 
 func NewStatManager(store gostats.Store, settings settings.Settings) *ManagerImpl {
@@ -28,6 +29,7 @@ func (this *ManagerImpl) NewStats(key string) RateLimitStats {
 	ret := RateLimitStats{}
 	logger.Debugf("Creating stats for key: '%s'", key)
 	ret.Key = key
+	key = utils.SanitizeStatName(key)
 	ret.TotalHits = this.rlStatsScope.NewCounter(key + ".total_hits")
 	ret.OverLimit = this.rlStatsScope.NewCounter(key + ".over_limit")
 	ret.NearLimit = this.rlStatsScope.NewCounter(key + ".near_limit")

diff --git a/src/utils/utilities.go b/src/utils/utilities.go
@@ -61,3 +61,9 @@ func MaskCredentialsInUrl(url string) string {
 
 	return strings.Join(urls, ",")
 }
+
+// Remove invalid characters from the stat name.
+func SanitizeStatName(s string) string {
+	r := strings.NewReplacer(":", "_", "|", "_")
+	return r.Replace(s)
+}
diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go
@@ -5,6 +5,7 @@ import (
 	logger "github.com/sirupsen/logrus"
 
 	"github.com/envoyproxy/ratelimit/src/stats"
+	"github.com/envoyproxy/ratelimit/src/utils"
 )
 
 type MockStatManager struct {
@@ -36,6 +37,7 @@ func (m *MockStatManager) NewStats(key string) stats.RateLimitStats {
 	ret := stats.RateLimitStats{}
 	logger.Debugf("outputing test gostats %s", key)
 	ret.Key = key
+	key = utils.SanitizeStatName(key)
 	ret.TotalHits = m.store.NewCounter(key + ".total_hits")
 	ret.OverLimit = m.store.NewCounter(key + ".over_limit")
 	ret.NearLimit = m.store.NewCounter(key + ".near_limit")

diff --git a/test/stats/manager_impl_test.go b/test/stats/manager_impl_test.go
@@ -0,0 +1,56 @@
+package test_stats
+
+import (
+	"fmt"
+	"testing"
+
+	gostats "github.com/lyft/gostats"
+	gostatsMock "github.com/lyft/gostats/mock"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/envoyproxy/ratelimit/src/settings"
+	"github.com/envoyproxy/ratelimit/src/stats"
+)
+
+func TestEscapingInvalidChartersInMetricName(t *testing.T) {
+	mockSink := gostatsMock.NewSink()
+	statsStore := gostats.NewStore(mockSink, false)
+	statsManager := stats.NewStatManager(statsStore, settings.Settings{})
+
+	tests := []struct {
+		name string
+		key  string
+		want string
+	}{
+		{
+			name: "use not modified key if it does not contain special characters",
+			key:  "path_/foo/bar",
+			want: "path_/foo/bar",
+		},
+		{
+			name: "escape colon",
+			key:  "path_/foo:*:bar",
+			want: "path_/foo_*_bar",
+		},
+		{
+			name: "escape pipe",
+			key:  "path_/foo|bar|baz",
+			want: "path_/foo_bar_baz",
+		},
+		{
+			name: "escape all special characters",
+			key:  "path_/foo:bar|baz",
+			want: "path_/foo_bar_baz",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			stats := statsManager.NewStats(tt.key)
+			assert.Equal(t, tt.key, stats.Key)
+
+			stats.TotalHits.Inc()
+			statsManager.GetStatsStore().Flush()
+			mockSink.AssertCounterExists(t, fmt.Sprintf("ratelimit.service.rate_limit.%s.total_hits", tt.want))
+		})
+	}
+}