feat: support for adaptive rate limiting [PIPE-481] #4160

BonapartePC · 2023-11-20T08:59:37Z

Description

Support for adaptive rate limiting. More info: https://www.notion.so/rudderstacks/Adaptive-Rate-Limiting-dfc7c99abb7e4a4c9a89bf8cef6141bd

In this PR, I have implemented the Approach 1

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Summary by CodeRabbit

New Features
- Introduced adaptive throttling with real-time adjustments based on system feedback.
- Added a new throttler factory system for more flexible throttler initialization and management.
Improvements
- Enhanced throttling configuration with support for dynamic limit adjustments.
- Improved system resilience by adding shutdown procedures for throttler components.
Testing
- Implemented comprehensive tests for adaptive throttling behavior and configuration.
Documentation
- Updated comments and documentation to reflect changes in throttling mechanisms and configurations.
Refactor
- Refactored throttler initialization to use factories instead of direct instance creation.
- Changed several throttler-related structures to improve performance and maintainability.
Bug Fixes
- Fixed issues with throttler limit calculations and response code handling.

codecov · 2023-11-20T11:38:51Z

Codecov Report

Attention: 54 lines in your changes are missing coverage. Please review.

Comparison is base (7ff4103) 72.61% compared to head (3b1d077) 72.62%.
Report is 5 commits behind head on master.

Files	Patch %	Lines
router/throttler/factory.go	75.21%	24 Missing and 5 partials ⚠️
router/throttler/adaptive.go	57.50%	16 Missing and 1 partial ⚠️
...er/throttler/adaptivethrottlercounter/algorithm.go	94.54%	2 Missing and 1 partial ⚠️
router/throttler/static.go	88.88%	2 Missing and 1 partial ⚠️
app/apphandlers/processorAppHandler.go	0.00%	1 Missing ⚠️
router/handle.go	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4160      +/-   ##
==========================================
+ Coverage   72.61%   72.62%   +0.01%     
==========================================
  Files         387      397      +10     
  Lines       56479    56395      -84     
==========================================
- Hits        41012    40959      -53     
+ Misses      13098    13073      -25     
+ Partials     2369     2363       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

router/throttler/adaptiveRateLimit.go

router/throttler/throttler.go

router/throttler/adaptiveRateLimit.go

router/throttler/adaptiveAlgorithmCounter/algorithm.go

router/throttler/adaptive.go

router/throttler/adaptiveAlgorithmCounter/algorithm.go

router/throttler/config.go

router/throttler/algorithm.go

router/throttler/default.go

lvrach · 2023-11-28T09:23:45Z

router/throttler/factory.go

+type Factory interface {
+	Get(destName, destID string) Throttler
+	Shutdown()
+}
+
+// NewFactory constructs a new Throttler Factory
+func NewFactory(config *config.Config, stats stats.Stats) (Factory, error) {


Why do we return a Factory interface instead of a concrete implementation Factory struct?

we can create a factory interface in router package if needed

I am returning the interface here to limit the scope of the consumer.
cc @atzoum

@atzoum can you elaborate, it seems like an anti-pattern to me

You are asking me to comment on a debatable topic :)
My view is that there are valid reasons for choosing to return interfaces instead of concrete implementations, e.g.:

Hiding implementation details from callers (no need to worry about misuse of erroneously exported fields in structs)

Augmenting/decorating behaviour at runtime

Succinct documentation of the provided API (methods) to callers

The above doesn't mean that the callers need to re-use the interface that is being returned as is, they can declare their own (scoped) interfaces if needed.

router/throttler/algorithm.go

router/throttler/config.go

router/throttler/factory.go

router/throttler/factory_test.go

router/handle.go

router/handle_lifecycle.go

router/throttler/adaptiveAlgorithmCounter/algorithm.go

lvrach · 2023-12-05T14:39:28Z

router/throttler/switcher.go

@@ -0,0 +1,37 @@
+package throttler


@atzoum Is it required to hot reload between the two implemantioans?

When we the switch happens, the previous throttles will keep running in the background:
a. the use of memory and CPU resources
b. they stop getting updates on what is going on, which might lead to unknown/unpredictable behaviour if we switch back to them.

From my perspective, the probability of switching between those implementations will be low and restarting the processer is not a big issue. Given the additional implementation and risk for switching between implementations.

I would suggest removing this package, and resorting to restarts if needed.

I don't see that much of a risk & the ability to switch without a restart is a blessing, especially if we need to do it in bulk.

