feat: sanitize "." in descriptor keys/values before publishing metrics

README.md

@@ -1022,6 +1022,13 @@ Configure statistics output frequency with `STATS_FLUSH_INTERVAL`, where the typ
 
 To disable statistics entirely, set env var `DISABLE_STATS` to `true`
 
+To sanitize `.` characters in descriptor keys and values before they are published as metrics, set env var
+`SANITIZE_DESCRIPTOR_METRIC_DOTS` to `true` (default `false`). When enabled, each `.` in a descriptor key or value is
+replaced with `_` in the emitted metric name. Because `.` is the statsd metric-hierarchy separator, a dotted value
+(e.g. a gRPC path like `/helloworld.Greeter/SayHello`) would otherwise inject unintended extra hierarchy levels —
+which breaks the Prometheus `statsd_exporter` metric-name-to-label mapping. This only affects metric names; rate limit
+matching (which keys off the raw descriptor) is unchanged. It is disabled by default for backward compatibility.
+
 Rate Limit Statistic Path:
 
 ```

src/config/config_impl.go

@@ -12,6 +12,7 @@ import (
 	"gopkg.in/yaml.v2"
 
 	"github.com/envoyproxy/ratelimit/src/stats"
+	"github.com/envoyproxy/ratelimit/src/utils"
 )
 
 type yamlReplaces struct {
@@ -443,6 +444,17 @@ func (this *rateLimitConfigImpl) Dump() string {
 	return ret
 }
 
+// maybeSanitize replaces '.' with '_' in a descriptor key or value fragment when the
+// SANITIZE_DESCRIPTOR_METRIC_DOTS setting is enabled, so dotted values don't inject extra
+// metric hierarchy levels. It must only be applied to fragments written into metric names,
+// never to keys used for descriptor map lookups (that would break rate limit matching).
+func (this *rateLimitConfigImpl) maybeSanitize(s string) string {
+	if this.statsManager.SanitizeDescriptorMetricDots() {
+		return utils.SanitizeStatKeyValue(s)
+	}
+	return s
+}
+
 func (this *rateLimitConfigImpl) GetLimit(
 	ctx context.Context, domain string, descriptor *pb_struct.RateLimitDescriptor,
 ) *RateLimit {
@@ -457,7 +469,7 @@ func (this *rateLimitConfigImpl) GetLimit(
 	}
 
 	if descriptor.GetLimit() != nil {
-		rateLimitKey := descriptorKey(domain, descriptor)
+		rateLimitKey := this.descriptorKey(domain, descriptor)
 		rateLimitOverrideUnit := pb.RateLimitResponse_RateLimit_Unit(descriptor.GetLimit().GetUnit())
 		// When limit override is provided by envoy config, we don't want to enable shadow_mode
 		rateLimit = NewRateLimit(
@@ -494,8 +506,12 @@ func (this *rateLimitConfigImpl) GetLimit(
 		// to check for a default value.
 		finalKey := entry.Key + "_" + entry.Value
 
+		// Write the sanitized fragments (never finalKey itself, which is reused for the
+		// descriptor map lookup below and must stay verbatim for rate limit matching).
 		detailedMetricFullKey.WriteString(".")
-		detailedMetricFullKey.WriteString(finalKey)
+		detailedMetricFullKey.WriteString(this.maybeSanitize(entry.Key))
+		detailedMetricFullKey.WriteString("_")
+		detailedMetricFullKey.WriteString(this.maybeSanitize(entry.Value))
 
 		logger.Debugf("looking up key: %s", finalKey)
 		nextDescriptor := descriptorsMap[finalKey]
@@ -561,14 +577,14 @@ func (this *rateLimitConfigImpl) GetLimit(
 			}
 
 			// Write key and value (if any)
-			valueToMetricFullKey.WriteString(entry.Key)
+			valueToMetricFullKey.WriteString(this.maybeSanitize(entry.Key))
 			if valueToUse != "" {
 				valueToMetricFullKey.WriteString("_")
-				valueToMetricFullKey.WriteString(valueToUse)
+				valueToMetricFullKey.WriteString(this.maybeSanitize(valueToUse))
 			}
 		} else {
 			// No next descriptor found; still append something deterministic
-			valueToMetricFullKey.WriteString(entry.Key)
+			valueToMetricFullKey.WriteString(this.maybeSanitize(entry.Key))
 		}
 
 		if nextDescriptor != nil && nextDescriptor.limit != nil {
@@ -635,15 +651,15 @@ func (this *rateLimitConfigImpl) GetLimit(
 			for i, entry := range descriptor.Entries {
 				shareThresholdMetricKey.WriteString(".")
 				if i < len(rateLimit.ShareThresholdKeyPattern) && rateLimit.ShareThresholdKeyPattern[i] != "" {
-					shareThresholdMetricKey.WriteString(entry.Key)
+					shareThresholdMetricKey.WriteString(this.maybeSanitize(entry.Key))
 					shareThresholdMetricKey.WriteString("_")
-					shareThresholdMetricKey.WriteString(rateLimit.ShareThresholdKeyPattern[i])
+					shareThresholdMetricKey.WriteString(this.maybeSanitize(rateLimit.ShareThresholdKeyPattern[i]))
 				} else {
 					// Include full key_value for entries without share_threshold
-					shareThresholdMetricKey.WriteString(entry.Key)
+					shareThresholdMetricKey.WriteString(this.maybeSanitize(entry.Key))
 					if entry.Value != "" {
 						shareThresholdMetricKey.WriteString("_")
-						shareThresholdMetricKey.WriteString(entry.Value)
+						shareThresholdMetricKey.WriteString(this.maybeSanitize(entry.Value))
 					}
 				}
 			}
@@ -676,15 +692,15 @@ func (this *rateLimitConfigImpl) IsEmptyDomains() bool {
 	return len(this.domains) == 0
 }
 
-func descriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string {
+func (this *rateLimitConfigImpl) descriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string {
 	rateLimitKey := ""
 	for _, entry := range descriptor.Entries {
 		if rateLimitKey != "" {
 			rateLimitKey += "."
 		}
-		rateLimitKey += entry.Key
+		rateLimitKey += this.maybeSanitize(entry.Key)
 		if entry.Value != "" {
-			rateLimitKey += "_" + entry.Value
+			rateLimitKey += "_" + this.maybeSanitize(entry.Value)
 		}
 	}
 	return domain + "." + rateLimitKey

src/settings/settings.go

@@ -90,6 +90,7 @@ type Settings struct {
 	ExtraTags                            map[string]string `envconfig:"EXTRA_TAGS" default:""`
 	StatsFlushInterval                   time.Duration     `envconfig:"STATS_FLUSH_INTERVAL" default:"10s"`
 	DisableStats                         bool              `envconfig:"DISABLE_STATS" default:"false"`
+	SanitizeDescriptorMetricDots         bool              `envconfig:"SANITIZE_DESCRIPTOR_METRIC_DOTS" default:"false"`
 	UsePrometheus                        bool              `envconfig:"USE_PROMETHEUS" default:"false"`
 	PrometheusAddr                       string            `envconfig:"PROMETHEUS_ADDR" default:":9090"`
 	PrometheusPath                       string            `envconfig:"PROMETHEUS_PATH" default:"/metrics"`

src/stats/manager.go

@@ -20,13 +20,17 @@ type Manager interface {
 	NewServiceStats() ServiceStats
 	// Returns the stats.Store wrapped by the Manager.
 	GetStatsStore() gostats.Store
+	// SanitizeDescriptorMetricDots reports whether '.' in descriptor keys/values should be
+	// replaced with '_' when building metric names (SANITIZE_DESCRIPTOR_METRIC_DOTS).
+	SanitizeDescriptorMetricDots() bool
 }
 
 type ManagerImpl struct {
-	store                gostats.Store
-	rlStatsScope         gostats.Scope
-	serviceStatsScope    gostats.Scope
-	shouldRateLimitScope gostats.Scope
+	store                        gostats.Store
+	rlStatsScope                 gostats.Scope
+	serviceStatsScope            gostats.Scope
+	shouldRateLimitScope         gostats.Scope
+	sanitizeDescriptorMetricDots bool
 }
 
 // Stats for panic recoveries.

src/stats/manager_impl.go

@@ -11,17 +11,22 @@ import (
 func NewStatManager(store gostats.Store, settings settings.Settings) *ManagerImpl {
 	serviceScope := store.ScopeWithTags("ratelimit", settings.ExtraTags).Scope("service")
 	return &ManagerImpl{
-		store:                store,
-		rlStatsScope:         serviceScope.Scope("rate_limit"),
-		serviceStatsScope:    serviceScope,
-		shouldRateLimitScope: serviceScope.Scope("call.should_rate_limit"),
+		store:                        store,
+		rlStatsScope:                 serviceScope.Scope("rate_limit"),
+		serviceStatsScope:            serviceScope,
+		shouldRateLimitScope:         serviceScope.Scope("call.should_rate_limit"),
+		sanitizeDescriptorMetricDots: settings.SanitizeDescriptorMetricDots,
 	}
 }
 
 func (this *ManagerImpl) GetStatsStore() gostats.Store {
 	return this.store
 }
 
+func (this *ManagerImpl) SanitizeDescriptorMetricDots() bool {
+	return this.sanitizeDescriptorMetricDots
+}
+
 // Create new rate descriptor stats for a descriptor tuple.
 // @param key supplies the fully resolved descriptor tuple.
 // @return new stats.

src/utils/utilities.go

@@ -87,6 +87,13 @@ func SanitizeStatName(s string) string {
 	})
 }
 
+// SanitizeStatKeyValue replaces the statsd hierarchy separator '.' with '_' so that
+// dots in a descriptor key or value do not create unintended metric hierarchy levels
+// (which, for example, break the Prometheus statsd_exporter metric-name -> label mapping).
+func SanitizeStatKeyValue(s string) string {
+	return strings.ReplaceAll(s, ".", "_")
+}
+
 type HitsAddend struct {
 	Value      uint64
 	IsNegative bool

test/config/config_test.go

@@ -2261,3 +2261,168 @@ func TestMetadata(t *testing.T) {
 
 // share_threshold parity (middle+trailing wildcards produce the same shared-counter
 // behaviour as trailing-only) is covered by TestShareThreshold Cases 5-7.
+
+// TestSanitizeDescriptorMetricDots verifies the SANITIZE_DESCRIPTOR_METRIC_DOTS behavior:
+// when enabled, '.' in descriptor keys/values is replaced with '_' in emitted metric names,
+// while rate-limit matching (which keys off the un-sanitized descriptor) is unaffected.
+func TestSanitizeDescriptorMetricDots(t *testing.T) {
+	// A config whose leaf key/value are looked up by the un-sanitized descriptor. The
+	// descriptor value carries dots (an IP and a dotted host) to exercise the metric path.
+	basicCfg := func() []config.RateLimitConfigToLoad {
+		return []config.RateLimitConfigToLoad{{
+			Name: "inline",
+			ConfigYaml: &config.YamlRoot{
+				Domain: "domain",
+				Descriptors: []config.YamlDescriptor{
+					{
+						Key:   "source_ip",
+						Value: "10.0.0.1",
+						RateLimit: &config.YamlRateLimit{
+							RequestsPerUnit: 5,
+							Unit:            "minute",
+						},
+					},
+				},
+			},
+		}}
+	}
+	ipDescriptor := &pb_struct.RateLimitDescriptor{
+		Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "source_ip", Value: "10.0.0.1"}},
+	}
+
+	t.Run("flag off keeps dots (backward compatible)", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		rlConfig := config.NewRateLimitConfigImpl(basicCfg(), mockstats.NewMockStatManagerWithSanitize(store, false), false)
+		rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor)
+		asrt.NotNil(rl)
+		asrt.Equal("domain.source_ip_10.0.0.1", rl.Stats.Key)
+	})
+
+	t.Run("flag on sanitizes dotted value", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		rlConfig := config.NewRateLimitConfigImpl(basicCfg(), mockstats.NewMockStatManagerWithSanitize(store, true), false)
+		rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor)
+		asrt.NotNil(rl)
+		expectedKey := "domain.source_ip_10_0_0_1"
+		asrt.Equal(expectedKey, rl.Stats.Key)
+
+		// The sanitized key is the one actually registered as a counter.
+		rl.Stats.TotalHits.Inc()
+		asrt.EqualValues(1, store.NewCounter(expectedKey+".total_hits").Value())
+	})
+
+	t.Run("flag on sanitizes non-ipv4 dotted value", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		cfg := []config.RateLimitConfigToLoad{{
+			Name: "inline",
+			ConfigYaml: &config.YamlRoot{
+				Domain: "domain",
+				Descriptors: []config.YamlDescriptor{
+					{
+						Key:   "host",
+						Value: "foo.bar",
+						RateLimit: &config.YamlRateLimit{
+							RequestsPerUnit: 5,
+							Unit:            "minute",
+						},
+					},
+				},
+			},
+		}}
+		rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false)
+		rl := rlConfig.GetLimit(context.TODO(), "domain",
+			&pb_struct.RateLimitDescriptor{Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "host", Value: "foo.bar"}}})
+		asrt.NotNil(rl)
+		asrt.Equal("domain.host_foo_bar", rl.Stats.Key)
+	})
+
+	t.Run("flag on sanitizes dotted key", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		cfg := []config.RateLimitConfigToLoad{{
+			Name: "inline",
+			ConfigYaml: &config.YamlRoot{
+				Domain: "domain",
+				Descriptors: []config.YamlDescriptor{
+					{
+						Key:   "a.b",
+						Value: "v",
+						RateLimit: &config.YamlRateLimit{
+							RequestsPerUnit: 5,
+							Unit:            "minute",
+						},
+					},
+				},
+			},
+		}}
+		rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false)
+		rl := rlConfig.GetLimit(context.TODO(), "domain",
+			&pb_struct.RateLimitDescriptor{Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "a.b", Value: "v"}}})
+		asrt.NotNil(rl)
+		asrt.Equal("domain.a_b_v", rl.Stats.Key)
+	})
+
+	t.Run("matching still works with dotted value when flag on", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		rlConfig := config.NewRateLimitConfigImpl(basicCfg(), mockstats.NewMockStatManagerWithSanitize(store, true), false)
+		// The descriptor still carries the un-sanitized dotted value; if sanitization had
+		// leaked into the map lookup key, this would fail to match and return nil.
+		rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor)
+		asrt.NotNil(rl)
+		asrt.EqualValues(5, rl.Limit.RequestsPerUnit)
+	})
+
+	t.Run("flag on sanitizes detailed_metric path", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		cfg := []config.RateLimitConfigToLoad{{
+			Name: "inline",
+			ConfigYaml: &config.YamlRoot{
+				Domain: "domain",
+				Descriptors: []config.YamlDescriptor{
+					{
+						Key:            "source_ip",
+						DetailedMetric: true,
+						RateLimit: &config.YamlRateLimit{
+							RequestsPerUnit: 5,
+							Unit:            "minute",
+						},
+					},
+				},
+			},
+		}}
+		rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false)
+		rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor)
+		asrt.NotNil(rl)
+		asrt.Equal("domain.source_ip_10_0_0_1", rl.Stats.Key)
+	})
+
+	t.Run("flag on sanitizes value_to_metric path", func(t *testing.T) {
+		asrt := assert.New(t)
+		store := stats.NewStore(stats.NewNullSink(), false)
+		cfg := []config.RateLimitConfigToLoad{{
+			Name: "inline",
+			ConfigYaml: &config.YamlRoot{
+				Domain: "domain",
+				Descriptors: []config.YamlDescriptor{
+					{
+						Key:           "source_ip",
+						ValueToMetric: true,
+						RateLimit: &config.YamlRateLimit{
+							RequestsPerUnit: 5,
+							Unit:            "minute",
+						},
+					},
+				},
+			},
+		}}
+		rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false)
+		rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor)
+		asrt.NotNil(rl)
+		asrt.Equal("domain.source_ip_10_0_0_1", rl.Stats.Key)
+	})
+}