From 21f6348159b56ecde5e7327c7cd2bb266e021d14 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Wed, 17 Mar 2021 16:40:09 -0300 Subject: [PATCH 01/18] Refactor metric handling Signed-off-by: Pablo Radnic --- src/config/config.go | 15 +-- src/config/config_impl.go | 68 ++++--------- src/config_check_cmd/main.go | 10 +- src/limiter/base_limiter.go | 35 ++++--- src/memcached/cache_impl.go | 11 +- src/redis/cache_impl.go | 4 +- src/redis/fixed_cache_impl.go | 5 +- src/server/server_impl.go | 19 ++-- src/service/ratelimit.go | 54 +++------- src/service/ratelimit_legacy.go | 25 +---- src/service_cmd/runner/runner.go | 33 +++--- src/stats/manager.go | 16 +++ src/stats/manager_impl.go | 140 ++++++++++++++++++++++++++ test/config/config_test.go | 102 ++++++++++--------- test/limiter/base_limiter_test.go | 37 ++++--- test/memcached/cache_impl_test.go | 53 +++++----- test/mocks/config/config.go | 4 +- test/mocks/stats/manager.go | 77 ++++++++++++++ test/redis/bench_test.go | 10 +- test/redis/fixed_cache_impl_test.go | 53 +++++----- test/service/ratelimit_legacy_test.go | 36 +++---- test/service/ratelimit_test.go | 38 +++---- 22 files changed, 518 insertions(+), 327 deletions(-) create mode 100644 src/stats/manager.go create mode 100644 src/stats/manager_impl.go create mode 100644 test/mocks/stats/manager.go diff --git a/src/config/config.go b/src/config/config.go index 83ed972b..49b5e280 100644 --- a/src/config/config.go +++ b/src/config/config.go @@ -3,7 +3,7 @@ package config import ( pb_struct "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3" pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3" - stats "github.com/lyft/gostats" + "github.com/envoyproxy/ratelimit/src/stats" "golang.org/x/net/context" ) @@ -14,19 +14,10 @@ func (e RateLimitConfigError) Error() string { return string(e) } -// Stats for an individual rate limit config entry. -type RateLimitStats struct { - TotalHits stats.Counter - OverLimit stats.Counter - NearLimit stats.Counter - OverLimitWithLocalCache stats.Counter - WithinLimit stats.Counter -} - // Wrapper for an individual rate limit config entry which includes the defined limit and stats. type RateLimit struct { FullKey string - Stats RateLimitStats + Stats stats.RateLimitStats Limit *pb.RateLimitResponse_RateLimit } @@ -56,5 +47,5 @@ type RateLimitConfigLoader interface { // @param statsScope supplies the stats scope to use for limit stats during runtime. // @return a new configuration. // @throws RateLimitConfigError if the configuration could not be created. - Load(configs []RateLimitConfigToLoad, statsScope stats.Scope) RateLimitConfig + Load(configs []RateLimitConfigToLoad, manager stats.Manager) RateLimitConfig } diff --git a/src/config/config_impl.go b/src/config/config_impl.go index b5118f5e..6f1c67ba 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -6,7 +6,7 @@ import ( pb_struct "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3" pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3" - stats "github.com/lyft/gostats" + "github.com/envoyproxy/ratelimit/src/stats" logger "github.com/sirupsen/logrus" "golang.org/x/net/context" "gopkg.in/yaml.v2" @@ -39,8 +39,8 @@ type rateLimitDomain struct { } type rateLimitConfigImpl struct { - domains map[string]*rateLimitDomain - statsScope stats.Scope + domains map[string]*rateLimitDomain + manager stats.Manager } var validKeys = map[string]bool{ @@ -53,30 +53,15 @@ var validKeys = map[string]bool{ "requests_per_unit": true, } -// Create new rate limit stats for a config entry. -// @param statsScope supplies the owning scope. -// @param key supplies the fully resolved key name of the entry. -// @return new stats. -func newRateLimitStats(statsScope stats.Scope, key string) RateLimitStats { - ret := RateLimitStats{} - ret.TotalHits = statsScope.NewCounter(key + ".total_hits") - ret.OverLimit = statsScope.NewCounter(key + ".over_limit") - ret.NearLimit = statsScope.NewCounter(key + ".near_limit") - ret.OverLimitWithLocalCache = statsScope.NewCounter(key + ".over_limit_with_local_cache") - ret.WithinLimit = statsScope.NewCounter(key + ".within_limit") - return ret -} - // Create a new rate limit config entry. // @param requestsPerUnit supplies the requests per unit of time for the entry. // @param unit supplies the unit of time for the entry. -// @param key supplies the fully resolved key name of the entry. -// @param scope supplies the owning scope. +// @param rlStats supplies the stats structure associated with the RateLimit // @return the new config entry. func NewRateLimit( - requestsPerUnit uint32, unit pb.RateLimitResponse_RateLimit_Unit, key string, scope stats.Scope) *RateLimit { + requestsPerUnit uint32, unit pb.RateLimitResponse_RateLimit_Unit, rlStats stats.RateLimitStats) *RateLimit { - return &RateLimit{FullKey: key, Stats: newRateLimitStats(scope, key), Limit: &pb.RateLimitResponse_RateLimit{RequestsPerUnit: requestsPerUnit, Unit: unit}} + return &RateLimit{FullKey: rlStats.String(), Stats: rlStats, Limit: &pb.RateLimitResponse_RateLimit{RequestsPerUnit: requestsPerUnit, Unit: unit}} } // Dump an individual descriptor for debugging purposes. @@ -104,10 +89,8 @@ func newRateLimitConfigError(config RateLimitConfigToLoad, err string) RateLimit // @param config supplies the config file that owns the descriptor. // @param parentKey supplies the fully resolved key name that owns this config level. // @param descriptors supplies the YAML descriptors to load. -// @param statsScope supplies the owning scope. -func (this *rateLimitDescriptor) loadDescriptors( - config RateLimitConfigToLoad, parentKey string, descriptors []yamlDescriptor, - statsScope stats.Scope) { +// @param manager that owns the stats.Scope. +func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, parentKey string, descriptors []yamlDescriptor, manager stats.Manager) { for _, descriptorConfig := range descriptors { if descriptorConfig.Key == "" { @@ -138,8 +121,7 @@ func (this *rateLimitDescriptor) loadDescriptors( } rateLimit = NewRateLimit( - descriptorConfig.RateLimit.RequestsPerUnit, pb.RateLimitResponse_RateLimit_Unit(value), newParentKey, - statsScope) + descriptorConfig.RateLimit.RequestsPerUnit, pb.RateLimitResponse_RateLimit_Unit(value), manager.NewStats(newParentKey)) rateLimitDebugString = fmt.Sprintf( " ratelimit={requests_per_unit=%d, unit=%s}", rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit.String()) @@ -148,8 +130,7 @@ func (this *rateLimitDescriptor) loadDescriptors( logger.Debugf( "loading descriptor: key=%s%s", newParentKey, rateLimitDebugString) newDescriptor := &rateLimitDescriptor{map[string]*rateLimitDescriptor{}, rateLimit} - newDescriptor.loadDescriptors( - config, newParentKey+".", descriptorConfig.Descriptors, statsScope) + newDescriptor.loadDescriptors(config, newParentKey+".", descriptorConfig.Descriptors, manager) this.descriptors[finalKey] = newDescriptor } } @@ -229,24 +210,10 @@ func (this *rateLimitConfigImpl) loadConfig(config RateLimitConfigToLoad) { logger.Debugf("loading domain: %s", root.Domain) newDomain := &rateLimitDomain{rateLimitDescriptor{map[string]*rateLimitDescriptor{}, nil}} - newDomain.loadDescriptors(config, root.Domain+".", root.Descriptors, this.statsScope) + newDomain.loadDescriptors(config, root.Domain+".", root.Descriptors, this.manager) this.domains[root.Domain] = newDomain } -func (this *rateLimitConfigImpl) descriptorToKey(descriptor *pb_struct.RateLimitDescriptor) string { - rateLimitKey := "" - for _, entry := range descriptor.Entries { - if rateLimitKey != "" { - rateLimitKey += "." - } - rateLimitKey += entry.Key - if entry.Value != "" { - rateLimitKey += "_" + entry.Value - } - } - return rateLimitKey -} - func (this *rateLimitConfigImpl) Dump() string { ret := "" for _, domain := range this.domains { @@ -268,13 +235,12 @@ func (this *rateLimitConfigImpl) GetLimit( } if descriptor.GetLimit() != nil { - rateLimitKey := domain + "." + this.descriptorToKey(descriptor) + rateLimitKey := stats.DescriptorKey(domain, descriptor) rateLimitOverrideUnit := pb.RateLimitResponse_RateLimit_Unit(descriptor.GetLimit().GetUnit()) rateLimit = NewRateLimit( descriptor.GetLimit().GetRequestsPerUnit(), rateLimitOverrideUnit, - rateLimitKey, - this.statsScope) + this.manager.NewStats(rateLimitKey)) return rateLimit } @@ -316,9 +282,9 @@ func (this *rateLimitConfigImpl) GetLimit( // @param stats supplies the stats scope to use for limit stats during runtime. // @return a new config. func NewRateLimitConfigImpl( - configs []RateLimitConfigToLoad, statsScope stats.Scope) RateLimitConfig { + configs []RateLimitConfigToLoad, manager stats.Manager) RateLimitConfig { - ret := &rateLimitConfigImpl{map[string]*rateLimitDomain{}, statsScope} + ret := &rateLimitConfigImpl{map[string]*rateLimitDomain{}, manager} for _, config := range configs { ret.loadConfig(config) } @@ -329,9 +295,9 @@ func NewRateLimitConfigImpl( type rateLimitConfigLoaderImpl struct{} func (this *rateLimitConfigLoaderImpl) Load( - configs []RateLimitConfigToLoad, statsScope stats.Scope) RateLimitConfig { + configs []RateLimitConfigToLoad, manager stats.Manager) RateLimitConfig { - return NewRateLimitConfigImpl(configs, statsScope) + return NewRateLimitConfigImpl(configs, manager) } // @return a new default config loader implementation. diff --git a/src/config_check_cmd/main.go b/src/config_check_cmd/main.go index f9f3c742..d5751cc7 100644 --- a/src/config_check_cmd/main.go +++ b/src/config_check_cmd/main.go @@ -3,12 +3,14 @@ package main import ( "flag" "fmt" + "github.com/envoyproxy/ratelimit/src/settings" + "github.com/envoyproxy/ratelimit/src/stats" "io/ioutil" "os" "path/filepath" "github.com/envoyproxy/ratelimit/src/config" - "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" ) func loadConfigs(allConfigs []config.RateLimitConfigToLoad) { @@ -19,9 +21,9 @@ func loadConfigs(allConfigs []config.RateLimitConfigToLoad) { os.Exit(1) } }() - - dummyStats := stats.NewStore(stats.NewNullSink(), false) - config.NewRateLimitConfigImpl(allConfigs, dummyStats) + settingStruct := settings.NewSettings() + manager := stats.NewStatManager(gostats.NewStore(gostats.NewNullSink(), false), settingStruct) + config.NewRateLimitConfigImpl(allConfigs, manager) } func main() { diff --git a/src/limiter/base_limiter.go b/src/limiter/base_limiter.go index 44c2633e..65f88646 100644 --- a/src/limiter/base_limiter.go +++ b/src/limiter/base_limiter.go @@ -5,6 +5,7 @@ import ( pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3" "github.com/envoyproxy/ratelimit/src/assert" "github.com/envoyproxy/ratelimit/src/config" + "github.com/envoyproxy/ratelimit/src/stats" "github.com/envoyproxy/ratelimit/src/utils" logger "github.com/sirupsen/logrus" "math" @@ -18,6 +19,7 @@ type BaseRateLimiter struct { cacheKeyGenerator CacheKeyGenerator localCache *freecache.Cache nearLimitRatio float32 + Manager stats.Manager } type LimitInfo struct { @@ -47,7 +49,7 @@ func (this *BaseRateLimiter) GenerateCacheKeys(request *pb.RateLimitRequest, cacheKeys[i] = this.cacheKeyGenerator.GenerateCacheKey(request.Domain, request.Descriptors[i], limits[i], now) // Increase statistics for limits hit by their respective requests. if limits[i] != nil { - limits[i].Stats.TotalHits.Add(uint64(hitsAddend)) + this.Manager.AddTotalHits(uint64(hitsAddend), limits[i].Stats) } } return cacheKeys @@ -74,8 +76,8 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo * nil, 0) } if isOverLimitWithLocalCache { - limitInfo.limit.Stats.OverLimit.Add(uint64(hitsAddend)) - limitInfo.limit.Stats.OverLimitWithLocalCache.Add(uint64(hitsAddend)) + this.Manager.AddOverLimit(uint64(hitsAddend), limitInfo.limit.Stats) + this.Manager.AddOverLimitWithLocalCache(uint64(hitsAddend), limitInfo.limit.Stats) return this.generateResponseDescriptorStatus(pb.RateLimitResponse_OVER_LIMIT, limitInfo.limit.Limit, 0) } @@ -89,7 +91,7 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo * responseDescriptorStatus = this.generateResponseDescriptorStatus(pb.RateLimitResponse_OVER_LIMIT, limitInfo.limit.Limit, 0) - checkOverLimitThreshold(limitInfo, hitsAddend) + this.checkOverLimitThreshold(limitInfo, hitsAddend) if this.localCache != nil { // Set the TTL of the local_cache to be the entire duration. @@ -109,14 +111,14 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo * limitInfo.limit.Limit, limitInfo.overLimitThreshold-limitInfo.limitAfterIncrease) // The limit is OK but we additionally want to know if we are near the limit. - checkNearLimitThreshold(limitInfo, hitsAddend) - limitInfo.limit.Stats.WithinLimit.Add(uint64(hitsAddend)) + this.checkNearLimitThreshold(limitInfo, hitsAddend) + this.Manager.AddWithinLimit(uint64(hitsAddend), limitInfo.limit.Stats) } return responseDescriptorStatus } func NewBaseRateLimit(timeSource utils.TimeSource, jitterRand *rand.Rand, expirationJitterMaxSeconds int64, - localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string) *BaseRateLimiter { + localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string, manager stats.Manager) *BaseRateLimiter { return &BaseRateLimiter{ timeSource: timeSource, JitterRand: jitterRand, @@ -124,37 +126,40 @@ func NewBaseRateLimit(timeSource utils.TimeSource, jitterRand *rand.Rand, expira cacheKeyGenerator: NewCacheKeyGenerator(cacheKeyPrefix), localCache: localCache, nearLimitRatio: nearLimitRatio, + Manager: manager, } } -func checkOverLimitThreshold(limitInfo *LimitInfo, hitsAddend uint32) { +func (this *BaseRateLimiter) checkOverLimitThreshold(limitInfo *LimitInfo, hitsAddend uint32) { // Increase over limit statistics. Because we support += behavior for increasing the limit, we need to // assess if the entire hitsAddend were over the limit. That is, if the limit's value before adding the // N hits was over the limit, then all the N hits were over limit. // Otherwise, only the difference between the current limit value and the over limit threshold // were over limit hits. if limitInfo.limitBeforeIncrease >= limitInfo.overLimitThreshold { - limitInfo.limit.Stats.OverLimit.Add(uint64(hitsAddend)) + this.Manager.AddOverLimit(uint64(hitsAddend), limitInfo.limit.Stats) } else { - limitInfo.limit.Stats.OverLimit.Add(uint64(limitInfo.limitAfterIncrease - limitInfo.overLimitThreshold)) + this.Manager.AddOverLimit(uint64(limitInfo.limitAfterIncrease-limitInfo.overLimitThreshold), limitInfo.limit.Stats) // If the limit before increase was below the over limit value, then some of the hits were // in the near limit range. - limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.overLimitThreshold - - utils.Max(limitInfo.nearLimitThreshold, limitInfo.limitBeforeIncrease))) + this.Manager.AddNearLimit( + uint64(limitInfo.overLimitThreshold-utils.Max(limitInfo.nearLimitThreshold, limitInfo.limitBeforeIncrease)), + limitInfo.limit.Stats, + ) } } -func checkNearLimitThreshold(limitInfo *LimitInfo, hitsAddend uint32) { +func (this *BaseRateLimiter) checkNearLimitThreshold(limitInfo *LimitInfo, hitsAddend uint32) { if limitInfo.limitAfterIncrease > limitInfo.nearLimitThreshold { // Here we also need to assess which portion of the hitsAddend were in the near limit range. // If all the hits were over the nearLimitThreshold, then all hits are near limit. Otherwise, // only the difference between the current limit value and the near limit threshold were near // limit hits. if limitInfo.limitBeforeIncrease >= limitInfo.nearLimitThreshold { - limitInfo.limit.Stats.NearLimit.Add(uint64(hitsAddend)) + this.Manager.AddNearLimit(uint64(hitsAddend), limitInfo.limit.Stats) } else { - limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.limitAfterIncrease - limitInfo.nearLimitThreshold)) + this.Manager.AddNearLimit(uint64(limitInfo.limitAfterIncrease-limitInfo.nearLimitThreshold), limitInfo.limit.Stats) } } } diff --git a/src/memcached/cache_impl.go b/src/memcached/cache_impl.go index 892565e8..78be2171 100644 --- a/src/memcached/cache_impl.go +++ b/src/memcached/cache_impl.go @@ -17,12 +17,13 @@ package memcached import ( "context" + "github.com/envoyproxy/ratelimit/src/stats" "math/rand" "strconv" "sync" "github.com/coocood/freecache" - stats "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" "github.com/bradfitz/gomemcache/memcache" @@ -174,7 +175,7 @@ func (this *rateLimitMemcacheImpl) Flush() { } func NewRateLimitCacheImpl(client Client, timeSource utils.TimeSource, jitterRand *rand.Rand, - expirationJitterMaxSeconds int64, localCache *freecache.Cache, scope stats.Scope, nearLimitRatio float32, cacheKeyPrefix string) limiter.RateLimitCache { + expirationJitterMaxSeconds int64, localCache *freecache.Cache, manager stats.Manager, nearLimitRatio float32, cacheKeyPrefix string) limiter.RateLimitCache { return &rateLimitMemcacheImpl{ client: client, timeSource: timeSource, @@ -182,19 +183,19 @@ func NewRateLimitCacheImpl(client Client, timeSource utils.TimeSource, jitterRan expirationJitterMaxSeconds: expirationJitterMaxSeconds, localCache: localCache, nearLimitRatio: nearLimitRatio, - baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix), + baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix, manager), } } func NewRateLimitCacheImplFromSettings(s settings.Settings, timeSource utils.TimeSource, jitterRand *rand.Rand, - localCache *freecache.Cache, scope stats.Scope) limiter.RateLimitCache { + localCache *freecache.Cache, scope gostats.Scope, manager stats.Manager) limiter.RateLimitCache { return NewRateLimitCacheImpl( CollectStats(memcache.New(s.MemcacheHostPort), scope.Scope("memcache")), timeSource, jitterRand, s.ExpirationJitterMaxSeconds, localCache, - scope, + manager, s.NearLimitRatio, s.CacheKeyPrefix, ) diff --git a/src/redis/cache_impl.go b/src/redis/cache_impl.go index 7e619b66..a6ddfe85 100644 --- a/src/redis/cache_impl.go +++ b/src/redis/cache_impl.go @@ -1,6 +1,7 @@ package redis import ( + "github.com/envoyproxy/ratelimit/src/stats" "math/rand" "github.com/coocood/freecache" @@ -10,7 +11,7 @@ import ( "github.com/envoyproxy/ratelimit/src/utils" ) -func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freecache.Cache, srv server.Server, timeSource utils.TimeSource, jitterRand *rand.Rand, expirationJitterMaxSeconds int64) limiter.RateLimitCache { +func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freecache.Cache, srv server.Server, timeSource utils.TimeSource, jitterRand *rand.Rand, expirationJitterMaxSeconds int64, manager stats.Manager) limiter.RateLimitCache { var perSecondPool Client if s.RedisPerSecond { perSecondPool = NewClientImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, @@ -29,5 +30,6 @@ func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freeca localCache, s.NearLimitRatio, s.CacheKeyPrefix, + manager, ) } diff --git a/src/redis/fixed_cache_impl.go b/src/redis/fixed_cache_impl.go index b2b3d3d2..2a0d5f7f 100644 --- a/src/redis/fixed_cache_impl.go +++ b/src/redis/fixed_cache_impl.go @@ -1,6 +1,7 @@ package redis import ( + "github.com/envoyproxy/ratelimit/src/stats" "math/rand" "github.com/coocood/freecache" @@ -107,10 +108,10 @@ func (this *fixedRateLimitCacheImpl) DoLimit( func (this *fixedRateLimitCacheImpl) Flush() {} func NewFixedRateLimitCacheImpl(client Client, perSecondClient Client, timeSource utils.TimeSource, - jitterRand *rand.Rand, expirationJitterMaxSeconds int64, localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string) limiter.RateLimitCache { + jitterRand *rand.Rand, expirationJitterMaxSeconds int64, localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string, manager stats.Manager) limiter.RateLimitCache { return &fixedRateLimitCacheImpl{ client: client, perSecondClient: perSecondClient, - baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix), + baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix, manager), } } diff --git a/src/server/server_impl.go b/src/server/server_impl.go index b60d1e32..ba3abdfc 100644 --- a/src/server/server_impl.go +++ b/src/server/server_impl.go @@ -4,6 +4,7 @@ import ( "bytes" "expvar" "fmt" + "github.com/envoyproxy/ratelimit/src/stats" "io" "net/http" "net/http/pprof" @@ -25,7 +26,7 @@ import ( "github.com/gorilla/mux" reuseport "github.com/kavu/go_reuseport" "github.com/lyft/goruntime/loader" - stats "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" logger "github.com/sirupsen/logrus" "google.golang.org/grpc" "google.golang.org/grpc/health" @@ -44,8 +45,8 @@ type server struct { debugPort int router *mux.Router grpcServer *grpc.Server - store stats.Store - scope stats.Scope + store gostats.Store + scope gostats.Scope runtime loader.IFace debugListener serverDebugListener httpServer *http.Server @@ -160,7 +161,7 @@ func (server *server) startGrpc() { server.grpcServer.Serve(lis) } -func (server *server) Scope() stats.Scope { +func (server *server) Scope() gostats.Scope { return server.scope } @@ -168,11 +169,11 @@ func (server *server) Runtime() loader.IFace { return server.runtime } -func NewServer(s settings.Settings, name string, store stats.Store, localCache *freecache.Cache, opts ...settings.Option) Server { - return newServer(s, name, store, localCache, opts...) +func NewServer(s settings.Settings, name string, manager stats.Manager, localCache *freecache.Cache, opts ...settings.Option) Server { + return newServer(s, name, manager, localCache, opts...) } -func newServer(s settings.Settings, name string, store stats.Store, localCache *freecache.Cache, opts ...settings.Option) *server { +func newServer(s settings.Settings, name string, manager stats.Manager, localCache *freecache.Cache, opts ...settings.Option) *server { for _, opt := range opts { opt(&s) } @@ -186,9 +187,9 @@ func newServer(s settings.Settings, name string, store stats.Store, localCache * ret.debugPort = s.DebugPort // setup stats - ret.store = store + ret.store = manager.GetStatsStore() ret.scope = ret.store.ScopeWithTags(name, s.ExtraTags) - ret.store.AddStatGenerator(stats.NewRuntimeStats(ret.scope.Scope("go"))) + ret.store.AddStatGenerator(gostats.NewRuntimeStats(ret.scope.Scope("go"))) if localCache != nil { ret.store.AddStatGenerator(limiter.NewLocalCacheStats(localCache, ret.scope.Scope("localcache"))) } diff --git a/src/service/ratelimit.go b/src/service/ratelimit.go index 126bb776..00d421af 100644 --- a/src/service/ratelimit.go +++ b/src/service/ratelimit.go @@ -2,6 +2,7 @@ package ratelimit import ( "fmt" + "github.com/envoyproxy/ratelimit/src/stats" "strings" "sync" @@ -11,37 +12,10 @@ import ( "github.com/envoyproxy/ratelimit/src/limiter" "github.com/envoyproxy/ratelimit/src/redis" "github.com/lyft/goruntime/loader" - stats "github.com/lyft/gostats" logger "github.com/sirupsen/logrus" "golang.org/x/net/context" ) -type shouldRateLimitStats struct { - redisError stats.Counter - serviceError stats.Counter -} - -func newShouldRateLimitStats(scope stats.Scope) shouldRateLimitStats { - ret := shouldRateLimitStats{} - ret.redisError = scope.NewCounter("redis_error") - ret.serviceError = scope.NewCounter("service_error") - return ret -} - -type serviceStats struct { - configLoadSuccess stats.Counter - configLoadError stats.Counter - shouldRateLimit shouldRateLimitStats -} - -func newServiceStats(scope stats.Scope) serviceStats { - ret := serviceStats{} - ret.configLoadSuccess = scope.NewCounter("config_load_success") - ret.configLoadError = scope.NewCounter("config_load_error") - ret.shouldRateLimit = newShouldRateLimitStats(scope.Scope("call.should_rate_limit")) - return ret -} - type RateLimitServiceServer interface { pb.RateLimitServiceServer GetCurrentConfig() config.RateLimitConfig @@ -55,13 +29,12 @@ type service struct { config config.RateLimitConfig runtimeUpdateEvent chan int cache limiter.RateLimitCache - stats serviceStats - rlStatsScope stats.Scope + stats stats.ServiceStats legacy *legacyService runtimeWatchRoot bool } -func (this *service) reloadConfig() { +func (this *service) reloadConfig(manager stats.Manager) { defer func() { if e := recover(); e != nil { configError, ok := e.(config.RateLimitConfigError) @@ -69,7 +42,7 @@ func (this *service) reloadConfig() { panic(e) } - this.stats.configLoadError.Inc() + this.stats.ConfigLoadError.Inc() logger.Errorf("error loading new configuration from runtime: %s", configError.Error()) } }() @@ -84,8 +57,8 @@ func (this *service) reloadConfig() { files = append(files, config.RateLimitConfigToLoad{key, snapshot.Get(key)}) } - newConfig := this.configLoader.Load(files, this.rlStatsScope) - this.stats.configLoadSuccess.Inc() + newConfig := this.configLoader.Load(files, manager) + this.stats.ConfigLoadSuccess.Inc() this.configLock.Lock() this.config = newConfig this.configLock.Unlock() @@ -170,12 +143,12 @@ func (this *service) ShouldRateLimit( switch t := err.(type) { case redis.RedisError: { - this.stats.shouldRateLimit.redisError.Inc() + this.stats.ShouldRateLimit.RedisError.Inc() finalError = t } case serviceError: { - this.stats.shouldRateLimit.serviceError.Inc() + this.stats.ShouldRateLimit.ServiceError.Inc() finalError = t } default: @@ -199,7 +172,7 @@ func (this *service) GetCurrentConfig() config.RateLimitConfig { } func NewService(runtime loader.IFace, cache limiter.RateLimitCache, - configLoader config.RateLimitConfigLoader, stats stats.Scope, runtimeWatchRoot bool) RateLimitServiceServer { + configLoader config.RateLimitConfigLoader, manager stats.Manager, runtimeWatchRoot bool) RateLimitServiceServer { newService := &service{ runtime: runtime, @@ -208,25 +181,24 @@ func NewService(runtime loader.IFace, cache limiter.RateLimitCache, config: nil, runtimeUpdateEvent: make(chan int), cache: cache, - stats: newServiceStats(stats), - rlStatsScope: stats.Scope("rate_limit"), + stats: manager.NewServiceStats(), runtimeWatchRoot: runtimeWatchRoot, } newService.legacy = &legacyService{ s: newService, - shouldRateLimitLegacyStats: newShouldRateLimitLegacyStats(stats), + shouldRateLimitLegacyStats: manager.NewShouldRateLimitLegacyStats(), } runtime.AddUpdateCallback(newService.runtimeUpdateEvent) - newService.reloadConfig() + newService.reloadConfig(manager) go func() { // No exit right now. for { logger.Debugf("waiting for runtime update") <-newService.runtimeUpdateEvent logger.Debugf("got runtime update and reloading config") - newService.reloadConfig() + newService.reloadConfig(manager) } }() diff --git a/src/service/ratelimit_legacy.go b/src/service/ratelimit_legacy.go index 17112675..ac3971e0 100644 --- a/src/service/ratelimit_legacy.go +++ b/src/service/ratelimit_legacy.go @@ -5,7 +5,7 @@ import ( pb_struct "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3" pb_legacy "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2" pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3" - "github.com/lyft/gostats" + "github.com/envoyproxy/ratelimit/src/stats" "golang.org/x/net/context" ) @@ -17,22 +17,7 @@ type RateLimitLegacyServiceServer interface { // the legacyService receives RateLimitRequests, converts the request, and calls the service's ShouldRateLimit method. type legacyService struct { s *service - shouldRateLimitLegacyStats shouldRateLimitLegacyStats -} - -type shouldRateLimitLegacyStats struct { - reqConversionError stats.Counter - respConversionError stats.Counter - shouldRateLimitError stats.Counter -} - -func newShouldRateLimitLegacyStats(scope stats.Scope) shouldRateLimitLegacyStats { - s := scope.Scope("call.should_rate_limit_legacy") - return shouldRateLimitLegacyStats{ - reqConversionError: s.NewCounter("req_conversion_error"), - respConversionError: s.NewCounter("resp_conversion_error"), - shouldRateLimitError: s.NewCounter("should_rate_limit_error"), - } + shouldRateLimitLegacyStats stats.ShouldRateLimitLegacyStats } func (this *legacyService) ShouldRateLimit( @@ -41,18 +26,18 @@ func (this *legacyService) ShouldRateLimit( request, err := ConvertLegacyRequest(legacyRequest) if err != nil { - this.shouldRateLimitLegacyStats.reqConversionError.Inc() + this.shouldRateLimitLegacyStats.ReqConversionError.Inc() return nil, err } resp, err := this.s.ShouldRateLimit(ctx, request) if err != nil { - this.shouldRateLimitLegacyStats.shouldRateLimitError.Inc() + this.shouldRateLimitLegacyStats.ShouldRateLimitError.Inc() return nil, err } legacyResponse, err := ConvertResponse(resp) if err != nil { - this.shouldRateLimitLegacyStats.respConversionError.Inc() + this.shouldRateLimitLegacyStats.RespConversionError.Inc() return nil, err } diff --git a/src/service_cmd/runner/runner.go b/src/service_cmd/runner/runner.go index afa1b144..e45ec392 100644 --- a/src/service_cmd/runner/runner.go +++ b/src/service_cmd/runner/runner.go @@ -1,6 +1,7 @@ package runner import ( + "github.com/envoyproxy/ratelimit/src/stats" "io" "math/rand" "net/http" @@ -8,7 +9,7 @@ import ( "sync" "time" - stats "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" "github.com/coocood/freecache" @@ -27,24 +28,24 @@ import ( ) type Runner struct { - statsStore stats.Store - settings settings.Settings - srv server.Server - mu sync.Mutex + manager stats.Manager + settings settings.Settings + srv server.Server + mu sync.Mutex } func NewRunner(s settings.Settings) Runner { return Runner{ - statsStore: stats.NewDefaultStore(), - settings: s, + manager: stats.NewStatManager(gostats.NewDefaultStore(), s), + settings: s, } } -func (runner *Runner) GetStatsStore() stats.Store { - return runner.statsStore +func (runner *Runner) GetStatsStore() gostats.Store { + return runner.manager.GetStatsStore() } -func createLimiter(srv server.Server, s settings.Settings, localCache *freecache.Cache) limiter.RateLimitCache { +func createLimiter(srv server.Server, s settings.Settings, localCache *freecache.Cache, manager stats.Manager) limiter.RateLimitCache { switch s.BackendType { case "redis", "": return redis.NewRateLimiterCacheImplFromSettings( @@ -53,14 +54,16 @@ func createLimiter(srv server.Server, s settings.Settings, localCache *freecache srv, utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), - s.ExpirationJitterMaxSeconds) + s.ExpirationJitterMaxSeconds, + manager) case "memcache": return memcached.NewRateLimitCacheImplFromSettings( s, utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), localCache, - srv.Scope()) + srv.Scope(), + manager) default: logger.Fatalf("Invalid setting for BackendType: %s", s.BackendType) panic("This line should not be reachable") @@ -91,16 +94,16 @@ func (runner *Runner) Run() { localCache = freecache.NewCache(s.LocalCacheSizeInBytes) } - srv := server.NewServer(s, "ratelimit", runner.statsStore, localCache, settings.GrpcUnaryInterceptor(nil)) + srv := server.NewServer(s, "ratelimit", runner.manager, localCache, settings.GrpcUnaryInterceptor(nil)) runner.mu.Lock() runner.srv = srv runner.mu.Unlock() service := ratelimit.NewService( srv.Runtime(), - createLimiter(srv, s, localCache), + createLimiter(srv, s, localCache, runner.manager), config.NewRateLimitConfigLoaderImpl(), - srv.Scope().Scope("service"), + runner.manager, s.RuntimeWatchRoot, ) diff --git a/src/stats/manager.go b/src/stats/manager.go new file mode 100644 index 00000000..eb902b97 --- /dev/null +++ b/src/stats/manager.go @@ -0,0 +1,16 @@ +package stats + +import stats "github.com/lyft/gostats" + +type Manager interface { + AddTotalHits(u uint64, rlStats RateLimitStats) + AddOverLimit(u uint64, rlStats RateLimitStats) + AddNearLimit(u uint64, rlStats RateLimitStats) + AddOverLimitWithLocalCache(u uint64, rlStats RateLimitStats) + AddWithinLimit(u uint64, rlStats RateLimitStats) + NewStats(key string) RateLimitStats + NewShouldRateLimitStats() ShouldRateLimitStats + NewServiceStats() ServiceStats + NewShouldRateLimitLegacyStats() ShouldRateLimitLegacyStats + GetStatsStore() stats.Store +} diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go new file mode 100644 index 00000000..6d9ff5b3 --- /dev/null +++ b/src/stats/manager_impl.go @@ -0,0 +1,140 @@ +package stats + +import ( + pb_struct "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3" + "github.com/envoyproxy/ratelimit/src/settings" + gostats "github.com/lyft/gostats" + logger "github.com/sirupsen/logrus" +) + +func NewStatManager(store gostats.Store, s settings.Settings) *ManagerImpl { + serviceScope := store.ScopeWithTags("ratelimit", s.ExtraTags).Scope("service") + return &ManagerImpl{ + store: store, + rlStatsScope: serviceScope.Scope("rate_limit"), + legacyStatsScope: serviceScope.Scope("call.should_rate_limit_legacy"), + serviceStatsScope: serviceScope, + detailedMetricsScope: serviceScope.Scope("rate_limit").Scope("detailed"), + } +} + +type ManagerImpl struct { + store gostats.Store + rlStatsScope gostats.Scope + legacyStatsScope gostats.Scope + serviceStatsScope gostats.Scope + detailedMetricsScope gostats.Scope + detailed bool +} + +func (this *ManagerImpl) GetStatsStore() gostats.Store { + return this.store +} + +func (this *ManagerImpl) AddTotalHits(u uint64, rlStats RateLimitStats) { + rlStats.TotalHits.Add(u) +} + +func (this *ManagerImpl) AddOverLimit(u uint64, rlStats RateLimitStats) { + rlStats.OverLimit.Add(u) +} + +func (this *ManagerImpl) AddNearLimit(u uint64, rlStats RateLimitStats) { + rlStats.NearLimit.Add(u) +} + +func (this *ManagerImpl) AddOverLimitWithLocalCache(u uint64, rlStats RateLimitStats) { + rlStats.OverLimitWithLocalCache.Add(u) +} + +func (this *ManagerImpl) AddWithinLimit(u uint64, rlStats RateLimitStats) { + rlStats.WithinLimit.Add(u) +} + +// Create new rate descriptor stats for a descriptor tuple. +// @param key supplies the fully resolved descriptor tuple. +// @return new stats. +func (this *ManagerImpl) NewStats(key string) RateLimitStats { + ret := RateLimitStats{} + logger.Debugf("Creating stats for key: '%s'", key) + ret.Key = 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") + ret.OverLimitWithLocalCache = this.rlStatsScope.NewCounter(key + ".over_limit_with_local_cache") + ret.WithinLimit = this.rlStatsScope.NewCounter(key + ".within_limit") + return ret +} + + +type ShouldRateLimitLegacyStats struct { + ReqConversionError gostats.Counter + RespConversionError gostats.Counter + ShouldRateLimitError gostats.Counter +} + +func (this *ManagerImpl) NewShouldRateLimitLegacyStats() ShouldRateLimitLegacyStats { + return ShouldRateLimitLegacyStats{ + ReqConversionError: this.legacyStatsScope.NewCounter("req_conversion_error"), + RespConversionError: this.legacyStatsScope.NewCounter("resp_conversion_error"), + ShouldRateLimitError: this.legacyStatsScope.NewCounter("should_rate_limit_error"), + } +} + +type ShouldRateLimitStats struct { + RedisError gostats.Counter + ServiceError gostats.Counter +} + +func (this *ManagerImpl) NewShouldRateLimitStats() ShouldRateLimitStats { + s := this.serviceStatsScope.Scope("call.should_rate_limit") + ret := ShouldRateLimitStats{} + ret.RedisError = s.NewCounter("redis_error") + ret.ServiceError = s.NewCounter("service_error") + return ret +} + +type ServiceStats struct { + ConfigLoadSuccess gostats.Counter + ConfigLoadError gostats.Counter + ShouldRateLimit ShouldRateLimitStats +} + +func (this *ManagerImpl) NewServiceStats() ServiceStats { + ret := ServiceStats{} + ret.ConfigLoadSuccess = this.serviceStatsScope.NewCounter("config_load_success") + ret.ConfigLoadError = this.serviceStatsScope.NewCounter("config_load_error") + ret.ShouldRateLimit = this.NewShouldRateLimitStats() + return ret +} + +func DescriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string { + rateLimitKey := "" + for _, entry := range descriptor.Entries { + if rateLimitKey != "" { + rateLimitKey += "." + } + rateLimitKey += entry.Key + if entry.Value != "" { + rateLimitKey += "_" + entry.Value + } + } + return domain + "." + rateLimitKey +} + +// Stats for an individual rate limit config entry. +//todo: Ideally the gostats package fields should be unexported +// the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 +// This ensures that setters such as Inc() and Add() can only be managed by ManagerImpl. +type RateLimitStats struct { + Key string + TotalHits gostats.Counter + OverLimit gostats.Counter + NearLimit gostats.Counter + OverLimitWithLocalCache gostats.Counter + WithinLimit gostats.Counter +} + +func (this RateLimitStats) String() string { + return this.Key +} diff --git a/test/config/config_test.go b/test/config/config_test.go index 4a244bce..e0f2a2e0 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -9,6 +9,7 @@ import ( pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3" pb_type "github.com/envoyproxy/go-control-plane/envoy/type/v3" "github.com/envoyproxy/ratelimit/src/config" + mockstats "github.com/envoyproxy/ratelimit/test/mocks/stats" "github.com/lyft/gostats" "github.com/stretchr/testify/assert" ) @@ -23,8 +24,8 @@ func loadFile(path string) []config.RateLimitConfigToLoad { func TestBasicConfig(t *testing.T) { assert := assert.New(t) - stats := stats.NewStore(stats.NewNullSink(), false) - rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), stats) + newStore := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), mockstats.NewMockStatManager(newStore)) rlConfig.Dump() assert.Nil(rlConfig.GetLimit(nil, "foo_domain", &pb_struct.RateLimitDescriptor{})) assert.Nil(rlConfig.GetLimit(nil, "test-domain", &pb_struct.RateLimitDescriptor{})) @@ -68,10 +69,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(5, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_SECOND, rl.Limit.Unit) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -85,13 +86,13 @@ func TestBasicConfig(t *testing.T) { assert.EqualValues(10, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_SECOND, rl.Limit.Unit) assert.EqualValues( - 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.total_hits").Value()) + 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.total_hits").Value()) assert.EqualValues( - 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.over_limit").Value()) + 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.over_limit").Value()) assert.EqualValues( - 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.near_limit").Value()) + 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.near_limit").Value()) assert.EqualValues( - 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.within_limit").Value()) + 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -104,10 +105,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(20, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_MINUTE, rl.Limit.Unit) - assert.EqualValues(1, stats.NewCounter("test-domain.key2.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key2.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key2.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key2.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -120,10 +121,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(30, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_MINUTE, rl.Limit.Unit) - assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -143,10 +144,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(1, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_HOUR, rl.Limit.Unit) - assert.EqualValues(1, stats.NewCounter("test-domain.key3.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key3.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key3.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key3.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key3.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key3.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key3.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key3.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -159,16 +160,16 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(1, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_DAY, rl.Limit.Unit) - assert.EqualValues(1, stats.NewCounter("test-domain.key4.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key4.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key4.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key4.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key4.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key4.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key4.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key4.within_limit").Value()) } func TestConfigLimitOverride(t *testing.T) { assert := assert.New(t) - stats := stats.NewStore(stats.NewNullSink(), false) - rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), stats) + newStore := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), mockstats.NewMockStatManager(newStore)) rlConfig.Dump() // No matching domain assert.Nil(rlConfig.GetLimit(nil, "foo_domain", &pb_struct.RateLimitDescriptor{ @@ -193,10 +194,10 @@ func TestConfigLimitOverride(t *testing.T) { rl.Stats.OverLimit.Inc() rl.Stats.NearLimit.Inc() rl.Stats.WithinLimit.Inc() - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) // Change in override value doesn't erase stats rl = rlConfig.GetLimit( @@ -216,10 +217,10 @@ func TestConfigLimitOverride(t *testing.T) { RequestsPerUnit: 42, Unit: pb.RateLimitResponse_RateLimit_HOUR, }, rl.Limit) - assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) - assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) - assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) - assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) + assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) + assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) + assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) + assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) // Different value creates a different counter rl = rlConfig.GetLimit( @@ -239,10 +240,10 @@ func TestConfigLimitOverride(t *testing.T) { rl.Stats.OverLimit.Inc() rl.Stats.NearLimit.Inc() rl.Stats.WithinLimit.Inc() - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.total_hits").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.over_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.near_limit").Value()) - assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.within_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.total_hits").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.over_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.near_limit").Value()) + assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.within_limit").Value()) } func expectConfigPanic(t *testing.T, call func(), expectedError string) { @@ -261,7 +262,7 @@ func TestEmptyDomain(t *testing.T) { t, func() { config.NewRateLimitConfigImpl( - loadFile("empty_domain.yaml"), stats.NewStore(stats.NewNullSink(), false)) + loadFile("empty_domain.yaml"), mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "empty_domain.yaml: config file cannot have empty domain") } @@ -272,7 +273,7 @@ func TestDuplicateDomain(t *testing.T) { func() { files := loadFile("basic_config.yaml") files = append(files, loadFile("duplicate_domain.yaml")...) - config.NewRateLimitConfigImpl(files, stats.NewStore(stats.NewNullSink(), false)) + config.NewRateLimitConfigImpl(files, mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "duplicate_domain.yaml: duplicate domain 'test-domain' in config file") } @@ -283,7 +284,7 @@ func TestEmptyKey(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("empty_key.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "empty_key.yaml: descriptor has empty key") } @@ -294,7 +295,7 @@ func TestDuplicateKey(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("duplicate_key.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "duplicate_key.yaml: duplicate descriptor composite key 'test-domain.key1_value1'") } @@ -305,7 +306,7 @@ func TestBadLimitUnit(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("bad_limit_unit.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "bad_limit_unit.yaml: invalid rate limit unit 'foo'") } @@ -316,7 +317,7 @@ func TestBadYaml(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("bad_yaml.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "bad_yaml.yaml: error loading config file: yaml: line 2: found unexpected end of stream") } @@ -327,7 +328,7 @@ func TestMisspelledKey(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("misspelled_key.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "misspelled_key.yaml: config error, unknown key 'ratelimit'") @@ -336,7 +337,8 @@ func TestMisspelledKey(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("misspelled_key2.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) + }, "misspelled_key2.yaml: config error, unknown key 'requestsperunit'") } @@ -347,7 +349,7 @@ func TestNonStringKey(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("non_string_key.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "non_string_key.yaml: config error, key is not of type string: 0.25") } @@ -358,7 +360,7 @@ func TestNonMapList(t *testing.T) { func() { config.NewRateLimitConfigImpl( loadFile("non_map_list.yaml"), - stats.NewStore(stats.NewNullSink(), false)) + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) }, "non_map_list.yaml: config error, yaml file contains list of type other than map: a") } diff --git a/test/limiter/base_limiter_test.go b/test/limiter/base_limiter_test.go index 41aa0e5b..e1b8167c 100644 --- a/test/limiter/base_limiter_test.go +++ b/test/limiter/base_limiter_test.go @@ -1,6 +1,7 @@ package limiter import ( + mockstats "github.com/envoyproxy/ratelimit/test/mocks/stats" "math/rand" "testing" @@ -22,10 +23,11 @@ func TestGenerateCacheKeys(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) jitterSource := mock_utils.NewMockJitterRandSource(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) + sm := mockstats.NewMockStatManager(statsStore) timeSource.EXPECT().UnixNow().Return(int64(1234)) - baseRateLimit := limiter.NewBaseRateLimit(timeSource, rand.New(jitterSource), 3600, nil, 0.8, "") + baseRateLimit := limiter.NewBaseRateLimit(timeSource, rand.New(jitterSource), 3600, nil, 0.8, "", sm) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal(uint64(0), limits[0].Stats.TotalHits.Value()) cacheKeys := baseRateLimit.GenerateCacheKeys(request, limits, 1) assert.Equal(1, len(cacheKeys)) @@ -40,10 +42,11 @@ func TestGenerateCacheKeysPrefix(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) jitterSource := mock_utils.NewMockJitterRandSource(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) + sm := mockstats.NewMockStatManager(statsStore) timeSource.EXPECT().UnixNow().Return(int64(1234)) - baseRateLimit := limiter.NewBaseRateLimit(timeSource, rand.New(jitterSource), 3600, nil, 0.8, "prefix:") + baseRateLimit := limiter.NewBaseRateLimit(timeSource, rand.New(jitterSource), 3600, nil, 0.8, "prefix:", sm) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal(uint64(0), limits[0].Stats.TotalHits.Value()) cacheKeys := baseRateLimit.GenerateCacheKeys(request, limits, 1) assert.Equal(1, len(cacheKeys)) @@ -57,7 +60,8 @@ func TestOverLimitWithLocalCache(t *testing.T) { defer controller.Finish() localCache := freecache.NewCache(100) localCache.Set([]byte("key"), []byte("value"), 100) - baseRateLimit := limiter.NewBaseRateLimit(nil, nil, 3600, localCache, 0.8, "") + sm := mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false)) + baseRateLimit := limiter.NewBaseRateLimit(nil, nil, 3600, localCache, 0.8, "", sm) // Returns true, as local cache contains over limit value for the key. assert.Equal(true, baseRateLimit.IsOverLimitWithLocalCache("key")) } @@ -66,11 +70,12 @@ func TestNoOverLimitWithLocalCache(t *testing.T) { assert := assert.New(t) controller := gomock.NewController(t) defer controller.Finish() - baseRateLimit := limiter.NewBaseRateLimit(nil, nil, 3600, nil, 0.8, "") + sm := mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false)) + baseRateLimit := limiter.NewBaseRateLimit(nil, nil, 3600, nil, 0.8, "", sm) // Returns false, as local cache is nil. assert.Equal(false, baseRateLimit.IsOverLimitWithLocalCache("domain_key_value_1234")) localCache := freecache.NewCache(100) - baseRateLimitWithLocalCache := limiter.NewBaseRateLimit(nil, nil, 3600, localCache, 0.8, "") + baseRateLimitWithLocalCache := limiter.NewBaseRateLimit(nil, nil, 3600, localCache, 0.8, "", sm) // Returns false, as local cache does not contain value for cache key. assert.Equal(false, baseRateLimitWithLocalCache.IsOverLimitWithLocalCache("domain_key_value_1234")) } @@ -79,7 +84,8 @@ func TestGetResponseStatusEmptyKey(t *testing.T) { assert := assert.New(t) controller := gomock.NewController(t) defer controller.Finish() - baseRateLimit := limiter.NewBaseRateLimit(nil, nil, 3600, nil, 0.8, "") + sm := mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false)) + baseRateLimit := limiter.NewBaseRateLimit(nil, nil, 3600, nil, 0.8, "", sm) responseStatus := baseRateLimit.GetResponseDescriptorStatus("", nil, false, 1) assert.Equal(pb.RateLimitResponse_OK, responseStatus.GetCode()) assert.Equal(uint32(0), responseStatus.GetLimitRemaining()) @@ -92,8 +98,9 @@ func TestGetResponseStatusOverLimitWithLocalCache(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) timeSource.EXPECT().UnixNow().Return(int64(1234)) statsStore := stats.NewStore(stats.NewNullSink(), false) - baseRateLimit := limiter.NewBaseRateLimit(timeSource, nil, 3600, nil, 0.8, "") - limits := []*config.RateLimit{config.NewRateLimit(5, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + sm := mockstats.NewMockStatManager(statsStore) + baseRateLimit := limiter.NewBaseRateLimit(timeSource, nil, 3600, nil, 0.8, "", sm) + limits := []*config.RateLimit{config.NewRateLimit(5, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} limitInfo := limiter.NewRateLimitInfo(limits[0], 2, 6, 4, 5) // As `isOverLimitWithLocalCache` is passed as `true`, immediate response is returned with no checks of the limits. responseStatus := baseRateLimit.GetResponseDescriptorStatus("key", limitInfo, true, 2) @@ -112,8 +119,9 @@ func TestGetResponseStatusOverLimit(t *testing.T) { timeSource.EXPECT().UnixNow().Return(int64(1234)) statsStore := stats.NewStore(stats.NewNullSink(), false) localCache := freecache.NewCache(100) - baseRateLimit := limiter.NewBaseRateLimit(timeSource, nil, 3600, localCache, 0.8, "") - limits := []*config.RateLimit{config.NewRateLimit(5, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + sm := mockstats.NewMockStatManager(statsStore) + baseRateLimit := limiter.NewBaseRateLimit(timeSource, nil, 3600, localCache, 0.8, "", sm) + limits := []*config.RateLimit{config.NewRateLimit(5, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} limitInfo := limiter.NewRateLimitInfo(limits[0], 2, 7, 4, 5) responseStatus := baseRateLimit.GetResponseDescriptorStatus("key", limitInfo, false, 1) assert.Equal(pb.RateLimitResponse_OVER_LIMIT, responseStatus.GetCode()) @@ -133,8 +141,9 @@ func TestGetResponseStatusBelowLimit(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) timeSource.EXPECT().UnixNow().Return(int64(1234)) statsStore := stats.NewStore(stats.NewNullSink(), false) - baseRateLimit := limiter.NewBaseRateLimit(timeSource, nil, 3600, nil, 0.8, "") - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + sm := mockstats.NewMockStatManager(statsStore) + baseRateLimit := limiter.NewBaseRateLimit(timeSource, nil, 3600, nil, 0.8, "", sm) + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} limitInfo := limiter.NewRateLimitInfo(limits[0], 2, 6, 9, 10) responseStatus := baseRateLimit.GetResponseDescriptorStatus("key", limitInfo, false, 1) assert.Equal(pb.RateLimitResponse_OK, responseStatus.GetCode()) diff --git a/test/memcached/cache_impl_test.go b/test/memcached/cache_impl_test.go index 1e2ba8d7..1358af44 100644 --- a/test/memcached/cache_impl_test.go +++ b/test/memcached/cache_impl_test.go @@ -5,6 +5,7 @@ package memcached_test import ( + mockstats "github.com/envoyproxy/ratelimit/test/mocks/stats" "math/rand" "strconv" "testing" @@ -34,7 +35,8 @@ func TestMemcached(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) client := mock_memcached.NewMockClient(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) - cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, statsStore, 0.8, "") + sm := mockstats.NewMockStatManager(statsStore) + cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, sm, 0.8, "") timeSource.EXPECT().UnixNow().Return(int64(1234)).MaxTimes(3) client.EXPECT().GetMulti([]string{"domain_key_value_1234"}).Return( @@ -43,7 +45,7 @@ func TestMemcached(t *testing.T) { client.EXPECT().Increment("domain_key_value_1234", uint64(1)).Return(uint64(5), nil) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 5, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -67,7 +69,7 @@ func TestMemcached(t *testing.T) { }, 1) limits = []*config.RateLimit{ nil, - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key2_value2_subkey2_subvalue2", statsStore)} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, sm.NewStats("key2_value2_subkey2_subvalue2"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: nil, LimitRemaining: 0}, {Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[1].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[1].Limit, timeSource)}}, @@ -97,8 +99,8 @@ func TestMemcached(t *testing.T) { {{"key3", "value3"}, {"subkey3", "subvalue3"}}, }, 1) limits = []*config.RateLimit{ - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_HOUR, "key3_value3", statsStore), - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_DAY, "key3_value3_subkey3_subvalue3", statsStore)} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_HOUR, sm.NewStats("key3_value3")), + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_DAY, sm.NewStats("key3_value3_subkey3_subvalue3"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{ {Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}, @@ -124,7 +126,8 @@ func TestMemcachedGetError(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) client := mock_memcached.NewMockClient(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) - cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, statsStore, 0.8, "") + sm := mockstats.NewMockStatManager(statsStore) + cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, sm, 0.8, "") timeSource.EXPECT().UnixNow().Return(int64(1234)).MaxTimes(3) client.EXPECT().GetMulti([]string{"domain_key_value_1234"}).Return( @@ -133,7 +136,7 @@ func TestMemcachedGetError(t *testing.T) { client.EXPECT().Increment("domain_key_value_1234", uint64(1)).Return(uint64(5), nil) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 9, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -151,7 +154,7 @@ func TestMemcachedGetError(t *testing.T) { client.EXPECT().Increment("domain_key_value1_1234", uint64(1)).Return(uint64(5), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value1"}}}, 1) - limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value1", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value1"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 9, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -210,7 +213,8 @@ func TestOverLimitWithLocalCache(t *testing.T) { localCache := freecache.NewCache(100) sink := &common.TestStatSink{} statsStore := stats.NewStore(sink, true) - cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, localCache, statsStore, 0.8, "") + sm := mockstats.NewMockStatManager(statsStore) + cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, localCache, sm, 0.8, "") localCacheStats := limiter.NewLocalCacheStats(localCache, statsStore.Scope("localcache")) // Test Near Limit Stats. Under Near Limit Ratio @@ -223,7 +227,7 @@ func TestOverLimitWithLocalCache(t *testing.T) { request := common.NewRateLimitRequest("domain", [][][2]string{{{"key4", "value4"}}}, 1) limits := []*config.RateLimit{ - config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, "key4_value4", statsStore)} + config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, sm.NewStats("key4_value4"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{ @@ -306,7 +310,8 @@ func TestNearLimit(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) client := mock_memcached.NewMockClient(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) - cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, statsStore, 0.8, "") + sm := mockstats.NewMockStatManager(statsStore) + cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, sm, 0.8, "") // Test Near Limit Stats. Under Near Limit Ratio timeSource.EXPECT().UnixNow().Return(int64(1000000)).MaxTimes(3) @@ -318,7 +323,7 @@ func TestNearLimit(t *testing.T) { request := common.NewRateLimitRequest("domain", [][][2]string{{{"key4", "value4"}}}, 1) limits := []*config.RateLimit{ - config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, "key4_value4", statsStore)} + config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, sm.NewStats("key4_value4"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{ @@ -371,7 +376,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().Increment("domain_key5_value5_1234", uint64(3)).Return(uint64(5), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key5", "value5"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key5_value5", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key5_value5"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 15, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -389,7 +394,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().Increment("domain_key6_value6_1234", uint64(2)).Return(uint64(7), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key6", "value6"}}}, 2) - limits = []*config.RateLimit{config.NewRateLimit(8, pb.RateLimitResponse_RateLimit_SECOND, "key6_value6", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(8, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key6_value6"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 1, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -407,7 +412,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().Increment("domain_key7_value7_1234", uint64(3)).Return(uint64(19), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key7", "value7"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key7_value7", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key7_value7"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 1, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -425,7 +430,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().Increment("domain_key8_value8_1234", uint64(3)).Return(uint64(22), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key8", "value8"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key8_value8", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key8_value8"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -443,7 +448,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().Increment("domain_key9_value9_1234", uint64(7)).Return(uint64(22), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key9", "value9"}}}, 7) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key9_value9", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key9_value9"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -461,7 +466,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().Increment("domain_key10_value10_1234", uint64(3)).Return(uint64(30), nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key10", "value10"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key10_value10", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key10_value10"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -483,7 +488,8 @@ func TestMemcacheWithJitter(t *testing.T) { client := mock_memcached.NewMockClient(controller) jitterSource := mock_utils.NewMockJitterRandSource(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) - cache := memcached.NewRateLimitCacheImpl(client, timeSource, rand.New(jitterSource), 3600, nil, statsStore, 0.8, "") + sm := mockstats.NewMockStatManager(statsStore) + cache := memcached.NewRateLimitCacheImpl(client, timeSource, rand.New(jitterSource), 3600, nil, sm, 0.8, "") timeSource.EXPECT().UnixNow().Return(int64(1234)).MaxTimes(3) jitterSource.EXPECT().Int63().Return(int64(100)) @@ -504,7 +510,7 @@ func TestMemcacheWithJitter(t *testing.T) { ).Return(nil) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 9, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -525,7 +531,8 @@ func TestMemcacheAdd(t *testing.T) { timeSource := mock_utils.NewMockTimeSource(controller) client := mock_memcached.NewMockClient(controller) statsStore := stats.NewStore(stats.NewNullSink(), false) - cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, statsStore, 0.8, "") + sm := mockstats.NewMockStatManager(statsStore) + cache := memcached.NewRateLimitCacheImpl(client, timeSource, nil, 0, nil, sm, 0.8, "") // Test a race condition with the initial add timeSource.EXPECT().UnixNow().Return(int64(1234)).MaxTimes(3) @@ -546,7 +553,7 @@ func TestMemcacheAdd(t *testing.T) { uint64(2), nil) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 9, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -570,7 +577,7 @@ func TestMemcacheAdd(t *testing.T) { ).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key2", "value2"}}}, 1) - limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key2_value2", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, sm.NewStats("key2_value2"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 9, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, diff --git a/test/mocks/config/config.go b/test/mocks/config/config.go index 38d5b347..b34328dc 100644 --- a/test/mocks/config/config.go +++ b/test/mocks/config/config.go @@ -8,8 +8,8 @@ import ( context "context" envoy_extensions_common_ratelimit_v3 "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3" config "github.com/envoyproxy/ratelimit/src/config" + stats "github.com/envoyproxy/ratelimit/src/stats" gomock "github.com/golang/mock/gomock" - stats "github.com/lyft/gostats" reflect "reflect" ) @@ -88,7 +88,7 @@ func (m *MockRateLimitConfigLoader) EXPECT() *MockRateLimitConfigLoaderMockRecor } // Load mocks base method -func (m *MockRateLimitConfigLoader) Load(arg0 []config.RateLimitConfigToLoad, arg1 stats.Scope) config.RateLimitConfig { +func (m *MockRateLimitConfigLoader) Load(arg0 []config.RateLimitConfigToLoad, arg1 stats.Manager) config.RateLimitConfig { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Load", arg0, arg1) ret0, _ := ret[0].(config.RateLimitConfig) diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go new file mode 100644 index 00000000..14fd7aa4 --- /dev/null +++ b/test/mocks/stats/manager.go @@ -0,0 +1,77 @@ +package stats + +import ( + stat "github.com/envoyproxy/ratelimit/src/stats" + stats "github.com/lyft/gostats" + logger "github.com/sirupsen/logrus" +) + +type MockStatManager struct { + store stats.Store +} + +func (m *MockStatManager) GetStatsStore() stats.Store { + return m.store +} + +func (m *MockStatManager) NewShouldRateLimitStats() stat.ShouldRateLimitStats { + s := m.store.Scope("call.should_rate_limit") + ret := stat.ShouldRateLimitStats{} + ret.RedisError = s.NewCounter("redis_error") + ret.ServiceError = s.NewCounter("service_error") + return ret +} + +func (m *MockStatManager) NewServiceStats() stat.ServiceStats { + ret := stat.ServiceStats{} + ret.ConfigLoadSuccess = m.store.NewCounter("config_load_success") + ret.ConfigLoadError = m.store.NewCounter("config_load_error") + ret.ShouldRateLimit = m.NewShouldRateLimitStats() + return ret +} + +func (m *MockStatManager) NewShouldRateLimitLegacyStats() stat.ShouldRateLimitLegacyStats { + s := m.store.Scope("call.should_rate_limit_legacy") + return stat.ShouldRateLimitLegacyStats{ + ReqConversionError: s.NewCounter("req_conversion_error"), + RespConversionError: s.NewCounter("resp_conversion_error"), + ShouldRateLimitError: s.NewCounter("should_rate_limit_error"), + } +} + +//todo: review mock implementation +func (m *MockStatManager) NewStats(key string) stat.RateLimitStats { + ret := stat.RateLimitStats{} + logger.Debugf("outputing test stats %s", key) + ret.Key = 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") + ret.OverLimitWithLocalCache = m.store.NewCounter(key + ".over_limit_with_local_cache") + ret.WithinLimit = m.store.NewCounter(key + ".within_limit") + return ret +} + +func (m *MockStatManager) AddTotalHits(u uint64, rlStats stat.RateLimitStats) { + rlStats.TotalHits.Add(u) +} + +func (this *MockStatManager) AddOverLimit(u uint64, rlStats stat.RateLimitStats) { + rlStats.OverLimit.Add(u) +} + +func (this *MockStatManager) AddNearLimit(u uint64, rlStats stat.RateLimitStats) { + rlStats.NearLimit.Add(u) +} + +func (this *MockStatManager) AddOverLimitWithLocalCache(u uint64, rlStats stat.RateLimitStats) { + rlStats.OverLimitWithLocalCache.Add(u) +} + +func (this *MockStatManager) AddWithinLimit(u uint64, rlStats stat.RateLimitStats) { + rlStats.WithinLimit.Add(u) +} + +func NewMockStatManager(store stats.Store) stat.Manager { + return &MockStatManager{store: store} +} diff --git a/test/redis/bench_test.go b/test/redis/bench_test.go index 6c190ea7..37bca184 100644 --- a/test/redis/bench_test.go +++ b/test/redis/bench_test.go @@ -2,6 +2,7 @@ package redis_test import ( "context" + "github.com/envoyproxy/ratelimit/test/mocks/stats" "runtime" "testing" "time" @@ -10,7 +11,7 @@ import ( "github.com/envoyproxy/ratelimit/src/config" "github.com/envoyproxy/ratelimit/src/redis" "github.com/envoyproxy/ratelimit/src/utils" - stats "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" "math/rand" @@ -40,13 +41,14 @@ func BenchmarkParallelDoLimit(b *testing.B) { mkDoLimitBench := func(pipelineWindow time.Duration, pipelineLimit int) func(*testing.B) { return func(b *testing.B) { - statsStore := stats.NewStore(stats.NewNullSink(), false) + statsStore := gostats.NewStore(gostats.NewNullSink(), false) + sm := stats.NewMockStatManager(statsStore) client := redis.NewClientImpl(statsStore, false, "", "single", "127.0.0.1:6379", poolSize, pipelineWindow, pipelineLimit) defer client.Close() - cache := redis.NewFixedRateLimitCacheImpl(client, nil, utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), 10, nil, 0.8, "") + cache := redis.NewFixedRateLimitCacheImpl(client, nil, utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), 10, nil, 0.8, "", sm) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(1000000000, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(1000000000, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} // wait for the pool to fill up for { diff --git a/test/redis/fixed_cache_impl_test.go b/test/redis/fixed_cache_impl_test.go index 65883f4b..e0723333 100644 --- a/test/redis/fixed_cache_impl_test.go +++ b/test/redis/fixed_cache_impl_test.go @@ -1,6 +1,7 @@ package redis_test import ( + "github.com/envoyproxy/ratelimit/test/mocks/stats" "testing" "github.com/coocood/freecache" @@ -11,7 +12,7 @@ import ( "github.com/envoyproxy/ratelimit/src/limiter" "github.com/envoyproxy/ratelimit/src/redis" "github.com/envoyproxy/ratelimit/src/utils" - stats "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" "math/rand" @@ -36,17 +37,18 @@ func testRedis(usePerSecondRedis bool) func(*testing.T) { assert := assert.New(t) controller := gomock.NewController(t) defer controller.Finish() + statsStore := gostats.NewStore(gostats.NewNullSink(), false) + sm := stats.NewMockStatManager(statsStore) client := mock_redis.NewMockClient(controller) perSecondClient := mock_redis.NewMockClient(controller) timeSource := mock_utils.NewMockTimeSource(controller) var cache limiter.RateLimitCache if usePerSecondRedis { - cache = redis.NewFixedRateLimitCacheImpl(client, perSecondClient, timeSource, rand.New(rand.NewSource(1)), 0, nil, 0.8, "") + cache = redis.NewFixedRateLimitCacheImpl(client, perSecondClient, timeSource, rand.New(rand.NewSource(1)), 0, nil, 0.8, "", sm) } else { - cache = redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(rand.NewSource(1)), 0, nil, 0.8, "") + cache = redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(rand.NewSource(1)), 0, nil, 0.8, "", sm) } - statsStore := stats.NewStore(stats.NewNullSink(), false) timeSource.EXPECT().UnixNow().Return(int64(1234)).MaxTimes(3) var clientUsed *mock_redis.MockClient @@ -61,7 +63,7 @@ func testRedis(usePerSecondRedis bool) func(*testing.T) { clientUsed.EXPECT().PipeDo(gomock.Any()).Return(nil) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 5, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -86,7 +88,7 @@ func testRedis(usePerSecondRedis bool) func(*testing.T) { }, 1) limits = []*config.RateLimit{ nil, - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key2_value2_subkey2_subvalue2", statsStore)} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, sm.NewStats("key2_value2_subkey2_subvalue2"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: nil, LimitRemaining: 0}, {Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[1].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[1].Limit, timeSource)}}, @@ -113,8 +115,8 @@ func testRedis(usePerSecondRedis bool) func(*testing.T) { {{"key3", "value3"}, {"subkey3", "subvalue3"}}, }, 1) limits = []*config.RateLimit{ - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_HOUR, "key3_value3", statsStore), - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_DAY, "key3_value3_subkey3_subvalue3", statsStore)} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_HOUR, sm.NewStats("key3_value3")), + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_DAY, sm.NewStats("key3_value3_subkey3_subvalue3"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{ {Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}, @@ -131,7 +133,7 @@ func testRedis(usePerSecondRedis bool) func(*testing.T) { } } -func testLocalCacheStats(localCacheStats stats.StatGenerator, statsStore stats.Store, sink *common.TestStatSink, +func testLocalCacheStats(localCacheStats gostats.StatGenerator, statsStore gostats.Store, sink *common.TestStatSink, expectedHitCount int, expectedMissCount int, expectedLookUpCount int, expectedExpiredCount int, expectedEntryCount int) func(*testing.T) { return func(t *testing.T) { @@ -175,9 +177,10 @@ func TestOverLimitWithLocalCache(t *testing.T) { client := mock_redis.NewMockClient(controller) timeSource := mock_utils.NewMockTimeSource(controller) localCache := freecache.NewCache(100) - cache := redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(rand.NewSource(1)), 0, localCache, 0.8, "") + statsStore := gostats.NewStore(gostats.NewNullSink(), false) + sm := stats.NewMockStatManager(statsStore) + cache := redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(rand.NewSource(1)), 0, localCache, 0.8, "", sm) sink := &common.TestStatSink{} - statsStore := stats.NewStore(sink, true) localCacheStats := limiter.NewLocalCacheStats(localCache, statsStore.Scope("localcache")) // Test Near Limit Stats. Under Near Limit Ratio @@ -190,7 +193,7 @@ func TestOverLimitWithLocalCache(t *testing.T) { request := common.NewRateLimitRequest("domain", [][][2]string{{{"key4", "value4"}}}, 1) limits := []*config.RateLimit{ - config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, "key4_value4", statsStore)} + config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, sm.NewStats("key4_value4"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{ @@ -271,8 +274,9 @@ func TestNearLimit(t *testing.T) { client := mock_redis.NewMockClient(controller) timeSource := mock_utils.NewMockTimeSource(controller) - cache := redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(rand.NewSource(1)), 0, nil, 0.8, "") - statsStore := stats.NewStore(stats.NewNullSink(), false) + statsStore := gostats.NewStore(gostats.NewNullSink(), false) + sm := stats.NewMockStatManager(statsStore) + cache := redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(rand.NewSource(1)), 0, nil, 0.8, "", sm) // Test Near Limit Stats. Under Near Limit Ratio timeSource.EXPECT().UnixNow().Return(int64(1000000)).MaxTimes(3) @@ -284,7 +288,7 @@ func TestNearLimit(t *testing.T) { request := common.NewRateLimitRequest("domain", [][][2]string{{{"key4", "value4"}}}, 1) limits := []*config.RateLimit{ - config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, "key4_value4", statsStore)} + config.NewRateLimit(15, pb.RateLimitResponse_RateLimit_HOUR, sm.NewStats("key4_value4"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{ @@ -336,7 +340,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key5", "value5"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key5_value5", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key5_value5"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 15, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -353,7 +357,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key6", "value6"}}}, 2) - limits = []*config.RateLimit{config.NewRateLimit(8, pb.RateLimitResponse_RateLimit_SECOND, "key6_value6", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(8, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key6_value6"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 1, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -370,7 +374,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key7", "value7"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key7_value7", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key7_value7"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 1, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -387,7 +391,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key8", "value8"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key8_value8", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key8_value8"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -404,7 +408,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key9", "value9"}}}, 7) - limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, "key9_value9", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(20, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key9_value9"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -421,7 +425,7 @@ func TestNearLimit(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request = common.NewRateLimitRequest("domain", [][][2]string{{{"key10", "value10"}}}, 3) - limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key10_value10", statsStore)} + limits = []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key10_value10"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OVER_LIMIT, CurrentLimit: limits[0].Limit, LimitRemaining: 0, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, @@ -440,8 +444,9 @@ func TestRedisWithJitter(t *testing.T) { client := mock_redis.NewMockClient(controller) timeSource := mock_utils.NewMockTimeSource(controller) jitterSource := mock_utils.NewMockJitterRandSource(controller) - cache := redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(jitterSource), 3600, nil, 0.8, "") - statsStore := stats.NewStore(stats.NewNullSink(), false) + statsStore := gostats.NewStore(gostats.NewNullSink(), false) + sm := stats.NewMockStatManager(statsStore) + cache := redis.NewFixedRateLimitCacheImpl(client, nil, timeSource, rand.New(jitterSource), 3600, nil, 0.8, "", sm) timeSource.EXPECT().UnixNow().Return(int64(1234)).MaxTimes(3) jitterSource.EXPECT().Int63().Return(int64(100)) @@ -450,7 +455,7 @@ func TestRedisWithJitter(t *testing.T) { client.EXPECT().PipeDo(gomock.Any()).Return(nil) request := common.NewRateLimitRequest("domain", [][][2]string{{{"key", "value"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, "key_value", statsStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_SECOND, sm.NewStats("key_value"))} assert.Equal( []*pb.RateLimitResponse_DescriptorStatus{{Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 5, DurationUntilReset: utils.CalculateReset(limits[0].Limit, timeSource)}}, diff --git a/test/service/ratelimit_legacy_test.go b/test/service/ratelimit_legacy_test.go index a51ddbe9..a0d7bef6 100644 --- a/test/service/ratelimit_legacy_test.go +++ b/test/service/ratelimit_legacy_test.go @@ -1,6 +1,7 @@ package ratelimit_test import ( + "github.com/envoyproxy/ratelimit/src/stats" "testing" core_legacy "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" @@ -14,7 +15,6 @@ import ( "github.com/envoyproxy/ratelimit/src/service" "github.com/envoyproxy/ratelimit/test/common" "github.com/golang/mock/gomock" - "github.com/lyft/gostats" "github.com/stretchr/testify/assert" "golang.org/x/net/context" ) @@ -80,7 +80,7 @@ func TestServiceLegacy(test *testing.T) { barrier := newBarrier() t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( - func([]config.RateLimitConfigToLoad, stats.Scope) { barrier.signal() }).Return(t.config) + func([]config.RateLimitConfigToLoad, stats.Manager) { barrier.signal() }).Return(t.config) t.runtimeUpdateCallback <- 1 barrier.wait() @@ -93,7 +93,7 @@ func TestServiceLegacy(test *testing.T) { } limits := []*config.RateLimit{ - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key", t.statStore), + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key")), nil} legacyLimits, err := convertRatelimits(limits) if err != nil { @@ -120,7 +120,7 @@ func TestServiceLegacy(test *testing.T) { // Config load failure. t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( - func([]config.RateLimitConfigToLoad, stats.Scope) { + func([]config.RateLimitConfigToLoad, stats.Manager) { barrier.signal() panic(config.RateLimitConfigError("load error")) }) @@ -130,7 +130,7 @@ func TestServiceLegacy(test *testing.T) { // Config should still be valid. Also make sure order does not affect results. limits = []*config.RateLimit{ nil, - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key", t.statStore)} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} legacyLimits, err = convertRatelimits(limits) if err != nil { t.assert.FailNow(err.Error()) @@ -153,8 +153,8 @@ func TestServiceLegacy(test *testing.T) { response) t.assert.Nil(err) - t.assert.EqualValues(2, t.statStore.NewCounter("config_load_success").Value()) - t.assert.EqualValues(1, t.statStore.NewCounter("config_load_error").Value()) + t.assert.EqualValues(2, t.store.NewCounter("config_load_success").Value()) + t.assert.EqualValues(1, t.store.NewCounter("config_load_error").Value()) } func TestEmptyDomainLegacy(test *testing.T) { @@ -166,8 +166,8 @@ func TestEmptyDomainLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit domain must not be empty", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } func TestEmptyDescriptorsLegacy(test *testing.T) { @@ -179,8 +179,8 @@ func TestEmptyDescriptorsLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit descriptor list must not be empty", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } func TestCacheErrorLegacy(test *testing.T) { @@ -193,7 +193,7 @@ func TestCacheErrorLegacy(test *testing.T) { if err != nil { t.assert.FailNow(err.Error()) } - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key", t.statStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} t.config.EXPECT().GetLimit(nil, "different-domain", req.Descriptors[0]).Return(limits[0]) t.cache.EXPECT().DoLimit(nil, req, limits).Do( func(context.Context, *pb.RateLimitRequest, []*config.RateLimit) { @@ -203,8 +203,8 @@ func TestCacheErrorLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, legacyRequest) t.assert.Nil(response) t.assert.Equal("cache error", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.redis_error").Value()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.redis_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } func TestInitialLoadErrorLegacy(test *testing.T) { @@ -218,17 +218,17 @@ func TestInitialLoadErrorLegacy(test *testing.T) { t.snapshot.EXPECT().Get("config.basic_config").Return("fake_yaml").MinTimes(1) t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( - func([]config.RateLimitConfigToLoad, stats.Scope) { + func([]config.RateLimitConfigToLoad, stats.Manager) { panic(config.RateLimitConfigError("load error")) }) - service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.statStore, true) + service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.sm, true) request := common.NewRateLimitRequestLegacy("test-domain", [][][2]string{{{"hello", "world"}}}, 1) response, err := service.GetLegacyService().ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("no rate limit configuration loaded", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } diff --git a/test/service/ratelimit_test.go b/test/service/ratelimit_test.go index 12c77926..d8c6836c 100644 --- a/test/service/ratelimit_test.go +++ b/test/service/ratelimit_test.go @@ -1,6 +1,7 @@ package ratelimit_test import ( + "github.com/envoyproxy/ratelimit/src/stats" "sync" "testing" @@ -13,8 +14,9 @@ import ( mock_limiter "github.com/envoyproxy/ratelimit/test/mocks/limiter" mock_loader "github.com/envoyproxy/ratelimit/test/mocks/runtime/loader" mock_snapshot "github.com/envoyproxy/ratelimit/test/mocks/runtime/snapshot" + mock_stats "github.com/envoyproxy/ratelimit/test/mocks/stats" "github.com/golang/mock/gomock" - stats "github.com/lyft/gostats" + gostats "github.com/lyft/gostats" "github.com/stretchr/testify/assert" "golang.org/x/net/context" ) @@ -55,7 +57,8 @@ type rateLimitServiceTestSuite struct { configLoader *mock_config.MockRateLimitConfigLoader config *mock_config.MockRateLimitConfig runtimeUpdateCallback chan<- int - statStore stats.Store + sm stats.Manager + store gostats.Store } func commonSetup(t *testing.T) rateLimitServiceTestSuite { @@ -67,7 +70,8 @@ func commonSetup(t *testing.T) rateLimitServiceTestSuite { ret.cache = mock_limiter.NewMockRateLimitCache(ret.controller) ret.configLoader = mock_config.NewMockRateLimitConfigLoader(ret.controller) ret.config = mock_config.NewMockRateLimitConfig(ret.controller) - ret.statStore = stats.NewStore(stats.NewNullSink(), false) + ret.store = gostats.NewStore(gostats.NewNullSink(), false) + ret.sm = mock_stats.NewMockStatManager(ret.store) return ret } @@ -82,7 +86,7 @@ func (this *rateLimitServiceTestSuite) setupBasicService() ratelimit.RateLimitSe this.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Return(this.config) - return ratelimit.NewService(this.runtime, this.cache, this.configLoader, this.statStore, true) + return ratelimit.NewService(this.runtime, this.cache, this.configLoader, this.sm, true) } func TestService(test *testing.T) { @@ -109,7 +113,7 @@ func TestService(test *testing.T) { barrier := newBarrier() t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( - func([]config.RateLimitConfigToLoad, stats.Scope) { barrier.signal() }).Return(t.config) + func([]config.RateLimitConfigToLoad, stats.Manager) { barrier.signal() }).Return(t.config) t.runtimeUpdateCallback <- 1 barrier.wait() @@ -117,7 +121,7 @@ func TestService(test *testing.T) { request = common.NewRateLimitRequest( "different-domain", [][][2]string{{{"foo", "bar"}}, {{"hello", "world"}}}, 1) limits := []*config.RateLimit{ - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key", t.statStore), + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key")), nil} t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[0]).Return(limits[0]) t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[1]).Return(limits[1]) @@ -139,7 +143,7 @@ func TestService(test *testing.T) { // Config load failure. t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( - func([]config.RateLimitConfigToLoad, stats.Scope) { + func([]config.RateLimitConfigToLoad, stats.Manager) { barrier.signal() panic(config.RateLimitConfigError("load error")) }) @@ -149,7 +153,7 @@ func TestService(test *testing.T) { // Config should still be valid. Also make sure order does not affect results. limits = []*config.RateLimit{ nil, - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key", t.statStore)} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[0]).Return(limits[0]) t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[1]).Return(limits[1]) t.cache.EXPECT().DoLimit(nil, request, limits).Return( @@ -167,8 +171,8 @@ func TestService(test *testing.T) { response) t.assert.Nil(err) - t.assert.EqualValues(2, t.statStore.NewCounter("config_load_success").Value()) - t.assert.EqualValues(1, t.statStore.NewCounter("config_load_error").Value()) + t.assert.EqualValues(2, t.store.NewCounter("config_load_success").Value()) + t.assert.EqualValues(1, t.store.NewCounter("config_load_error").Value()) } func TestEmptyDomain(test *testing.T) { @@ -180,7 +184,7 @@ func TestEmptyDomain(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit domain must not be empty", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) } func TestEmptyDescriptors(test *testing.T) { @@ -192,7 +196,7 @@ func TestEmptyDescriptors(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit descriptor list must not be empty", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) } func TestCacheError(test *testing.T) { @@ -201,7 +205,7 @@ func TestCacheError(test *testing.T) { service := t.setupBasicService() request := common.NewRateLimitRequest("different-domain", [][][2]string{{{"foo", "bar"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, "key", t.statStore)} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[0]).Return(limits[0]) t.cache.EXPECT().DoLimit(nil, request, limits).Do( func(context.Context, *pb.RateLimitRequest, []*config.RateLimit) { @@ -211,7 +215,7 @@ func TestCacheError(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("cache error", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.redis_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.redis_error").Value()) } func TestInitialLoadError(test *testing.T) { @@ -225,14 +229,14 @@ func TestInitialLoadError(test *testing.T) { t.snapshot.EXPECT().Get("config.basic_config").Return("fake_yaml").MinTimes(1) t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( - func([]config.RateLimitConfigToLoad, stats.Scope) { + func([]config.RateLimitConfigToLoad, stats.Manager) { panic(config.RateLimitConfigError("load error")) }) - service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.statStore, true) + service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.sm, true) request := common.NewRateLimitRequest("test-domain", [][][2]string{{{"hello", "world"}}}, 1) response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("no rate limit configuration loaded", err.Error()) - t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) } From ed9a52cb13c83faa3451b3547c83c0269dc1ca04 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Wed, 17 Mar 2021 16:55:59 -0300 Subject: [PATCH 02/18] Revert config_test import renaming Signed-off-by: Pablo Radnic --- test/config/config_test.go | 52 +++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/config/config_test.go b/test/config/config_test.go index e0f2a2e0..14f64bbe 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -24,8 +24,8 @@ func loadFile(path string) []config.RateLimitConfigToLoad { func TestBasicConfig(t *testing.T) { assert := assert.New(t) - newStore := stats.NewStore(stats.NewNullSink(), false) - rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), mockstats.NewMockStatManager(newStore)) + stats := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), mockstats.NewMockStatManager(stats)) rlConfig.Dump() assert.Nil(rlConfig.GetLimit(nil, "foo_domain", &pb_struct.RateLimitDescriptor{})) assert.Nil(rlConfig.GetLimit(nil, "test-domain", &pb_struct.RateLimitDescriptor{})) @@ -69,10 +69,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(5, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_SECOND, rl.Limit.Unit) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -86,13 +86,13 @@ func TestBasicConfig(t *testing.T) { assert.EqualValues(10, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_SECOND, rl.Limit.Unit) assert.EqualValues( - 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.total_hits").Value()) + 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.total_hits").Value()) assert.EqualValues( - 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.over_limit").Value()) + 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.over_limit").Value()) assert.EqualValues( - 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.near_limit").Value()) + 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.near_limit").Value()) assert.EqualValues( - 1, newStore.NewCounter("test-domain.key1_value1.subkey1_subvalue1.within_limit").Value()) + 1, stats.NewCounter("test-domain.key1_value1.subkey1_subvalue1.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -105,10 +105,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(20, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_MINUTE, rl.Limit.Unit) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -121,10 +121,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(30, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_MINUTE, rl.Limit.Unit) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key2_value2.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key2_value2.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -144,10 +144,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(1, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_HOUR, rl.Limit.Unit) - assert.EqualValues(1, newStore.NewCounter("test-domain.key3.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key3.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key3.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key3.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key3.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key3.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key3.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key3.within_limit").Value()) rl = rlConfig.GetLimit( nil, "test-domain", @@ -160,10 +160,10 @@ func TestBasicConfig(t *testing.T) { rl.Stats.WithinLimit.Inc() assert.EqualValues(1, rl.Limit.RequestsPerUnit) assert.Equal(pb.RateLimitResponse_RateLimit_DAY, rl.Limit.Unit) - assert.EqualValues(1, newStore.NewCounter("test-domain.key4.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key4.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key4.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key4.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key4.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key4.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key4.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key4.within_limit").Value()) } func TestConfigLimitOverride(t *testing.T) { From 92758a1d64545ae04a5fb277514e4c64287881e0 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Thu, 8 Apr 2021 12:05:44 -0300 Subject: [PATCH 03/18] Unwrap ratelimit metric addition Signed-off-by: Pablo Radnic --- src/limiter/base_limiter.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/limiter/base_limiter.go b/src/limiter/base_limiter.go index 65f88646..361858ed 100644 --- a/src/limiter/base_limiter.go +++ b/src/limiter/base_limiter.go @@ -49,7 +49,7 @@ func (this *BaseRateLimiter) GenerateCacheKeys(request *pb.RateLimitRequest, cacheKeys[i] = this.cacheKeyGenerator.GenerateCacheKey(request.Domain, request.Descriptors[i], limits[i], now) // Increase statistics for limits hit by their respective requests. if limits[i] != nil { - this.Manager.AddTotalHits(uint64(hitsAddend), limits[i].Stats) + limits[i].Stats.TotalHits.Add(uint64(hitsAddend)) } } return cacheKeys @@ -76,8 +76,8 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo * nil, 0) } if isOverLimitWithLocalCache { - this.Manager.AddOverLimit(uint64(hitsAddend), limitInfo.limit.Stats) - this.Manager.AddOverLimitWithLocalCache(uint64(hitsAddend), limitInfo.limit.Stats) + limitInfo.limit.Stats.OverLimit.Add(uint64(hitsAddend)) + limitInfo.limit.Stats.OverLimitWithLocalCache.Add(uint64(hitsAddend)) return this.generateResponseDescriptorStatus(pb.RateLimitResponse_OVER_LIMIT, limitInfo.limit.Limit, 0) } @@ -112,7 +112,7 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo * // The limit is OK but we additionally want to know if we are near the limit. this.checkNearLimitThreshold(limitInfo, hitsAddend) - this.Manager.AddWithinLimit(uint64(hitsAddend), limitInfo.limit.Stats) + limitInfo.limit.Stats.WithinLimit.Add(uint64(hitsAddend)) } return responseDescriptorStatus } @@ -137,16 +137,13 @@ func (this *BaseRateLimiter) checkOverLimitThreshold(limitInfo *LimitInfo, hitsA // Otherwise, only the difference between the current limit value and the over limit threshold // were over limit hits. if limitInfo.limitBeforeIncrease >= limitInfo.overLimitThreshold { - this.Manager.AddOverLimit(uint64(hitsAddend), limitInfo.limit.Stats) + limitInfo.limit.Stats.OverLimit.Add(uint64(hitsAddend)) } else { - this.Manager.AddOverLimit(uint64(limitInfo.limitAfterIncrease-limitInfo.overLimitThreshold), limitInfo.limit.Stats) + limitInfo.limit.Stats.OverLimit.Add(uint64(limitInfo.limitAfterIncrease-limitInfo.overLimitThreshold)) // If the limit before increase was below the over limit value, then some of the hits were // in the near limit range. - this.Manager.AddNearLimit( - uint64(limitInfo.overLimitThreshold-utils.Max(limitInfo.nearLimitThreshold, limitInfo.limitBeforeIncrease)), - limitInfo.limit.Stats, - ) + limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.overLimitThreshold-utils.Max(limitInfo.nearLimitThreshold, limitInfo.limitBeforeIncrease))) } } @@ -157,9 +154,9 @@ func (this *BaseRateLimiter) checkNearLimitThreshold(limitInfo *LimitInfo, hitsA // only the difference between the current limit value and the near limit threshold were near // limit hits. if limitInfo.limitBeforeIncrease >= limitInfo.nearLimitThreshold { - this.Manager.AddNearLimit(uint64(hitsAddend), limitInfo.limit.Stats) + limitInfo.limit.Stats.NearLimit.Add(uint64(hitsAddend)) } else { - this.Manager.AddNearLimit(uint64(limitInfo.limitAfterIncrease-limitInfo.nearLimitThreshold), limitInfo.limit.Stats) + limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.limitAfterIncrease-limitInfo.nearLimitThreshold)) } } } From 4fd402cbd75906413fa773897fb16ba36ac5e875 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Thu, 8 Apr 2021 15:22:18 -0300 Subject: [PATCH 04/18] Remove setter methods Signed-off-by: Pablo Radnic --- src/limiter/base_limiter.go | 6 +++--- src/service_cmd/runner/runner.go | 2 +- src/stats/manager.go | 5 ----- src/stats/manager_impl.go | 21 --------------------- test/mocks/stats/manager.go | 20 -------------------- 5 files changed, 4 insertions(+), 50 deletions(-) diff --git a/src/limiter/base_limiter.go b/src/limiter/base_limiter.go index 361858ed..03e4fae2 100644 --- a/src/limiter/base_limiter.go +++ b/src/limiter/base_limiter.go @@ -139,11 +139,11 @@ func (this *BaseRateLimiter) checkOverLimitThreshold(limitInfo *LimitInfo, hitsA if limitInfo.limitBeforeIncrease >= limitInfo.overLimitThreshold { limitInfo.limit.Stats.OverLimit.Add(uint64(hitsAddend)) } else { - limitInfo.limit.Stats.OverLimit.Add(uint64(limitInfo.limitAfterIncrease-limitInfo.overLimitThreshold)) + limitInfo.limit.Stats.OverLimit.Add(uint64(limitInfo.limitAfterIncrease - limitInfo.overLimitThreshold)) // If the limit before increase was below the over limit value, then some of the hits were // in the near limit range. - limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.overLimitThreshold-utils.Max(limitInfo.nearLimitThreshold, limitInfo.limitBeforeIncrease))) + limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.overLimitThreshold - utils.Max(limitInfo.nearLimitThreshold, limitInfo.limitBeforeIncrease))) } } @@ -156,7 +156,7 @@ func (this *BaseRateLimiter) checkNearLimitThreshold(limitInfo *LimitInfo, hitsA if limitInfo.limitBeforeIncrease >= limitInfo.nearLimitThreshold { limitInfo.limit.Stats.NearLimit.Add(uint64(hitsAddend)) } else { - limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.limitAfterIncrease-limitInfo.nearLimitThreshold)) + limitInfo.limit.Stats.NearLimit.Add(uint64(limitInfo.limitAfterIncrease - limitInfo.nearLimitThreshold)) } } } diff --git a/src/service_cmd/runner/runner.go b/src/service_cmd/runner/runner.go index 17ab342c..eb89b5bb 100644 --- a/src/service_cmd/runner/runner.go +++ b/src/service_cmd/runner/runner.go @@ -1,8 +1,8 @@ package runner import ( - "github.com/envoyproxy/ratelimit/src/stats" "github.com/envoyproxy/ratelimit/src/metrics" + "github.com/envoyproxy/ratelimit/src/stats" "io" "math/rand" "net/http" diff --git a/src/stats/manager.go b/src/stats/manager.go index eb902b97..2b89ecf9 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -3,11 +3,6 @@ package stats import stats "github.com/lyft/gostats" type Manager interface { - AddTotalHits(u uint64, rlStats RateLimitStats) - AddOverLimit(u uint64, rlStats RateLimitStats) - AddNearLimit(u uint64, rlStats RateLimitStats) - AddOverLimitWithLocalCache(u uint64, rlStats RateLimitStats) - AddWithinLimit(u uint64, rlStats RateLimitStats) NewStats(key string) RateLimitStats NewShouldRateLimitStats() ShouldRateLimitStats NewServiceStats() ServiceStats diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index 6d9ff5b3..c9f8f70c 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -31,26 +31,6 @@ func (this *ManagerImpl) GetStatsStore() gostats.Store { return this.store } -func (this *ManagerImpl) AddTotalHits(u uint64, rlStats RateLimitStats) { - rlStats.TotalHits.Add(u) -} - -func (this *ManagerImpl) AddOverLimit(u uint64, rlStats RateLimitStats) { - rlStats.OverLimit.Add(u) -} - -func (this *ManagerImpl) AddNearLimit(u uint64, rlStats RateLimitStats) { - rlStats.NearLimit.Add(u) -} - -func (this *ManagerImpl) AddOverLimitWithLocalCache(u uint64, rlStats RateLimitStats) { - rlStats.OverLimitWithLocalCache.Add(u) -} - -func (this *ManagerImpl) AddWithinLimit(u uint64, rlStats RateLimitStats) { - rlStats.WithinLimit.Add(u) -} - // Create new rate descriptor stats for a descriptor tuple. // @param key supplies the fully resolved descriptor tuple. // @return new stats. @@ -66,7 +46,6 @@ func (this *ManagerImpl) NewStats(key string) RateLimitStats { return ret } - type ShouldRateLimitLegacyStats struct { ReqConversionError gostats.Counter RespConversionError gostats.Counter diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go index 14fd7aa4..5ffb9fa6 100644 --- a/test/mocks/stats/manager.go +++ b/test/mocks/stats/manager.go @@ -52,26 +52,6 @@ func (m *MockStatManager) NewStats(key string) stat.RateLimitStats { return ret } -func (m *MockStatManager) AddTotalHits(u uint64, rlStats stat.RateLimitStats) { - rlStats.TotalHits.Add(u) -} - -func (this *MockStatManager) AddOverLimit(u uint64, rlStats stat.RateLimitStats) { - rlStats.OverLimit.Add(u) -} - -func (this *MockStatManager) AddNearLimit(u uint64, rlStats stat.RateLimitStats) { - rlStats.NearLimit.Add(u) -} - -func (this *MockStatManager) AddOverLimitWithLocalCache(u uint64, rlStats stat.RateLimitStats) { - rlStats.OverLimitWithLocalCache.Add(u) -} - -func (this *MockStatManager) AddWithinLimit(u uint64, rlStats stat.RateLimitStats) { - rlStats.WithinLimit.Add(u) -} - func NewMockStatManager(store stats.Store) stat.Manager { return &MockStatManager{store: store} } From 51bf9d89b3afade15d0bcaa273c5df26ed4637c7 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Thu, 8 Apr 2021 15:27:57 -0300 Subject: [PATCH 05/18] rename variable Signed-off-by: Pablo Radnic --- test/service/ratelimit_legacy_test.go | 20 ++++++++++---------- test/service/ratelimit_test.go | 18 +++++++++--------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/test/service/ratelimit_legacy_test.go b/test/service/ratelimit_legacy_test.go index a0d7bef6..52c77f4c 100644 --- a/test/service/ratelimit_legacy_test.go +++ b/test/service/ratelimit_legacy_test.go @@ -153,8 +153,8 @@ func TestServiceLegacy(test *testing.T) { response) t.assert.Nil(err) - t.assert.EqualValues(2, t.store.NewCounter("config_load_success").Value()) - t.assert.EqualValues(1, t.store.NewCounter("config_load_error").Value()) + t.assert.EqualValues(2, t.statStore.NewCounter("config_load_success").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("config_load_error").Value()) } func TestEmptyDomainLegacy(test *testing.T) { @@ -166,8 +166,8 @@ func TestEmptyDomainLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit domain must not be empty", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } func TestEmptyDescriptorsLegacy(test *testing.T) { @@ -179,8 +179,8 @@ func TestEmptyDescriptorsLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit descriptor list must not be empty", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } func TestCacheErrorLegacy(test *testing.T) { @@ -203,8 +203,8 @@ func TestCacheErrorLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, legacyRequest) t.assert.Nil(response) t.assert.Equal("cache error", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.redis_error").Value()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.redis_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } func TestInitialLoadErrorLegacy(test *testing.T) { @@ -227,8 +227,8 @@ func TestInitialLoadErrorLegacy(test *testing.T) { response, err := service.GetLegacyService().ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("no rate limit configuration loaded", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit_legacy.should_rate_limit_error").Value()) } diff --git a/test/service/ratelimit_test.go b/test/service/ratelimit_test.go index d8c6836c..141480eb 100644 --- a/test/service/ratelimit_test.go +++ b/test/service/ratelimit_test.go @@ -58,7 +58,7 @@ type rateLimitServiceTestSuite struct { config *mock_config.MockRateLimitConfig runtimeUpdateCallback chan<- int sm stats.Manager - store gostats.Store + statStore gostats.Store } func commonSetup(t *testing.T) rateLimitServiceTestSuite { @@ -70,8 +70,8 @@ func commonSetup(t *testing.T) rateLimitServiceTestSuite { ret.cache = mock_limiter.NewMockRateLimitCache(ret.controller) ret.configLoader = mock_config.NewMockRateLimitConfigLoader(ret.controller) ret.config = mock_config.NewMockRateLimitConfig(ret.controller) - ret.store = gostats.NewStore(gostats.NewNullSink(), false) - ret.sm = mock_stats.NewMockStatManager(ret.store) + ret.statStore = gostats.NewStore(gostats.NewNullSink(), false) + ret.sm = mock_stats.NewMockStatManager(ret.statStore) return ret } @@ -171,8 +171,8 @@ func TestService(test *testing.T) { response) t.assert.Nil(err) - t.assert.EqualValues(2, t.store.NewCounter("config_load_success").Value()) - t.assert.EqualValues(1, t.store.NewCounter("config_load_error").Value()) + t.assert.EqualValues(2, t.statStore.NewCounter("config_load_success").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("config_load_error").Value()) } func TestEmptyDomain(test *testing.T) { @@ -184,7 +184,7 @@ func TestEmptyDomain(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit domain must not be empty", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) } func TestEmptyDescriptors(test *testing.T) { @@ -196,7 +196,7 @@ func TestEmptyDescriptors(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("rate limit descriptor list must not be empty", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) } func TestCacheError(test *testing.T) { @@ -215,7 +215,7 @@ func TestCacheError(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("cache error", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.redis_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.redis_error").Value()) } func TestInitialLoadError(test *testing.T) { @@ -238,5 +238,5 @@ func TestInitialLoadError(test *testing.T) { response, err := service.ShouldRateLimit(nil, request) t.assert.Nil(response) t.assert.Equal("no rate limit configuration loaded", err.Error()) - t.assert.EqualValues(1, t.store.NewCounter("call.should_rate_limit.service_error").Value()) + t.assert.EqualValues(1, t.statStore.NewCounter("call.should_rate_limit.service_error").Value()) } From fceb8fde54001c685f151869cd28e4ef5782ea25 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Fri, 9 Apr 2021 10:59:58 -0300 Subject: [PATCH 06/18] Rename stats variable Signed-off-by: Pablo Radnic --- test/config/config_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/config/config_test.go b/test/config/config_test.go index 14f64bbe..107b3a43 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -168,8 +168,8 @@ func TestBasicConfig(t *testing.T) { func TestConfigLimitOverride(t *testing.T) { assert := assert.New(t) - newStore := stats.NewStore(stats.NewNullSink(), false) - rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), mockstats.NewMockStatManager(newStore)) + stats := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(loadFile("basic_config.yaml"), mockstats.NewMockStatManager(stats)) rlConfig.Dump() // No matching domain assert.Nil(rlConfig.GetLimit(nil, "foo_domain", &pb_struct.RateLimitDescriptor{ @@ -194,10 +194,10 @@ func TestConfigLimitOverride(t *testing.T) { rl.Stats.OverLimit.Inc() rl.Stats.NearLimit.Inc() rl.Stats.WithinLimit.Inc() - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) // Change in override value doesn't erase stats rl = rlConfig.GetLimit( @@ -217,10 +217,10 @@ func TestConfigLimitOverride(t *testing.T) { RequestsPerUnit: 42, Unit: pb.RateLimitResponse_RateLimit_HOUR, }, rl.Limit) - assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) - assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) - assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) - assert.EqualValues(2, newStore.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) + assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.total_hits").Value()) + assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.over_limit").Value()) + assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.near_limit").Value()) + assert.EqualValues(2, stats.NewCounter("test-domain.key1_value1.subkey1_something.within_limit").Value()) // Different value creates a different counter rl = rlConfig.GetLimit( @@ -240,10 +240,10 @@ func TestConfigLimitOverride(t *testing.T) { rl.Stats.OverLimit.Inc() rl.Stats.NearLimit.Inc() rl.Stats.WithinLimit.Inc() - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.total_hits").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.over_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.near_limit").Value()) - assert.EqualValues(1, newStore.NewCounter("test-domain.key1_value1.subkey1_something_else.within_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.total_hits").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.over_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.near_limit").Value()) + assert.EqualValues(1, stats.NewCounter("test-domain.key1_value1.subkey1_something_else.within_limit").Value()) } func expectConfigPanic(t *testing.T, call func(), expectedError string) { From c846e96f58976b4eb20fa6a77d3ea9019be90ea8 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 11:46:53 -0300 Subject: [PATCH 07/18] Rename manager variable to statsManager Signed-off-by: Pablo Radnic --- src/config/config.go | 4 ++-- src/config/config_impl.go | 24 +++++++++++------------ src/config_check_cmd/main.go | 4 ++-- src/limiter/base_limiter.go | 6 +++--- src/memcached/cache_impl.go | 8 ++++---- src/redis/cache_impl.go | 4 ++-- src/redis/fixed_cache_impl.go | 4 ++-- src/server/server_impl.go | 8 ++++---- src/service/ratelimit.go | 14 +++++++------- src/service_cmd/runner/runner.go | 28 +++++++++++++-------------- test/mocks/stats/manager.go | 28 +++++++++++++-------------- test/service/ratelimit_legacy_test.go | 8 ++++---- test/service/ratelimit_test.go | 14 +++++++------- 13 files changed, 77 insertions(+), 77 deletions(-) diff --git a/src/config/config.go b/src/config/config.go index 49b5e280..dbaf8968 100644 --- a/src/config/config.go +++ b/src/config/config.go @@ -44,8 +44,8 @@ type RateLimitConfigToLoad struct { type RateLimitConfigLoader interface { // Load a new configuration from a list of YAML files. // @param configs supplies a list of full YAML files in string form. - // @param statsScope supplies the stats scope to use for limit stats during runtime. + // @param statsManager supplies the statsManager to initialize stats during runtime. // @return a new configuration. // @throws RateLimitConfigError if the configuration could not be created. - Load(configs []RateLimitConfigToLoad, manager stats.Manager) RateLimitConfig + Load(configs []RateLimitConfigToLoad, statsManager stats.Manager) RateLimitConfig } diff --git a/src/config/config_impl.go b/src/config/config_impl.go index 6f1c67ba..552f1476 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -39,8 +39,8 @@ type rateLimitDomain struct { } type rateLimitConfigImpl struct { - domains map[string]*rateLimitDomain - manager stats.Manager + domains map[string]*rateLimitDomain + statsManager stats.Manager } var validKeys = map[string]bool{ @@ -89,8 +89,8 @@ func newRateLimitConfigError(config RateLimitConfigToLoad, err string) RateLimit // @param config supplies the config file that owns the descriptor. // @param parentKey supplies the fully resolved key name that owns this config level. // @param descriptors supplies the YAML descriptors to load. -// @param manager that owns the stats.Scope. -func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, parentKey string, descriptors []yamlDescriptor, manager stats.Manager) { +// @param statsManager that owns the stats.Scope. +func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, parentKey string, descriptors []yamlDescriptor, statsManager stats.Manager) { for _, descriptorConfig := range descriptors { if descriptorConfig.Key == "" { @@ -121,7 +121,7 @@ func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, p } rateLimit = NewRateLimit( - descriptorConfig.RateLimit.RequestsPerUnit, pb.RateLimitResponse_RateLimit_Unit(value), manager.NewStats(newParentKey)) + descriptorConfig.RateLimit.RequestsPerUnit, pb.RateLimitResponse_RateLimit_Unit(value), statsManager.NewStats(newParentKey)) rateLimitDebugString = fmt.Sprintf( " ratelimit={requests_per_unit=%d, unit=%s}", rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit.String()) @@ -130,7 +130,7 @@ func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, p logger.Debugf( "loading descriptor: key=%s%s", newParentKey, rateLimitDebugString) newDescriptor := &rateLimitDescriptor{map[string]*rateLimitDescriptor{}, rateLimit} - newDescriptor.loadDescriptors(config, newParentKey+".", descriptorConfig.Descriptors, manager) + newDescriptor.loadDescriptors(config, newParentKey+".", descriptorConfig.Descriptors, statsManager) this.descriptors[finalKey] = newDescriptor } } @@ -210,7 +210,7 @@ func (this *rateLimitConfigImpl) loadConfig(config RateLimitConfigToLoad) { logger.Debugf("loading domain: %s", root.Domain) newDomain := &rateLimitDomain{rateLimitDescriptor{map[string]*rateLimitDescriptor{}, nil}} - newDomain.loadDescriptors(config, root.Domain+".", root.Descriptors, this.manager) + newDomain.loadDescriptors(config, root.Domain+".", root.Descriptors, this.statsManager) this.domains[root.Domain] = newDomain } @@ -240,7 +240,7 @@ func (this *rateLimitConfigImpl) GetLimit( rateLimit = NewRateLimit( descriptor.GetLimit().GetRequestsPerUnit(), rateLimitOverrideUnit, - this.manager.NewStats(rateLimitKey)) + this.statsManager.NewStats(rateLimitKey)) return rateLimit } @@ -282,9 +282,9 @@ func (this *rateLimitConfigImpl) GetLimit( // @param stats supplies the stats scope to use for limit stats during runtime. // @return a new config. func NewRateLimitConfigImpl( - configs []RateLimitConfigToLoad, manager stats.Manager) RateLimitConfig { + configs []RateLimitConfigToLoad, statsManager stats.Manager) RateLimitConfig { - ret := &rateLimitConfigImpl{map[string]*rateLimitDomain{}, manager} + ret := &rateLimitConfigImpl{map[string]*rateLimitDomain{}, statsManager} for _, config := range configs { ret.loadConfig(config) } @@ -295,9 +295,9 @@ func NewRateLimitConfigImpl( type rateLimitConfigLoaderImpl struct{} func (this *rateLimitConfigLoaderImpl) Load( - configs []RateLimitConfigToLoad, manager stats.Manager) RateLimitConfig { + configs []RateLimitConfigToLoad, statsManager stats.Manager) RateLimitConfig { - return NewRateLimitConfigImpl(configs, manager) + return NewRateLimitConfigImpl(configs, statsManager) } // @return a new default config loader implementation. diff --git a/src/config_check_cmd/main.go b/src/config_check_cmd/main.go index d5751cc7..b9c59608 100644 --- a/src/config_check_cmd/main.go +++ b/src/config_check_cmd/main.go @@ -22,8 +22,8 @@ func loadConfigs(allConfigs []config.RateLimitConfigToLoad) { } }() settingStruct := settings.NewSettings() - manager := stats.NewStatManager(gostats.NewStore(gostats.NewNullSink(), false), settingStruct) - config.NewRateLimitConfigImpl(allConfigs, manager) + statsManager := stats.NewStatManager(gostats.NewStore(gostats.NewNullSink(), false), settingStruct) + config.NewRateLimitConfigImpl(allConfigs, statsManager) } func main() { diff --git a/src/limiter/base_limiter.go b/src/limiter/base_limiter.go index 03e4fae2..346ff871 100644 --- a/src/limiter/base_limiter.go +++ b/src/limiter/base_limiter.go @@ -19,7 +19,7 @@ type BaseRateLimiter struct { cacheKeyGenerator CacheKeyGenerator localCache *freecache.Cache nearLimitRatio float32 - Manager stats.Manager + StatsManager stats.Manager } type LimitInfo struct { @@ -118,7 +118,7 @@ func (this *BaseRateLimiter) GetResponseDescriptorStatus(key string, limitInfo * } func NewBaseRateLimit(timeSource utils.TimeSource, jitterRand *rand.Rand, expirationJitterMaxSeconds int64, - localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string, manager stats.Manager) *BaseRateLimiter { + localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string, statsManager stats.Manager) *BaseRateLimiter { return &BaseRateLimiter{ timeSource: timeSource, JitterRand: jitterRand, @@ -126,7 +126,7 @@ func NewBaseRateLimit(timeSource utils.TimeSource, jitterRand *rand.Rand, expira cacheKeyGenerator: NewCacheKeyGenerator(cacheKeyPrefix), localCache: localCache, nearLimitRatio: nearLimitRatio, - Manager: manager, + StatsManager: statsManager, } } diff --git a/src/memcached/cache_impl.go b/src/memcached/cache_impl.go index f4e6d9c6..536c2fc2 100644 --- a/src/memcached/cache_impl.go +++ b/src/memcached/cache_impl.go @@ -175,7 +175,7 @@ func (this *rateLimitMemcacheImpl) Flush() { } func NewRateLimitCacheImpl(client Client, timeSource utils.TimeSource, jitterRand *rand.Rand, - expirationJitterMaxSeconds int64, localCache *freecache.Cache, manager stats.Manager, nearLimitRatio float32, cacheKeyPrefix string) limiter.RateLimitCache { + expirationJitterMaxSeconds int64, localCache *freecache.Cache, statsManager stats.Manager, nearLimitRatio float32, cacheKeyPrefix string) limiter.RateLimitCache { return &rateLimitMemcacheImpl{ client: client, timeSource: timeSource, @@ -183,19 +183,19 @@ func NewRateLimitCacheImpl(client Client, timeSource utils.TimeSource, jitterRan expirationJitterMaxSeconds: expirationJitterMaxSeconds, localCache: localCache, nearLimitRatio: nearLimitRatio, - baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix, manager), + baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix, statsManager), } } func NewRateLimitCacheImplFromSettings(s settings.Settings, timeSource utils.TimeSource, jitterRand *rand.Rand, - localCache *freecache.Cache, scope gostats.Scope, manager stats.Manager) limiter.RateLimitCache { + localCache *freecache.Cache, scope gostats.Scope, statsManager stats.Manager) limiter.RateLimitCache { return NewRateLimitCacheImpl( CollectStats(memcache.New(s.MemcacheHostPort...), scope.Scope("memcache")), timeSource, jitterRand, s.ExpirationJitterMaxSeconds, localCache, - manager, + statsManager, s.NearLimitRatio, s.CacheKeyPrefix, ) diff --git a/src/redis/cache_impl.go b/src/redis/cache_impl.go index 1bd488b7..7bf9eafc 100644 --- a/src/redis/cache_impl.go +++ b/src/redis/cache_impl.go @@ -11,7 +11,7 @@ import ( "github.com/envoyproxy/ratelimit/src/utils" ) -func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freecache.Cache, srv server.Server, timeSource utils.TimeSource, jitterRand *rand.Rand, expirationJitterMaxSeconds int64, manager stats.Manager) limiter.RateLimitCache { +func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freecache.Cache, srv server.Server, timeSource utils.TimeSource, jitterRand *rand.Rand, expirationJitterMaxSeconds int64, statsManager stats.Manager) limiter.RateLimitCache { var perSecondPool Client if s.RedisPerSecond { perSecondPool = NewClientImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, @@ -30,6 +30,6 @@ func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freeca localCache, s.NearLimitRatio, s.CacheKeyPrefix, - manager, + statsManager, ) } diff --git a/src/redis/fixed_cache_impl.go b/src/redis/fixed_cache_impl.go index 2a0d5f7f..d364f4ea 100644 --- a/src/redis/fixed_cache_impl.go +++ b/src/redis/fixed_cache_impl.go @@ -108,10 +108,10 @@ func (this *fixedRateLimitCacheImpl) DoLimit( func (this *fixedRateLimitCacheImpl) Flush() {} func NewFixedRateLimitCacheImpl(client Client, perSecondClient Client, timeSource utils.TimeSource, - jitterRand *rand.Rand, expirationJitterMaxSeconds int64, localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string, manager stats.Manager) limiter.RateLimitCache { + jitterRand *rand.Rand, expirationJitterMaxSeconds int64, localCache *freecache.Cache, nearLimitRatio float32, cacheKeyPrefix string, statsManager stats.Manager) limiter.RateLimitCache { return &fixedRateLimitCacheImpl{ client: client, perSecondClient: perSecondClient, - baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix, manager), + baseRateLimiter: limiter.NewBaseRateLimit(timeSource, jitterRand, expirationJitterMaxSeconds, localCache, nearLimitRatio, cacheKeyPrefix, statsManager), } } diff --git a/src/server/server_impl.go b/src/server/server_impl.go index ba3abdfc..e96d4e72 100644 --- a/src/server/server_impl.go +++ b/src/server/server_impl.go @@ -169,11 +169,11 @@ func (server *server) Runtime() loader.IFace { return server.runtime } -func NewServer(s settings.Settings, name string, manager stats.Manager, localCache *freecache.Cache, opts ...settings.Option) Server { - return newServer(s, name, manager, localCache, opts...) +func NewServer(s settings.Settings, name string, statsManager stats.Manager, localCache *freecache.Cache, opts ...settings.Option) Server { + return newServer(s, name, statsManager, localCache, opts...) } -func newServer(s settings.Settings, name string, manager stats.Manager, localCache *freecache.Cache, opts ...settings.Option) *server { +func newServer(s settings.Settings, name string, statsManager stats.Manager, localCache *freecache.Cache, opts ...settings.Option) *server { for _, opt := range opts { opt(&s) } @@ -187,7 +187,7 @@ func newServer(s settings.Settings, name string, manager stats.Manager, localCac ret.debugPort = s.DebugPort // setup stats - ret.store = manager.GetStatsStore() + ret.store = statsManager.GetStatsStore() ret.scope = ret.store.ScopeWithTags(name, s.ExtraTags) ret.store.AddStatGenerator(gostats.NewRuntimeStats(ret.scope.Scope("go"))) if localCache != nil { diff --git a/src/service/ratelimit.go b/src/service/ratelimit.go index 00d421af..4444d2f1 100644 --- a/src/service/ratelimit.go +++ b/src/service/ratelimit.go @@ -34,7 +34,7 @@ type service struct { runtimeWatchRoot bool } -func (this *service) reloadConfig(manager stats.Manager) { +func (this *service) reloadConfig(statsManager stats.Manager) { defer func() { if e := recover(); e != nil { configError, ok := e.(config.RateLimitConfigError) @@ -57,7 +57,7 @@ func (this *service) reloadConfig(manager stats.Manager) { files = append(files, config.RateLimitConfigToLoad{key, snapshot.Get(key)}) } - newConfig := this.configLoader.Load(files, manager) + newConfig := this.configLoader.Load(files, statsManager) this.stats.ConfigLoadSuccess.Inc() this.configLock.Lock() this.config = newConfig @@ -172,7 +172,7 @@ func (this *service) GetCurrentConfig() config.RateLimitConfig { } func NewService(runtime loader.IFace, cache limiter.RateLimitCache, - configLoader config.RateLimitConfigLoader, manager stats.Manager, runtimeWatchRoot bool) RateLimitServiceServer { + configLoader config.RateLimitConfigLoader, statsManager stats.Manager, runtimeWatchRoot bool) RateLimitServiceServer { newService := &service{ runtime: runtime, @@ -181,24 +181,24 @@ func NewService(runtime loader.IFace, cache limiter.RateLimitCache, config: nil, runtimeUpdateEvent: make(chan int), cache: cache, - stats: manager.NewServiceStats(), + stats: statsManager.NewServiceStats(), runtimeWatchRoot: runtimeWatchRoot, } newService.legacy = &legacyService{ s: newService, - shouldRateLimitLegacyStats: manager.NewShouldRateLimitLegacyStats(), + shouldRateLimitLegacyStats: statsManager.NewShouldRateLimitLegacyStats(), } runtime.AddUpdateCallback(newService.runtimeUpdateEvent) - newService.reloadConfig(manager) + newService.reloadConfig(statsManager) go func() { // No exit right now. for { logger.Debugf("waiting for runtime update") <-newService.runtimeUpdateEvent logger.Debugf("got runtime update and reloading config") - newService.reloadConfig(manager) + newService.reloadConfig(statsManager) } }() diff --git a/src/service_cmd/runner/runner.go b/src/service_cmd/runner/runner.go index eb89b5bb..c8fb45e3 100644 --- a/src/service_cmd/runner/runner.go +++ b/src/service_cmd/runner/runner.go @@ -29,24 +29,24 @@ import ( ) type Runner struct { - manager stats.Manager - settings settings.Settings - srv server.Server - mu sync.Mutex + statsManager stats.Manager + settings settings.Settings + srv server.Server + mu sync.Mutex } func NewRunner(s settings.Settings) Runner { return Runner{ - manager: stats.NewStatManager(gostats.NewDefaultStore(), s), - settings: s, + statsManager: stats.NewStatManager(gostats.NewDefaultStore(), s), + settings: s, } } func (runner *Runner) GetStatsStore() gostats.Store { - return runner.manager.GetStatsStore() + return runner.statsManager.GetStatsStore() } -func createLimiter(srv server.Server, s settings.Settings, localCache *freecache.Cache, manager stats.Manager) limiter.RateLimitCache { +func createLimiter(srv server.Server, s settings.Settings, localCache *freecache.Cache, statsManager stats.Manager) limiter.RateLimitCache { switch s.BackendType { case "redis", "": return redis.NewRateLimiterCacheImplFromSettings( @@ -56,7 +56,7 @@ func createLimiter(srv server.Server, s settings.Settings, localCache *freecache utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), s.ExpirationJitterMaxSeconds, - manager) + statsManager) case "memcache": return memcached.NewRateLimitCacheImplFromSettings( s, @@ -64,7 +64,7 @@ func createLimiter(srv server.Server, s settings.Settings, localCache *freecache rand.New(utils.NewLockedSource(time.Now().Unix())), localCache, srv.Scope(), - manager) + statsManager) default: logger.Fatalf("Invalid setting for BackendType: %s", s.BackendType) panic("This line should not be reachable") @@ -95,18 +95,18 @@ func (runner *Runner) Run() { localCache = freecache.NewCache(s.LocalCacheSizeInBytes) } - serverReporter := metrics.NewServerReporter(runner.manager.GetStatsStore().ScopeWithTags("ratelimit_server", s.ExtraTags)) + serverReporter := metrics.NewServerReporter(runner.statsManager.GetStatsStore().ScopeWithTags("ratelimit_server", s.ExtraTags)) - srv := server.NewServer(s, "ratelimit", runner.manager, localCache, settings.GrpcUnaryInterceptor(serverReporter.UnaryServerInterceptor())) + srv := server.NewServer(s, "ratelimit", runner.statsManager, localCache, settings.GrpcUnaryInterceptor(serverReporter.UnaryServerInterceptor())) runner.mu.Lock() runner.srv = srv runner.mu.Unlock() service := ratelimit.NewService( srv.Runtime(), - createLimiter(srv, s, localCache, runner.manager), + createLimiter(srv, s, localCache, runner.statsManager), config.NewRateLimitConfigLoaderImpl(), - runner.manager, + runner.statsManager, s.RuntimeWatchRoot, ) diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go index 5ffb9fa6..f4d5e9ee 100644 --- a/test/mocks/stats/manager.go +++ b/test/mocks/stats/manager.go @@ -1,38 +1,38 @@ package stats import ( - stat "github.com/envoyproxy/ratelimit/src/stats" - stats "github.com/lyft/gostats" + "github.com/envoyproxy/ratelimit/src/stats" + gostats "github.com/lyft/gostats" logger "github.com/sirupsen/logrus" ) type MockStatManager struct { - store stats.Store + store gostats.Store } -func (m *MockStatManager) GetStatsStore() stats.Store { +func (m *MockStatManager) GetStatsStore() gostats.Store { return m.store } -func (m *MockStatManager) NewShouldRateLimitStats() stat.ShouldRateLimitStats { +func (m *MockStatManager) NewShouldRateLimitStats() stats.ShouldRateLimitStats { s := m.store.Scope("call.should_rate_limit") - ret := stat.ShouldRateLimitStats{} + ret := stats.ShouldRateLimitStats{} ret.RedisError = s.NewCounter("redis_error") ret.ServiceError = s.NewCounter("service_error") return ret } -func (m *MockStatManager) NewServiceStats() stat.ServiceStats { - ret := stat.ServiceStats{} +func (m *MockStatManager) NewServiceStats() stats.ServiceStats { + ret := stats.ServiceStats{} ret.ConfigLoadSuccess = m.store.NewCounter("config_load_success") ret.ConfigLoadError = m.store.NewCounter("config_load_error") ret.ShouldRateLimit = m.NewShouldRateLimitStats() return ret } -func (m *MockStatManager) NewShouldRateLimitLegacyStats() stat.ShouldRateLimitLegacyStats { +func (m *MockStatManager) NewShouldRateLimitLegacyStats() stats.ShouldRateLimitLegacyStats { s := m.store.Scope("call.should_rate_limit_legacy") - return stat.ShouldRateLimitLegacyStats{ + return stats.ShouldRateLimitLegacyStats{ ReqConversionError: s.NewCounter("req_conversion_error"), RespConversionError: s.NewCounter("resp_conversion_error"), ShouldRateLimitError: s.NewCounter("should_rate_limit_error"), @@ -40,9 +40,9 @@ func (m *MockStatManager) NewShouldRateLimitLegacyStats() stat.ShouldRateLimitLe } //todo: review mock implementation -func (m *MockStatManager) NewStats(key string) stat.RateLimitStats { - ret := stat.RateLimitStats{} - logger.Debugf("outputing test stats %s", key) +func (m *MockStatManager) NewStats(key string) stats.RateLimitStats { + ret := stats.RateLimitStats{} + logger.Debugf("outputing test gostats %s", key) ret.Key = key ret.TotalHits = m.store.NewCounter(key + ".total_hits") ret.OverLimit = m.store.NewCounter(key + ".over_limit") @@ -52,6 +52,6 @@ func (m *MockStatManager) NewStats(key string) stat.RateLimitStats { return ret } -func NewMockStatManager(store stats.Store) stat.Manager { +func NewMockStatManager(store gostats.Store) stats.Manager { return &MockStatManager{store: store} } diff --git a/test/service/ratelimit_legacy_test.go b/test/service/ratelimit_legacy_test.go index 52c77f4c..2b13ac5e 100644 --- a/test/service/ratelimit_legacy_test.go +++ b/test/service/ratelimit_legacy_test.go @@ -93,7 +93,7 @@ func TestServiceLegacy(test *testing.T) { } limits := []*config.RateLimit{ - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key")), + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.statsManager.NewStats("key")), nil} legacyLimits, err := convertRatelimits(limits) if err != nil { @@ -130,7 +130,7 @@ func TestServiceLegacy(test *testing.T) { // Config should still be valid. Also make sure order does not affect results. limits = []*config.RateLimit{ nil, - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.statsManager.NewStats("key"))} legacyLimits, err = convertRatelimits(limits) if err != nil { t.assert.FailNow(err.Error()) @@ -193,7 +193,7 @@ func TestCacheErrorLegacy(test *testing.T) { if err != nil { t.assert.FailNow(err.Error()) } - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.statsManager.NewStats("key"))} t.config.EXPECT().GetLimit(nil, "different-domain", req.Descriptors[0]).Return(limits[0]) t.cache.EXPECT().DoLimit(nil, req, limits).Do( func(context.Context, *pb.RateLimitRequest, []*config.RateLimit) { @@ -221,7 +221,7 @@ func TestInitialLoadErrorLegacy(test *testing.T) { func([]config.RateLimitConfigToLoad, stats.Manager) { panic(config.RateLimitConfigError("load error")) }) - service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.sm, true) + service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.statsManager, true) request := common.NewRateLimitRequestLegacy("test-domain", [][][2]string{{{"hello", "world"}}}, 1) response, err := service.GetLegacyService().ShouldRateLimit(nil, request) diff --git a/test/service/ratelimit_test.go b/test/service/ratelimit_test.go index 141480eb..fef51426 100644 --- a/test/service/ratelimit_test.go +++ b/test/service/ratelimit_test.go @@ -57,7 +57,7 @@ type rateLimitServiceTestSuite struct { configLoader *mock_config.MockRateLimitConfigLoader config *mock_config.MockRateLimitConfig runtimeUpdateCallback chan<- int - sm stats.Manager + statsManager stats.Manager statStore gostats.Store } @@ -71,7 +71,7 @@ func commonSetup(t *testing.T) rateLimitServiceTestSuite { ret.configLoader = mock_config.NewMockRateLimitConfigLoader(ret.controller) ret.config = mock_config.NewMockRateLimitConfig(ret.controller) ret.statStore = gostats.NewStore(gostats.NewNullSink(), false) - ret.sm = mock_stats.NewMockStatManager(ret.statStore) + ret.statsManager = mock_stats.NewMockStatManager(ret.statStore) return ret } @@ -86,7 +86,7 @@ func (this *rateLimitServiceTestSuite) setupBasicService() ratelimit.RateLimitSe this.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Return(this.config) - return ratelimit.NewService(this.runtime, this.cache, this.configLoader, this.sm, true) + return ratelimit.NewService(this.runtime, this.cache, this.configLoader, this.statsManager, true) } func TestService(test *testing.T) { @@ -121,7 +121,7 @@ func TestService(test *testing.T) { request = common.NewRateLimitRequest( "different-domain", [][][2]string{{{"foo", "bar"}}, {{"hello", "world"}}}, 1) limits := []*config.RateLimit{ - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key")), + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.statsManager.NewStats("key")), nil} t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[0]).Return(limits[0]) t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[1]).Return(limits[1]) @@ -153,7 +153,7 @@ func TestService(test *testing.T) { // Config should still be valid. Also make sure order does not affect results. limits = []*config.RateLimit{ nil, - config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} + config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.statsManager.NewStats("key"))} t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[0]).Return(limits[0]) t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[1]).Return(limits[1]) t.cache.EXPECT().DoLimit(nil, request, limits).Return( @@ -205,7 +205,7 @@ func TestCacheError(test *testing.T) { service := t.setupBasicService() request := common.NewRateLimitRequest("different-domain", [][][2]string{{{"foo", "bar"}}}, 1) - limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.sm.NewStats("key"))} + limits := []*config.RateLimit{config.NewRateLimit(10, pb.RateLimitResponse_RateLimit_MINUTE, t.statsManager.NewStats("key"))} t.config.EXPECT().GetLimit(nil, "different-domain", request.Descriptors[0]).Return(limits[0]) t.cache.EXPECT().DoLimit(nil, request, limits).Do( func(context.Context, *pb.RateLimitRequest, []*config.RateLimit) { @@ -232,7 +232,7 @@ func TestInitialLoadError(test *testing.T) { func([]config.RateLimitConfigToLoad, stats.Manager) { panic(config.RateLimitConfigError("load error")) }) - service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.sm, true) + service := ratelimit.NewService(t.runtime, t.cache, t.configLoader, t.statsManager, true) request := common.NewRateLimitRequest("test-domain", [][][2]string{{{"hello", "world"}}}, 1) response, err := service.ShouldRateLimit(nil, request) From c4cbbed092cb21f0d34978c857b30a1e371ff978 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 11:49:09 -0300 Subject: [PATCH 08/18] Inline variables Signed-off-by: Pablo Radnic --- src/config_check_cmd/main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/config_check_cmd/main.go b/src/config_check_cmd/main.go index b9c59608..e451694c 100644 --- a/src/config_check_cmd/main.go +++ b/src/config_check_cmd/main.go @@ -21,8 +21,7 @@ func loadConfigs(allConfigs []config.RateLimitConfigToLoad) { os.Exit(1) } }() - settingStruct := settings.NewSettings() - statsManager := stats.NewStatManager(gostats.NewStore(gostats.NewNullSink(), false), settingStruct) + statsManager := stats.NewStatManager(gostats.NewStore(gostats.NewNullSink(), false), settings.NewSettings()) config.NewRateLimitConfigImpl(allConfigs, statsManager) } From 38649fdb499d97c7cf4b097a155610dbc661a6ed Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 12:00:59 -0300 Subject: [PATCH 09/18] Rename String() to GetKey() Signed-off-by: Pablo Radnic --- src/config/config_impl.go | 2 +- src/stats/manager_impl.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config/config_impl.go b/src/config/config_impl.go index 552f1476..e1951b61 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -61,7 +61,7 @@ var validKeys = map[string]bool{ func NewRateLimit( requestsPerUnit uint32, unit pb.RateLimitResponse_RateLimit_Unit, rlStats stats.RateLimitStats) *RateLimit { - return &RateLimit{FullKey: rlStats.String(), Stats: rlStats, Limit: &pb.RateLimitResponse_RateLimit{RequestsPerUnit: requestsPerUnit, Unit: unit}} + return &RateLimit{FullKey: rlStats.GetKey(), Stats: rlStats, Limit: &pb.RateLimitResponse_RateLimit{RequestsPerUnit: requestsPerUnit, Unit: unit}} } // Dump an individual descriptor for debugging purposes. diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index c9f8f70c..c7cc4034 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -114,6 +114,6 @@ type RateLimitStats struct { WithinLimit gostats.Counter } -func (this RateLimitStats) String() string { +func (this RateLimitStats) GetKey() string { return this.Key } From 68139152043577757bb636fc5328dcc43d389535 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 12:10:21 -0300 Subject: [PATCH 10/18] Added shouldRateLimitScope to Manager Signed-off-by: Pablo Radnic --- src/stats/manager_impl.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index c7cc4034..9386bed7 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -7,14 +7,15 @@ import ( logger "github.com/sirupsen/logrus" ) -func NewStatManager(store gostats.Store, s settings.Settings) *ManagerImpl { - serviceScope := store.ScopeWithTags("ratelimit", s.ExtraTags).Scope("service") +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"), legacyStatsScope: serviceScope.Scope("call.should_rate_limit_legacy"), serviceStatsScope: serviceScope, detailedMetricsScope: serviceScope.Scope("rate_limit").Scope("detailed"), + shouldRateLimitScope: serviceScope.Scope("call.should_rate_limit"), } } @@ -25,6 +26,7 @@ type ManagerImpl struct { serviceStatsScope gostats.Scope detailedMetricsScope gostats.Scope detailed bool + shouldRateLimitScope gostats.Scope } func (this *ManagerImpl) GetStatsStore() gostats.Store { @@ -66,10 +68,9 @@ type ShouldRateLimitStats struct { } func (this *ManagerImpl) NewShouldRateLimitStats() ShouldRateLimitStats { - s := this.serviceStatsScope.Scope("call.should_rate_limit") ret := ShouldRateLimitStats{} - ret.RedisError = s.NewCounter("redis_error") - ret.ServiceError = s.NewCounter("service_error") + ret.RedisError = this.shouldRateLimitScope.NewCounter("redis_error") + ret.ServiceError = this.shouldRateLimitScope.NewCounter("service_error") return ret } From 9cf84471ec2f0e1913f5f26108fe74529d4de9bf Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 13:48:46 -0300 Subject: [PATCH 11/18] Move type definitions to manager.go Signed-off-by: Pablo Radnic --- src/stats/manager.go | 41 +++++++++++++++++++++++++++++++++++++++ src/stats/manager_impl.go | 40 -------------------------------------- 2 files changed, 41 insertions(+), 40 deletions(-) diff --git a/src/stats/manager.go b/src/stats/manager.go index 2b89ecf9..e2e62804 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -1,6 +1,7 @@ package stats import stats "github.com/lyft/gostats" +import gostats "github.com/lyft/gostats" type Manager interface { NewStats(key string) RateLimitStats @@ -9,3 +10,43 @@ type Manager interface { NewShouldRateLimitLegacyStats() ShouldRateLimitLegacyStats GetStatsStore() stats.Store } + +type ManagerImpl struct { + store gostats.Store + rlStatsScope gostats.Scope + legacyStatsScope gostats.Scope + serviceStatsScope gostats.Scope + detailedMetricsScope gostats.Scope + detailed bool + shouldRateLimitScope gostats.Scope +} + +type ShouldRateLimitStats struct { + RedisError gostats.Counter + ServiceError gostats.Counter +} + +type ServiceStats struct { + ConfigLoadSuccess gostats.Counter + ConfigLoadError gostats.Counter + ShouldRateLimit ShouldRateLimitStats +} + +type ShouldRateLimitLegacyStats struct { + ReqConversionError gostats.Counter + RespConversionError gostats.Counter + ShouldRateLimitError gostats.Counter +} + +// Stats for an individual rate limit config entry. +//todo: Ideally the gostats package fields should be unexported +// the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 +// This ensures that setters such as Inc() and Add() can only be managed by ManagerImpl. +type RateLimitStats struct { + Key string + TotalHits gostats.Counter + OverLimit gostats.Counter + NearLimit gostats.Counter + OverLimitWithLocalCache gostats.Counter + WithinLimit gostats.Counter +} \ No newline at end of file diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index 9386bed7..d69b70ae 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -19,16 +19,6 @@ func NewStatManager(store gostats.Store, settings settings.Settings) *ManagerImp } } -type ManagerImpl struct { - store gostats.Store - rlStatsScope gostats.Scope - legacyStatsScope gostats.Scope - serviceStatsScope gostats.Scope - detailedMetricsScope gostats.Scope - detailed bool - shouldRateLimitScope gostats.Scope -} - func (this *ManagerImpl) GetStatsStore() gostats.Store { return this.store } @@ -48,12 +38,6 @@ func (this *ManagerImpl) NewStats(key string) RateLimitStats { return ret } -type ShouldRateLimitLegacyStats struct { - ReqConversionError gostats.Counter - RespConversionError gostats.Counter - ShouldRateLimitError gostats.Counter -} - func (this *ManagerImpl) NewShouldRateLimitLegacyStats() ShouldRateLimitLegacyStats { return ShouldRateLimitLegacyStats{ ReqConversionError: this.legacyStatsScope.NewCounter("req_conversion_error"), @@ -62,11 +46,6 @@ func (this *ManagerImpl) NewShouldRateLimitLegacyStats() ShouldRateLimitLegacySt } } -type ShouldRateLimitStats struct { - RedisError gostats.Counter - ServiceError gostats.Counter -} - func (this *ManagerImpl) NewShouldRateLimitStats() ShouldRateLimitStats { ret := ShouldRateLimitStats{} ret.RedisError = this.shouldRateLimitScope.NewCounter("redis_error") @@ -74,12 +53,6 @@ func (this *ManagerImpl) NewShouldRateLimitStats() ShouldRateLimitStats { return ret } -type ServiceStats struct { - ConfigLoadSuccess gostats.Counter - ConfigLoadError gostats.Counter - ShouldRateLimit ShouldRateLimitStats -} - func (this *ManagerImpl) NewServiceStats() ServiceStats { ret := ServiceStats{} ret.ConfigLoadSuccess = this.serviceStatsScope.NewCounter("config_load_success") @@ -102,19 +75,6 @@ func DescriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) str return domain + "." + rateLimitKey } -// Stats for an individual rate limit config entry. -//todo: Ideally the gostats package fields should be unexported -// the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 -// This ensures that setters such as Inc() and Add() can only be managed by ManagerImpl. -type RateLimitStats struct { - Key string - TotalHits gostats.Counter - OverLimit gostats.Counter - NearLimit gostats.Counter - OverLimitWithLocalCache gostats.Counter - WithinLimit gostats.Counter -} - func (this RateLimitStats) GetKey() string { return this.Key } From f3f214001d561241c89f47568f26dc57127cf939 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 13:56:35 -0300 Subject: [PATCH 12/18] Removed detailed metrics references Signed-off-by: Pablo Radnic --- src/stats/manager.go | 4 +--- src/stats/manager_impl.go | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/stats/manager.go b/src/stats/manager.go index e2e62804..dead43a5 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -16,8 +16,6 @@ type ManagerImpl struct { rlStatsScope gostats.Scope legacyStatsScope gostats.Scope serviceStatsScope gostats.Scope - detailedMetricsScope gostats.Scope - detailed bool shouldRateLimitScope gostats.Scope } @@ -41,7 +39,7 @@ type ShouldRateLimitLegacyStats struct { // Stats for an individual rate limit config entry. //todo: Ideally the gostats package fields should be unexported // the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 -// This ensures that setters such as Inc() and Add() can only be managed by ManagerImpl. +// This ensures that setters such as Inc() and Add() can only be managed by RateLimitStats. type RateLimitStats struct { Key string TotalHits gostats.Counter diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index d69b70ae..e516bbac 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -14,7 +14,6 @@ func NewStatManager(store gostats.Store, settings settings.Settings) *ManagerImp rlStatsScope: serviceScope.Scope("rate_limit"), legacyStatsScope: serviceScope.Scope("call.should_rate_limit_legacy"), serviceStatsScope: serviceScope, - detailedMetricsScope: serviceScope.Scope("rate_limit").Scope("detailed"), shouldRateLimitScope: serviceScope.Scope("call.should_rate_limit"), } } From cb39a2539bced1962e04edf46ad5b696389c292b Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 15:16:01 -0300 Subject: [PATCH 13/18] Relocate descriptorKey function to config package Signed-off-by: Pablo Radnic --- src/config/config_impl.go | 17 ++++++++++++++++- src/stats/manager_impl.go | 15 --------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/config/config_impl.go b/src/config/config_impl.go index e1951b61..22262ca9 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -235,7 +235,7 @@ func (this *rateLimitConfigImpl) GetLimit( } if descriptor.GetLimit() != nil { - rateLimitKey := stats.DescriptorKey(domain, descriptor) + rateLimitKey := descriptorKey(domain, descriptor) rateLimitOverrideUnit := pb.RateLimitResponse_RateLimit_Unit(descriptor.GetLimit().GetUnit()) rateLimit = NewRateLimit( descriptor.GetLimit().GetRequestsPerUnit(), @@ -277,6 +277,21 @@ func (this *rateLimitConfigImpl) GetLimit( return rateLimit } + +func descriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string { + rateLimitKey := "" + for _, entry := range descriptor.Entries { + if rateLimitKey != "" { + rateLimitKey += "." + } + rateLimitKey += entry.Key + if entry.Value != "" { + rateLimitKey += "_" + entry.Value + } + } + return domain + "." + rateLimitKey +} + // Create rate limit config from a list of input YAML files. // @param configs specifies a list of YAML files to load. // @param stats supplies the stats scope to use for limit stats during runtime. diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index e516bbac..48a01b1a 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -1,7 +1,6 @@ package stats import ( - pb_struct "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3" "github.com/envoyproxy/ratelimit/src/settings" gostats "github.com/lyft/gostats" logger "github.com/sirupsen/logrus" @@ -60,20 +59,6 @@ func (this *ManagerImpl) NewServiceStats() ServiceStats { return ret } -func DescriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string { - rateLimitKey := "" - for _, entry := range descriptor.Entries { - if rateLimitKey != "" { - rateLimitKey += "." - } - rateLimitKey += entry.Key - if entry.Value != "" { - rateLimitKey += "_" + entry.Value - } - } - return domain + "." + rateLimitKey -} - func (this RateLimitStats) GetKey() string { return this.Key } From 62b519e18f675cbe4381aa334ad1604e3f2669ea Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Mon, 19 Apr 2021 16:14:10 -0300 Subject: [PATCH 14/18] Added Documentation on Manager.go Signed-off-by: Pablo Radnic --- src/config/config_impl.go | 1 - src/stats/manager.go | 25 ++++++++++++++++++++----- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/config/config_impl.go b/src/config/config_impl.go index 22262ca9..7b48d326 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -277,7 +277,6 @@ func (this *rateLimitConfigImpl) GetLimit( return rateLimit } - func descriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string { rateLimitKey := "" for _, entry := range descriptor.Entries { diff --git a/src/stats/manager.go b/src/stats/manager.go index dead43a5..47b91186 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -3,11 +3,21 @@ package stats import stats "github.com/lyft/gostats" import gostats "github.com/lyft/gostats" +// Manager is the interface that wraps initialization of stat structures type Manager interface { - NewStats(key string) RateLimitStats + // NewStats provides a RateLimitStats structure associated with a given descriptorKey. + // Multiple calls with the same descriptorKey argument are guaranteed to be equivalent. + NewStats(descriptorKey string) RateLimitStats + // Initializes a ShouldRateLimitStats structure. + // Multiple calls to this method are idempotent NewShouldRateLimitStats() ShouldRateLimitStats + // Initializes a ServiceStats structure. + // Multiple calls to this method are idempotent NewServiceStats() ServiceStats + // Initializes a ShouldRateLimitLegacyStats structure. + // Multiple calls to this method are idempotent NewShouldRateLimitLegacyStats() ShouldRateLimitLegacyStats + // Returns the stats.Store wrapped by the Manager GetStatsStore() stats.Store } @@ -19,17 +29,22 @@ type ManagerImpl struct { shouldRateLimitScope gostats.Scope } +// Stats for panic recoveries. +// Identifies if a recovered panic is a redis.RedisError or a ServiceError. type ShouldRateLimitStats struct { RedisError gostats.Counter ServiceError gostats.Counter } +// Stats for server errors. +// Keeps failure and success metrics. type ServiceStats struct { ConfigLoadSuccess gostats.Counter ConfigLoadError gostats.Counter ShouldRateLimit ShouldRateLimitStats } +// Legacy Stats for ratelimit errors. type ShouldRateLimitLegacyStats struct { ReqConversionError gostats.Counter RespConversionError gostats.Counter @@ -37,9 +52,9 @@ type ShouldRateLimitLegacyStats struct { } // Stats for an individual rate limit config entry. -//todo: Ideally the gostats package fields should be unexported -// the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 -// This ensures that setters such as Inc() and Add() can only be managed by RateLimitStats. +// todo: Ideally the gostats package fields should be unexported +// the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 +// This ensures that setters such as Inc() and Add() can only be managed by RateLimitStats. type RateLimitStats struct { Key string TotalHits gostats.Counter @@ -47,4 +62,4 @@ type RateLimitStats struct { NearLimit gostats.Counter OverLimitWithLocalCache gostats.Counter WithinLimit gostats.Counter -} \ No newline at end of file +} From 56c3e52a6d2be77aeb3e611a81e27780ce9a3c10 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Tue, 20 Apr 2021 09:44:07 -0300 Subject: [PATCH 15/18] Added '.' at the end of manager.go documentation Signed-off-by: Pablo Radnic --- src/stats/manager.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/stats/manager.go b/src/stats/manager.go index 47b91186..2043a146 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -3,21 +3,21 @@ package stats import stats "github.com/lyft/gostats" import gostats "github.com/lyft/gostats" -// Manager is the interface that wraps initialization of stat structures +// Manager is the interface that wraps initialization of stat structures. type Manager interface { // NewStats provides a RateLimitStats structure associated with a given descriptorKey. // Multiple calls with the same descriptorKey argument are guaranteed to be equivalent. NewStats(descriptorKey string) RateLimitStats // Initializes a ShouldRateLimitStats structure. - // Multiple calls to this method are idempotent + // Multiple calls to this method are idempotent. NewShouldRateLimitStats() ShouldRateLimitStats // Initializes a ServiceStats structure. - // Multiple calls to this method are idempotent + // Multiple calls to this method are idempotent. NewServiceStats() ServiceStats // Initializes a ShouldRateLimitLegacyStats structure. - // Multiple calls to this method are idempotent + // Multiple calls to this method are idempotent. NewShouldRateLimitLegacyStats() ShouldRateLimitLegacyStats - // Returns the stats.Store wrapped by the Manager + // Returns the stats.Store wrapped by the Manager. GetStatsStore() stats.Store } From a3b6752c85a8721be9195b6b018c98bb18abf088 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Wed, 2 Jun 2021 11:41:14 -0300 Subject: [PATCH 16/18] Supply StatManager in memcache tests Signed-off-by: Pablo Radnic --- src/stats/manager.go | 3 --- test/memcached/cache_impl_test.go | 4 ++-- test/mocks/stats/manager.go | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/stats/manager.go b/src/stats/manager.go index 2043a146..a96753b7 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -52,9 +52,6 @@ type ShouldRateLimitLegacyStats struct { } // Stats for an individual rate limit config entry. -// todo: Ideally the gostats package fields should be unexported -// the inner value could be interacted with via getters such as rlStats.TotalHits() uint64 -// This ensures that setters such as Inc() and Add() can only be managed by RateLimitStats. type RateLimitStats struct { Key string TotalHits gostats.Counter diff --git a/test/memcached/cache_impl_test.go b/test/memcached/cache_impl_test.go index 0244d3bf..c9173e20 100644 --- a/test/memcached/cache_impl_test.go +++ b/test/memcached/cache_impl_test.go @@ -605,7 +605,7 @@ func TestNewRateLimitCacheImplFromSettingsWhenSrvCannotBeResolved(t *testing.T) s.ExpirationJitterMaxSeconds = 300 s.MemcacheSrv = "_something._tcp.example.invalid" - assert.Panics(func() { memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore) }) + assert.Panics(func() { memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore, mockstats.NewMockStatManager(statsStore)) }) } func TestNewRateLimitCacheImplFromSettingsWhenHostAndPortAndSrvAreBothSet(t *testing.T) { @@ -623,7 +623,7 @@ func TestNewRateLimitCacheImplFromSettingsWhenHostAndPortAndSrvAreBothSet(t *tes s.MemcacheSrv = "_something._tcp.example.invalid" s.MemcacheHostPort = []string{"example.org:11211"} - assert.Panics(func() { memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore) }) + assert.Panics(func() { memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore, mockstats.NewMockStatManager(statsStore)) }) } func getMultiResult(vals map[string]int) map[string]*memcache.Item { diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go index f4d5e9ee..52282e72 100644 --- a/test/mocks/stats/manager.go +++ b/test/mocks/stats/manager.go @@ -39,7 +39,6 @@ func (m *MockStatManager) NewShouldRateLimitLegacyStats() stats.ShouldRateLimitL } } -//todo: review mock implementation func (m *MockStatManager) NewStats(key string) stats.RateLimitStats { ret := stats.RateLimitStats{} logger.Debugf("outputing test gostats %s", key) From feaa224dce23576b0fbc195d655f4974cf6da457 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Wed, 2 Jun 2021 13:09:33 -0300 Subject: [PATCH 17/18] Format fix Signed-off-by: Pablo Radnic --- test/memcached/cache_impl_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/memcached/cache_impl_test.go b/test/memcached/cache_impl_test.go index c9173e20..d663f675 100644 --- a/test/memcached/cache_impl_test.go +++ b/test/memcached/cache_impl_test.go @@ -605,7 +605,9 @@ func TestNewRateLimitCacheImplFromSettingsWhenSrvCannotBeResolved(t *testing.T) s.ExpirationJitterMaxSeconds = 300 s.MemcacheSrv = "_something._tcp.example.invalid" - assert.Panics(func() { memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore, mockstats.NewMockStatManager(statsStore)) }) + assert.Panics(func() { + memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore, mockstats.NewMockStatManager(statsStore)) + }) } func TestNewRateLimitCacheImplFromSettingsWhenHostAndPortAndSrvAreBothSet(t *testing.T) { @@ -623,7 +625,9 @@ func TestNewRateLimitCacheImplFromSettingsWhenHostAndPortAndSrvAreBothSet(t *tes s.MemcacheSrv = "_something._tcp.example.invalid" s.MemcacheHostPort = []string{"example.org:11211"} - assert.Panics(func() { memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore, mockstats.NewMockStatManager(statsStore)) }) + assert.Panics(func() { + memcached.NewRateLimitCacheImplFromSettings(s, timeSource, nil, nil, statsStore, mockstats.NewMockStatManager(statsStore)) + }) } func getMultiResult(vals map[string]int) map[string]*memcache.Item { From 2d88c77974c92e8b9aa501e7fe3f24bff64fec24 Mon Sep 17 00:00:00 2001 From: Pablo Radnic Date: Wed, 2 Jun 2021 15:27:07 -0300 Subject: [PATCH 18/18] Fix flaky test TestService Signed-off-by: Pablo Radnic --- test/service/ratelimit_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/service/ratelimit_test.go b/test/service/ratelimit_test.go index fef51426..787fbf2b 100644 --- a/test/service/ratelimit_test.go +++ b/test/service/ratelimit_test.go @@ -144,7 +144,7 @@ func TestService(test *testing.T) { t.configLoader.EXPECT().Load( []config.RateLimitConfigToLoad{{"config.basic_config", "fake_yaml"}}, gomock.Any()).Do( func([]config.RateLimitConfigToLoad, stats.Manager) { - barrier.signal() + defer barrier.signal() panic(config.RateLimitConfigError("load error")) }) t.runtimeUpdateCallback <- 1