a. CPU and memory overhead shouldn't be a consideration, GCRA requirements are minimal, let alone when it's not being used.
b. Both can be updated during ResponseCodeReceived to keep them up-to-date.

router/throttler/adaptivethrottlercounter/algorithm.go

router/throttler/switcher.go

atzoum · 2023-12-06T09:48:06Z

router/throttler/switcher.go

@@ -0,0 +1,37 @@
+package throttler


I don't see that much of a risk & the ability to switch without a restart is a blessing, especially if we need to do it in bulk.

a. CPU and memory overhead shouldn't be a consideration, GCRA requirements are minimal, let alone when it's not being used.
b. Both can be updated during ResponseCodeReceived to keep them up-to-date.

router/throttler/adaptive.go

atzoum · 2023-12-06T10:04:20Z

router/throttler/factory.go

+type Factory interface {
+	Get(destName, destID string) Throttler
+	Shutdown()
+}
+
+// NewFactory constructs a new Throttler Factory
+func NewFactory(config *config.Config, stats stats.Stats) (Factory, error) {


You are asking me to comment on a debatable topic :)
My view is that there are valid reasons for choosing to return interfaces instead of concrete implementations, e.g.:

Hiding implementation details from callers (no need to worry about misuse of erroneously exported fields in structs)

Augmenting/decorating behaviour at runtime

Succinct documentation of the provided API (methods) to callers

The above doesn't mean that the callers need to re-use the interface that is being returned as is, they can declare their own (scoped) interfaces if needed.

atzoum

Some last comments

atzoum · 2023-12-06T13:56:36Z

router/throttler/adaptive.go

