Metric refactor #242

pradnic · 2021-04-09T14:52:45Z

As requested here.

In the current version of the system metric production is done ad hoc to comply with specific requirements, Scopes are initialized in service components, Ex: 1, 2, 3; and metrics are directly set by all components as well Ex: 1, 2.

This makes adding service-wide configurations to metric generation (such as Detailed Metrics) particularly difficult. In order to accommodate this feature, we made the following refactor.

This PR refactors metric handling in the ratelimit service, namely:

Adds stats.Manager structure.
Centralizes metric creation in the stats.Manager.
Removes, where possible, metric scope handling from the other service's components.

Signed-off-by: Pablo Radnic <[email protected]>

mattklein123 · 2021-04-12T22:28:11Z

cc @nezdolik @ysawa0 would either of you be willing to help review this?

nezdolik · 2021-04-13T13:25:26Z

@mattklein123 I can help review this Friday

nezdolik

Does extending existing settings with option to include detailed stats come in separate PR?

nezdolik · 2021-04-16T09:46:07Z

src/config/config.go

@@ -56,5 +47,5 @@ type RateLimitConfigLoader interface {
 	// @param statsScope supplies the stats scope to use for limit stats during runtime.


nit: update function documentation with new parameter

nezdolik · 2021-04-16T12:08:10Z

src/config/config_impl.go

-	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) {


nit: rename arg name manager ->statsManager

nezdolik · 2021-04-16T12:12:15Z

src/config/config_impl.go

@@ -268,13 +235,12 @@ func (this *rateLimitConfigImpl) GetLimit(
 	}

 	if descriptor.GetLimit() != nil {
-		rateLimitKey := domain + "." + this.descriptorToKey(descriptor)
+		rateLimitKey := stats.DescriptorKey(domain, descriptor)


what is the reasoning behind moving descriptorToKey function to stats package?

Moved to client package (where it is used currently).

nezdolik · 2021-04-16T12:13:03Z

src/config/config_impl.go

@@ -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 {


nit: same as above, rename manager to smth like statsManager

nezdolik · 2021-04-16T12:16:33Z

src/config_check_cmd/main.go

-	dummyStats := stats.NewStore(stats.NewNullSink(), false)
-	config.NewRateLimitConfigImpl(allConfigs, dummyStats)
+	settingStruct := settings.NewSettings()
+	manager := stats.NewStatManager(gostats.NewStore(gostats.NewNullSink(), false), settingStruct)


nit: inline creation of settings here instead

nezdolik · 2021-04-16T12:34:30Z

src/stats/manager_impl.go

+	logger "github.com/sirupsen/logrus"
+)
+
+func NewStatManager(store gostats.Store, s settings.Settings) *ManagerImpl {


renaming s settings would probably clash with settings package, but it could have improved readability

nezdolik · 2021-04-16T12:35:59Z

src/stats/manager_impl.go

+}
+
+func (this *ManagerImpl) NewShouldRateLimitStats() ShouldRateLimitStats {
+	s := this.serviceStatsScope.Scope("call.should_rate_limit")


nit: s-> scope

nezdolik · 2021-04-16T12:38:41Z

src/stats/manager_impl.go

+//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 {


could be better to group all those struct definitions together either in top of this file or pull them out into manager.go file

nezdolik · 2021-04-16T12:43:12Z

src/stats/manager_impl.go

+		rlStatsScope:         serviceScope.Scope("rate_limit"),
+		legacyStatsScope:     serviceScope.Scope("call.should_rate_limit_legacy"),
+		serviceStatsScope:    serviceScope,
+		detailedMetricsScope: serviceScope.Scope("rate_limit").Scope("detailed"),


should this scope be initialised only if detailed is set to true?

Removed all mentions of detailedMetrics they will be added on the DetailedMetrics PR

nezdolik · 2021-04-16T12:43:25Z

src/stats/manager_impl.go

+	legacyStatsScope     gostats.Scope
+	serviceStatsScope    gostats.Scope
+	detailedMetricsScope gostats.Scope
+	detailed             bool


how is this field currently used?

Removed all mentions of detailedMetrics they will be added on the DetailedMetrics PR

Signed-off-by: Pablo Radnic <[email protected]>

pradnic · 2021-04-19T19:32:58Z

Thank you @nezdolik for the review.

Finished working on the requested changes. Alongside the nit's these are the most important points:

Renamed stats.Manager variables to statsManager in order to improve clarity.
Added documentation on manager.go types and methods.
Moved type definitions from manager_impl.go to manager.go

nezdolik

PR looks good to me. @mattklein123 @dweitzman would you take a look?

nezdolik · 2021-04-20T08:38:30Z

src/stats/manager.go

+import stats "github.com/lyft/gostats"
+import gostats "github.com/lyft/gostats"
+
+// Manager is the interface that wraps initialization of stat structures


nit: add '.' in the end of each doc sentence

Signed-off-by: Pablo Radnic <[email protected]>

pradnic · 2021-04-27T14:31:32Z

Hi @mattklein123 let me know if we need any more work on this PR or if we can go forwards with the next steps in order to add detailed-metrics feature.

pradnic · 2021-05-26T15:31:56Z

Hi @mattklein123 what are the next steps you suggest in order to get this feature merged in upstream?

mattklein123 · 2021-05-26T15:33:11Z

Merge main, and then I will take a look.

/wait

Signed-off-by: Pablo Radnic <[email protected]>

mattklein123 · 2021-06-02T15:28:57Z

Please check CI

/wait

Signed-off-by: Pablo Radnic <[email protected]>

pradnic · 2021-06-02T18:30:33Z

test/service/ratelimit_test.go

-		func([]config.RateLimitConfigToLoad, stats.Scope) {
-			barrier.signal()
+		func([]config.RateLimitConfigToLoad, stats.Manager) {
+			defer barrier.signal()


@mattklein123 I believe the CI build failed because of a race-condition in this test. The scenario is similar to the one fixed in this recently merged PR.
I believe the issue should be resolved with this change.

mattklein123

Thanks! Just to make sure I understand, there should be no functional change for this PR, right? In a future PR you will add the ability to have detailed stats?

/wait-any

pradnic · 2021-06-02T19:32:38Z

Thanks! Just to make sure I understand, there should be no functional change for this PR, right? In a future PR you will add the ability to have detailed stats?

Correct! This changeset holds no functional changes. The opt-in Detailed Metrics feature will come in the next PR.

dweitzman · 2021-06-12T23:27:52Z

@dweitzman would you take a look?

I didn't read all the code, but the overall goal makes sense to me. I'm in favor of anything that isolates the github.com/lyft/gostats imports so they can more easily be wrapped in or replaced by a more modern, flexible, or expressive way to tracking metrics.

Signed-off-by: Pablo Radnic <[email protected]>

pradnic added 7 commits March 17, 2021 16:40

Refactor metric handling

21f6348

Signed-off-by: Pablo Radnic <[email protected]>

Revert config_test import renaming

ed9a52c

Signed-off-by: Pablo Radnic <[email protected]>

Merge upstream changes

6465cf3

Signed-off-by: Pablo Radnic <[email protected]>

Unwrap ratelimit metric addition

92758a1

Signed-off-by: Pablo Radnic <[email protected]>

Remove setter methods

4fd402c

Signed-off-by: Pablo Radnic <[email protected]>

rename variable

51bf9d8

Signed-off-by: Pablo Radnic <[email protected]>

Rename stats variable

fceb8fd

Signed-off-by: Pablo Radnic <[email protected]>

mattklein123 self-assigned this Apr 9, 2021

mattklein123 assigned nezdolik Apr 13, 2021

nezdolik reviewed Apr 16, 2021

View reviewed changes

pradnic added 8 commits April 19, 2021 11:46

Rename manager variable to statsManager

c846e96

Signed-off-by: Pablo Radnic <[email protected]>

Inline variables

c4cbbed

Signed-off-by: Pablo Radnic <[email protected]>

Rename String() to GetKey()

38649fd

Signed-off-by: Pablo Radnic <[email protected]>

Added shouldRateLimitScope to Manager

6813915

Signed-off-by: Pablo Radnic <[email protected]>

Move type definitions to manager.go

9cf8447

Signed-off-by: Pablo Radnic <[email protected]>

Removed detailed metrics references

f3f2140

Signed-off-by: Pablo Radnic <[email protected]>

Relocate descriptorKey function to config package

cb39a25

Signed-off-by: Pablo Radnic <[email protected]>

Added Documentation on Manager.go

62b519e

Signed-off-by: Pablo Radnic <[email protected]>

nezdolik reviewed Apr 20, 2021

View reviewed changes

Added '.' at the end of manager.go documentation

56c3e52

Signed-off-by: Pablo Radnic <[email protected]>

pradnic force-pushed the metric-refactor branch from c04cf0c to 56c3e52 Compare April 20, 2021 12:53

nezdolik previously approved these changes Apr 23, 2021

View reviewed changes

mattklein123 added the no stalebot label Apr 27, 2021

mattklein123 removed the no stalebot label May 25, 2021

repokitteh-read-only bot added the waiting label May 26, 2021

pradnic added 2 commits June 2, 2021 11:31

Merge remote-tracking branch 'upstream/main' into metric-refactor

d03eba2

Signed-off-by: Pablo Radnic <[email protected]>

Supply StatManager in memcache tests

a3b6752

Signed-off-by: Pablo Radnic <[email protected]>

pradnic dismissed nezdolik’s stale review via a3b6752 June 2, 2021 14:41

repokitteh-read-only bot removed the waiting label Jun 2, 2021

repokitteh-read-only bot added waiting and removed waiting labels Jun 2, 2021

Format fix

feaa224

Signed-off-by: Pablo Radnic <[email protected]>

pradnic force-pushed the metric-refactor branch from 70f3800 to feaa224 Compare June 2, 2021 16:13

Fix flaky test TestService

2d88c77

Signed-off-by: Pablo Radnic <[email protected]>

pradnic commented Jun 2, 2021

View reviewed changes

mattklein123 approved these changes Jun 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Jun 2, 2021

repokitteh-read-only bot removed the waiting:any label Jun 2, 2021

mattklein123 merged commit 6aaad7c into envoyproxy:main Jun 2, 2021

zdmytriv pushed a commit to verygoodsecurity/ratelimit that referenced this pull request Aug 2, 2021

Metric refactor (envoyproxy#242)

b7d9cfd

Signed-off-by: Pablo Radnic <[email protected]>

mattklein123 mentioned this pull request Aug 3, 2021

Memory leak when RateLimitOverride is set with uniq ip on every call to ShouldRateLimit method #275

Open

jdamata mentioned this pull request Mar 18, 2022

Add detailed metrics #323

Closed

timcovar pushed a commit to goatapp/ratelimit that referenced this pull request Jan 16, 2024

Metric refactor (envoyproxy#242)

849bd51

Signed-off-by: Pablo Radnic <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metric refactor #242

Metric refactor #242

pradnic commented Apr 9, 2021

mattklein123 commented Apr 12, 2021

nezdolik commented Apr 13, 2021

nezdolik left a comment

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

pradnic Apr 19, 2021

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

nezdolik Apr 16, 2021

pradnic Apr 19, 2021

nezdolik Apr 16, 2021

pradnic Apr 19, 2021

pradnic commented Apr 19, 2021

nezdolik left a comment

nezdolik Apr 20, 2021

pradnic commented Apr 27, 2021

pradnic commented May 26, 2021

mattklein123 commented May 26, 2021

mattklein123 commented Jun 2, 2021

pradnic Jun 2, 2021

mattklein123 left a comment

pradnic commented Jun 2, 2021

dweitzman commented Jun 12, 2021

		@@ -56,5 +47,5 @@ type RateLimitConfigLoader interface {
		// @param statsScope supplies the stats scope to use for limit stats during runtime.

Metric refactor #242

Metric refactor #242

Conversation

pradnic commented Apr 9, 2021

mattklein123 commented Apr 12, 2021

nezdolik commented Apr 13, 2021

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradnic commented Apr 19, 2021

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradnic commented Apr 27, 2021

pradnic commented May 26, 2021

mattklein123 commented May 26, 2021

mattklein123 commented Jun 2, 2021

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

pradnic commented Jun 2, 2021

dweitzman commented Jun 12, 2021