Feature/rate limiter http #2055
Conversation
There was a problem hiding this comment.
- There are no tests added for the new code.
- Documentation is not provided.
- Removes the current godoc comments at many places.
- Version changes are unncessary and should be removed.
- The way we are injecting Rate Limiter doesn't comply with what we are currently following for other HTTP options. Let's try to stick to the previous way only.
| @@ -1,3 +1,3 @@ | |||
| package version | |||
|
|
|||
| const Framework = "dev" | |||
| const Framework = "v1.42.3" | |||
There was a problem hiding this comment.
This change is not required. Please revert it.
| @@ -24,50 +24,35 @@ type httpService struct { | |||
| } | |||
|
|
|||
| type HTTP interface { | |||
| // HTTP is embedded as HTTP would be able to access it's clients method | |||
There was a problem hiding this comment.
Sorry, but any particular reason for removing these godoc comments??
| for _, o := range options { | ||
| if rlOption, ok := o.(*WithRateLimiter); ok { |
There was a problem hiding this comment.
If you need to add this option manually instead of doing:
svc = o.AddOption(svc)
i think the implementation is wrong. For the other configs/features we are not additionally providing the logger metrics etc then why for rate limiter??
There was a problem hiding this comment.
Sure, I’ll review the implementation again and make the necessary updates. Thanks for the feedback!
| type Options interface { | ||
| AddOption(h HTTP) HTTP | ||
| AddOption(HTTP) HTTP |
There was a problem hiding this comment.
Why this change?
I think sticking to :
a.AddHTTPService("cat-facts", "https://catfact.ninja",
service.NewAPIKeyConfig("some-random-key"),
service.NewBasicAuthConfig("username", "password"),
&service.CircuitBreakerConfig{
Threshold: 4,
Interval: 1 * time.Second,
},
&service.DefaultHeaders{Headers: map[string]string{"key": "value"}},
&service.HealthConfig{
HealthEndpoint: "breeds",
},
service.NewOAuthConfig("clientID", "clientSecret",
"https://tokenurl.com", nil, nil, 0),
&service.RetryConfig{
MaxRetries: 5
},
)
Is better?
|
Hey! @archit-2003. Thankyou for your contribution. Currently your PR has the below linter issues. Please resolve all the linters to completely pass the workflow. 
|
|
@archit-2003 Are you still working on this issue. If yes then please resolve the review comments given. Let us know if you need help in anything. |
This PR introduces a built-in rate limiting capability for interservice HTTP calls within Gofr.
Tasks Completed:
RateLimiterConfigstruct: This new struct allows configuration of rate limits viaRequestsPerSecondandBurstparameters.AddHTTPService: Aservice.WithRateLimiteroption is now available to apply rate limiting when defining HTTP services.WithRateLimitergets its own independent rate limiter instance, ensuring isolation.Loggerinterface.OpenTelemetryevent is added to the active span for rate-limited requests.Prometheuscounter (app_http_service_rate_limit_exceeded_total) is incremented upon rate limit triggers.Technical Details / Files Changed:
pkg/gofr/service/ratelimiter.go: New file definingRateLimiterConfigand the core rate limiting logic.pkg/gofr/service/options.go: Modified to include theWithRateLimiteroption.pkg/gofr/service/new.go: Modified to correctly initialize and inject dependencies into the rate limiter.examples/using-http-service/main.go): Modified for local testing demonstration.