Fix shadow mode metric is not collected when local cache hit

src/limiter/base_limiter.go

@@ -80,9 +80,9 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo *
 			nil, 0)
 	}
 	var responseDescriptorStatus *pb.RateLimitResponse_DescriptorStatus
-	over_limit := false
+	isOverLimit := false
 	if isOverLimitWithLocalCache {
-		over_limit = true
+		isOverLimit = true
 		limitInfo.limit.Stats.OverLimit.Add(uint64(hitsAddend))
 		limitInfo.limit.Stats.OverLimitWithLocalCache.Add(uint64(hitsAddend))
 		responseDescriptorStatus = this.generateResponseDescriptorStatus(pb.RateLimitResponse_OVER_LIMIT,
@@ -94,7 +94,7 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo *
 		limitInfo.nearLimitThreshold = uint32(math.Floor(float64(float32(limitInfo.overLimitThreshold) * this.nearLimitRatio)))
 		logger.Debugf("cache key: %s current: %d", key, limitInfo.limitAfterIncrease)
 		if limitInfo.limitAfterIncrease > limitInfo.overLimitThreshold {
-			over_limit = true
+			isOverLimit = true
 			responseDescriptorStatus = this.generateResponseDescriptorStatus(pb.RateLimitResponse_OVER_LIMIT,
 				limitInfo.limit.Limit, 0)
 
@@ -124,11 +124,11 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo *
 	}
 
 	// If the limit is in ShadowMode, it should be always return OK
-	// We only want to increase stats if the limit was actually over the limit
-	if over_limit && limitInfo.limit.ShadowMode {
+	if isOverLimit && limitInfo.limit.ShadowMode {
 		logger.Debugf("Limit with key %s, is in shadow_mode", limitInfo.limit.FullKey)
 		responseDescriptorStatus.Code = pb.RateLimitResponse_OK
-		limitInfo.limit.Stats.ShadowMode.Add(uint64(hitsAddend))
+		// Increase shadow mode stats if the limit was actually over the limit
+		this.increaseShadowModeStats(isOverLimitWithLocalCache, limitInfo, hitsAddend)
 	}
 
 	return responseDescriptorStatus
@@ -178,6 +178,16 @@ func (this *BaseRateLimiter) checkNearLimitThreshold(limitInfo *LimitInfo, hitsA
 	}
 }
 
+func (this *BaseRateLimiter) increaseShadowModeStats(isOverLimitWithLocalCache bool, limitInfo *LimitInfo, hitsAddend uint32) {
+	// Increase shadow mode statistics. For the same reason as over limit stats,
+	// if the limit value before adding the N hits over the limit, then all N hits were over limit.
+	if isOverLimitWithLocalCache || limitInfo.limitBeforeIncrease >= limitInfo.overLimitThreshold {
+		limitInfo.limit.Stats.ShadowMode.Add(uint64(hitsAddend))
+	} else {
+		limitInfo.limit.Stats.ShadowMode.Add(uint64(limitInfo.limitAfterIncrease - limitInfo.overLimitThreshold))
+	}
+}
+
 func (this *BaseRateLimiter) generateResponseDescriptorStatus(responseCode pb.RateLimitResponse_Code,
 	limit *pb.RateLimitResponse_RateLimit, limitRemaining uint32) *pb.RateLimitResponse_DescriptorStatus {
 	if limit != nil {

src/redis/fixed_cache_impl.go

@@ -61,14 +61,12 @@ func (this *fixedRateLimitCacheImpl) DoLimit(
 
 		// Check if key is over the limit in local cache.
 		if this.baseRateLimiter.IsOverLimitWithLocalCache(cacheKey.Key) {
-
 			if limits[i].ShadowMode {
 				logger.Debugf("Cache key %s would be rate limited but shadow mode is enabled on this rule", cacheKey.Key)
 			} else {
 				logger.Debugf("cache key is over the limit: %s", cacheKey.Key)
-				isOverLimitWithLocalCache[i] = true
 			}
-
+			isOverLimitWithLocalCache[i] = true
 			continue
 		}
 

test/redis/fixed_cache_impl_test.go

@@ -564,6 +564,7 @@ func TestOverLimitWithLocalCacheShadowRule(t *testing.T) {
 	assert.Equal(uint64(3), limits[0].Stats.TotalHits.Value())
 	assert.Equal(uint64(1), limits[0].Stats.OverLimit.Value())
 	assert.Equal(uint64(0), limits[0].Stats.OverLimitWithLocalCache.Value())
+	assert.Equal(uint64(1), limits[0].Stats.ShadowMode.Value())
 	assert.Equal(uint64(1), limits[0].Stats.NearLimit.Value())
 	assert.Equal(uint64(2), limits[0].Stats.WithinLimit.Value())
 
@@ -579,15 +580,17 @@ func TestOverLimitWithLocalCacheShadowRule(t *testing.T) {
 	// The result should be OK since limit is in ShadowMode
 	assert.Equal(
 		[]*pb.RateLimitResponse_DescriptorStatus{
-			{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 15, DurationUntilReset: utils.CalculateReset(&limits[0].Limit.Unit, timeSource)},
+			{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(&limits[0].Limit.Unit, timeSource)},
 		},
 		cache.DoLimit(context.Background(), request, limits))
-	// TODO: How should we handle statistics? Should there be a separate ShadowMode statistics?  Should the other Stats remain as if they were unaffected by shadowmode?
+
+	// Even if you hit the local cache, other metrics should increase normally.
 	assert.Equal(uint64(4), limits[0].Stats.TotalHits.Value())
-	assert.Equal(uint64(1), limits[0].Stats.OverLimit.Value())
-	assert.Equal(uint64(0), limits[0].Stats.OverLimitWithLocalCache.Value())
+	assert.Equal(uint64(2), limits[0].Stats.OverLimit.Value())
+	assert.Equal(uint64(1), limits[0].Stats.OverLimitWithLocalCache.Value())
+	assert.Equal(uint64(2), limits[0].Stats.ShadowMode.Value())
 	assert.Equal(uint64(1), limits[0].Stats.NearLimit.Value())
-	assert.Equal(uint64(3), limits[0].Stats.WithinLimit.Value())
+	assert.Equal(uint64(2), limits[0].Stats.WithinLimit.Value())
 
 	// Check the local cache stats.
 	testLocalCacheStats(localCacheStats, statsStore, sink, 1, 3, 4, 0, 1)