+
+func (t *adaptiveThrottler) getLimit() int64 {
+	limitFactor := t.algorithm.LimitFactor()
+	defer t.limitFactorMeasurement.Gauge(limitFactor)


Suggested change

defer t.limitFactorMeasurement.Gauge(limitFactor)

t.limitFactorMeasurement.Gauge(limitFactor)

atzoum · 2023-12-06T13:57:58Z

router/throttler/factory.go

+			"destination_id":   destID,
+			"destination_name": destName,


Suggested change

"destination_id": destID,

"destination_name": destName,

"destinationId": destID,

"destType": destName,

atzoum · 2023-12-06T13:58:31Z

router/throttler/factory.go

+				"destination_id":   destID,
+				"destination_name": destName,


I believe these are the labels we are using

Suggested change

"destination_id": destID,

"destination_name": destName,

"destinationId": destID,

"destType": destName,

atzoum · 2023-12-06T13:58:49Z

router/throttler/factory.go

+			tags := stats.Tags{
+				"destination_id":   destID,
+				"destination_name": destName,
+				"adaptive_enabled": fmt.Sprintf("%t", f.throttlers[destID].(*switchingThrottler).adaptiveEnabled.Load()),


Suggested change

"adaptive_enabled": fmt.Sprintf("%t", f.throttlers[destID].(*switchingThrottler).adaptiveEnabled.Load()),

"adaptive": fmt.Sprintf("%t", f.throttlers[destID].(*switchingThrottler).adaptiveEnabled.Load()),

feat: support for adaptive rate limiting

e4dffcc

github-actions bot added the server-team label Nov 20, 2023

BonapartePC marked this pull request as draft November 20, 2023 09:01

BonapartePC marked this pull request as ready for review November 20, 2023 09:11

BonapartePC marked this pull request as draft November 20, 2023 09:12

BonapartePC added 2 commits November 20, 2023 15:27

change defaults

9f62f71

fix lint

d85cc43

add tests

2916ba2

github-actions bot added the with tests label Nov 20, 2023

BonapartePC marked this pull request as ready for review November 20, 2023 13:32

BonapartePC requested review from atzoum, lvrach and cisse21 November 20, 2023 13:58

lvrach requested changes Nov 21, 2023

View reviewed changes

address few comments

1204bb0

BonapartePC force-pushed the feat.adaptiveRateLimiting branch from 44e1790 to 1204bb0 Compare November 21, 2023 14:28

address review comments

d8302b1

BonapartePC force-pushed the feat.adaptiveRateLimiting branch from 1c303d6 to d8302b1 Compare November 23, 2023 15:46

BonapartePC and others added 2 commits November 23, 2023 21:16

Merge branch 'master' into feat.adaptiveRateLimiting

9154c43

add nil checks

5afbf5e

BonapartePC force-pushed the feat.adaptiveRateLimiting branch from 1422c5e to 5afbf5e Compare November 23, 2023 16:19

BonapartePC requested a review from lvrach November 23, 2023 16:35

modify tests

3fd54f2

mihir20 reviewed Nov 28, 2023

View reviewed changes

Merge branch 'master' into feat.adaptiveRateLimiting

97d82df

This comment was marked as resolved.

Sign in to view

This comment was marked as spam.

Sign in to view

lvrach reviewed Nov 28, 2023

View reviewed changes

atzoum reviewed Nov 28, 2023

View reviewed changes

achettyiitr self-requested a review December 4, 2023 15:17

rudderlabs deleted a comment from coderabbitai bot Dec 4, 2023

lvrach reviewed Dec 5, 2023

View reviewed changes

refactor code (#4200)

a30df71

BonapartePC force-pushed the feat.adaptiveRateLimiting branch from 03c59ad to a30df71 Compare December 5, 2023 16:59

Merge branch 'master' into feat.adaptiveRateLimiting

0a900a9

rudderlabs deleted a comment from coderabbitai bot Dec 6, 2023

atzoum reviewed Dec 6, 2023

View reviewed changes

add stat for limit factor and send response code to static and adaptive

7b4cc73

BonapartePC requested review from lvrach and atzoum December 6, 2023 13:54

atzoum approved these changes Dec 6, 2023

View reviewed changes

address comments

3b1d077

lvrach approved these changes Dec 6, 2023

View reviewed changes

BonapartePC merged commit d711148 into master Dec 6, 2023
41 checks passed

lvrach mentioned this pull request Dec 7, 2023

chore: merge release 1.18.0 in main branch #4210

Merged

rudderlabs deleted a comment from coderabbitai bot Dec 7, 2023

This was referenced Jan 8, 2024

chore: prerelease 1.19.0-rc.1 #4289

Closed

chore: prerelease 1.19.0-rc.1 #4293

Merged

chore: prerelease 1.19.0-rc-2 #4298

Merged

achettyiitr deleted the feat.adaptiveRateLimiting branch January 12, 2024 05:59

This was referenced Jan 15, 2024

chore: prerelease 1.19.0-rc.3 #4314

Merged

chore: release 1.19.0 #4316

Merged

This was referenced Feb 9, 2024

chore: release 1.19.0 #4382

Closed

chore: prerelease 1.20.0-rc.1 #4383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for adaptive rate limiting [PIPE-481] #4160

feat: support for adaptive rate limiting [PIPE-481] #4160

BonapartePC commented Nov 20, 2023 •

edited by coderabbitai bot

Loading

codecov bot commented Nov 20, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as spam.

lvrach Nov 28, 2023

BonapartePC Nov 28, 2023

lvrach Dec 5, 2023 •

edited

Loading

atzoum Dec 6, 2023 •

edited

Loading

lvrach Dec 5, 2023

atzoum Dec 6, 2023

atzoum Dec 6, 2023

atzoum Dec 6, 2023 •

edited

Loading

atzoum left a comment

atzoum Dec 6, 2023

BonapartePC Dec 6, 2023

atzoum Dec 6, 2023

BonapartePC Dec 6, 2023

atzoum Dec 6, 2023

BonapartePC Dec 6, 2023

atzoum Dec 6, 2023

BonapartePC Dec 6, 2023

	defer t.limitFactorMeasurement.Gauge(limitFactor)
	t.limitFactorMeasurement.Gauge(limitFactor)

	"adaptive_enabled": fmt.Sprintf("%t", f.throttlers[destID].(*switchingThrottler).adaptiveEnabled.Load()),
	"adaptive": fmt.Sprintf("%t", f.throttlers[destID].(*switchingThrottler).adaptiveEnabled.Load()),

feat: support for adaptive rate limiting [PIPE-481] #4160

feat: support for adaptive rate limiting [PIPE-481] #4160

Conversation

BonapartePC commented Nov 20, 2023 • edited by coderabbitai bot Loading

Description

Security

Summary by CodeRabbit

codecov bot commented Nov 20, 2023 • edited Loading

Codecov Report

This comment was marked as resolved.

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvrach Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

atzoum Dec 6, 2023 • edited Loading

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

atzoum Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

atzoum 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

BonapartePC commented Nov 20, 2023 •

edited by coderabbitai bot

Loading

codecov bot commented Nov 20, 2023 •

edited

Loading

lvrach Dec 5, 2023 •

edited

Loading

atzoum Dec 6, 2023 •

edited

Loading

atzoum Dec 6, 2023 •

edited

Loading