config: fix detailed metric keys missing in leading keys

only the last descriptor uses the provided descriptor value in case of
detailed metrics. when traversing the list of descriptors, the code
"loses" the previous keys. this leads to metrics like:
"test-domain.first-key_.second-key_second-value", where the last
descriptor properly uses the detailed metric descriptor value, but all
other descriptors (the first one here) are missing the value.

this patch introduces a new string builder, that builds the detailed
metric as the iteration of the input descriptor is happening.
a unit test is attached to show the behavior. it fails without the new
code, and successfully preserves all descriptor keys with the patched
code.

Signed-off-by: Johannes Brüderl <[email protected]>

src/config/config_impl.go

@@ -306,10 +306,19 @@ func (this *rateLimitConfigImpl) GetLimit(
 
 	descriptorsMap := value.descriptors
 	prevDescriptor := &value.rateLimitDescriptor
+
+	// Build detailed metric as we traverse the list of descriptors
+	var detailedMetricFullKey strings.Builder
+	detailedMetricFullKey.WriteString(domain)
+
 	for i, entry := range descriptor.Entries {
 		// First see if key_value is in the map. If that isn't in the map we look for just key
 		// to check for a default value.
 		finalKey := entry.Key + "_" + entry.Value
+
+		detailedMetricFullKey.WriteString(".")
+		detailedMetricFullKey.WriteString(finalKey)
+
 		logger.Debugf("looking up key: %s", finalKey)
 		nextDescriptor := descriptorsMap[finalKey]
 
@@ -343,7 +352,7 @@ func (this *rateLimitConfigImpl) GetLimit(
 			descriptorsMap = nextDescriptor.descriptors
 		} else {
 			if rateLimit != nil && rateLimit.DetailedMetric {
-				rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(rateLimit.FullKey+"_"+entry.Value), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, false)
+				rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(detailedMetricFullKey.String()), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, false)
 			}
 
 			break

src/config/config_test.go

@@ -0,0 +1,65 @@
+package config
+
+import (
+	"context"
+	"testing"
+
+	ratelimitv3 "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3"
+	pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3"
+	"github.com/envoyproxy/ratelimit/src/settings"
+	"github.com/envoyproxy/ratelimit/src/stats"
+	gostats "github.com/lyft/gostats"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDetailedMetric(t *testing.T) {
+	assert := require.New(t)
+
+	cfg := &rateLimitConfigImpl{
+		statsManager: stats.NewStatManager(gostats.NewDefaultStore(), settings.Settings{}),
+		domains: map[string]*rateLimitDomain{
+			"test-domain": &rateLimitDomain{
+				rateLimitDescriptor: rateLimitDescriptor{
+					descriptors: map[string]*rateLimitDescriptor{
+						"first-key": &rateLimitDescriptor{
+							descriptors: map[string]*rateLimitDescriptor{
+								"second-key": &rateLimitDescriptor{
+									limit: &RateLimit{
+										FullKey: "",
+										Stats:   stats.RateLimitStats{},
+										Limit: &pb.RateLimitResponse_RateLimit{
+											Name:            "",
+											RequestsPerUnit: 100,
+											Unit:            pb.RateLimitResponse_RateLimit_MINUTE,
+										},
+										Unlimited:      false,
+										ShadowMode:     false,
+										Name:           "",
+										Replaces:       []string{},
+										DetailedMetric: true,
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	rl := cfg.GetLimit(context.Background(), "test-domain", &ratelimitv3.RateLimitDescriptor{
+		Entries: []*ratelimitv3.RateLimitDescriptor_Entry{
+			{
+				Key:   "first-key",
+				Value: "first-value",
+			},
+			{
+				Key:   "second-key",
+				Value: "second-value",
+			},
+		},
+	})
+
+	assert.NotNil(rl)
+	assert.Equal("test-domain.first-key_first-value.second-key_second-value", rl.Stats.Key)
+}