#585 #1229 #1598 #1915 Rate limiting global configuration#2294
#585 #1229 #1598 #1915 Rate limiting global configuration#2294raman-m merged 74 commits intoThreeMammals:developfrom
Conversation
raman-m
added
feature
|
Hello, Milad! |
raman-m
changed the title
raman-m
changed the title
There was a problem hiding this comment.
Yellow card penalty! 🟨
Thank once again for submitting this PR However, after the code review, I consider it a draft.
The primary concern is the lack both unit and acceptance tests ❗
While your intention to add documentation is appreciated, please note that documentation should be written upon the completion of the code (at the end of development).
Several minor but important issues need to be addressed as listed below 👇
- I recommend starting with the file models design.
- Please be aware that the linked #1229 issue requires grouping routes by a key. Prioritize this type of grouping initially, and upon completion, proceed with the Patterns concept.
- Presenting your ideas using patterns and method grouping is also necessary. However, I do not support your idea of including the "Methods" feature, as it mixes concerns.
P.S. I will provide my vision of the model's design at a later stage.
P.P.S. I anticipate several rounds of code review. However, this feat has high priority, and I will support you through multiple pair-programming sessions.
P.P.P.S. Other members from the Ocelot development team could review this PR and provide separate code feedback 😉
Life, indeed, not easy 😛
src/Ocelot/Configuration/Creator/GlobalRateLimitOptionsCreator.cs
Outdated
Show resolved
Hide resolved
Development Complete ✔️ |
|
Hello @raman-m review on going... |
|
Documentation review commit → be6c3b2 |
|
@raman-m i was waiting on your doc updates. |
|
@ggnaegi Please start! But to read nice HTML docs you have to checkout and make local docs build by command |
There was a problem hiding this comment.
Ready for delivery ✅
- Code review ✔️✔️
- Unit testing ✔️✔️✔️
- Acceptance testing ✔️
- Updated docs ✔️✔️
@ggnaegi @RaynaldM
This is my 2nd in-depth review of the feature design, and I’m now confident that the feature functions as intended with the fixed window algorithm. The thorough unit testing and additional acceptance testing ensure feature quality. That's why I dedicated a month of development efforts to implementing global configuration and completing the second review.
I identified a few bottlenecks in the architecture, resolved a minor issue with the X-RateLimit-Limit header, and I'm hoping for upcoming feature enhancements, so check the Roadmap in the docs. The next development stage will be addressed in the successor PR #2188 which goes to the next version 25.0.
Here is the development history I want to discuss:
- #37 and I was genuinely surprised at how poor the quality was. This version didn’t function properly, to be honest, and it was unreliable, full of bugs, and overall disappointing
- #404 where Tom introduced the waiting period in the documentation and attempted to fix the bug, but his feature design review failed because he wasn't the author.
- #508 Tom introduced the
DynamicRoutessection in the configuration to set up rate limiting. While it was a configuration update for the Dynamic Routing feature, there were no improvements made to Rate Limiting. - #1307 by Jolanta aka "Ms. J." 😄 The bug fix addressed the issue with headers in the
RateLimitingMiddleware, and I really admire her attitude and contributions to the project. - #1592 The author resolved the bug introduced by Tom 🤣, specifically the issue with the fixed window counter expiry. The fixed window algorithm was also refactored entirely to catch and eliminate the bug 🙈 I've finally decided to push ahead with aggressive refactoring to complete the first major upgrade of the feature's architecture. After the design review, the feature was stable and functioning correctly. I proposed "Quota Exceeded period" (
PeriodTimepsanoption) to evolve the algo as hybrid fixed window. - Current PR #2294 by me, sorry, Milad, the author, provided a draft version, and he didn't implement the feat actually 🙉 This PR introduces global configuration for static routes, marking the second major upgrade of the feature. The main goal is to split the old algorithm into two independent fixed-window algorithms: the proposed
Wait(akaPeriodTimespan) with a nullable option for a hybrid version, and the isolated basicPeriodalgorithm as the classic fixed window. This allows both algorithms to be used independently. Additionally, as an extra goal, the configuration is being prepared to accommodate more rate limiter rules and algorithms in the future. - The next PR #2188 from the Roadmap will be included in version 25.0. My goal is to ensure Ocelot's rate limiting middleware is compatible with the ASP.NET Core one. If they turn out to be incompatible, we will introduce a new rate limiting rule. Alright, let's see how it goes...
P.S.
Milad, the mastermind behind this PR, skipped working on the attached #1229 feature and instead pitched a wild idea: grouping routes by upstream path regular expressions (what a mad genius). In my opinion, this is not bad idea which could be implemented in addition to configuration groups by route keys.
I came across a draft feature without any tests, and to my surprise, the author vanished after the first code review 🤣 In the end, he got two penalties: a yellow card and a red one. It was clear he had no intention of writing solid, high-quality code or addressing the review issues. On top of that, he violated our Development Process.
| .Append(']') | ||
| .ToString(); | ||
| char hdrSign = EnableHeaders == false ? '-' : '+'; | ||
| string waitWindow = PeriodTimespan.HasValue ? PeriodTimespan.Value.ToString("F3") + 's' : Wait.IfEmpty(None); |
There was a problem hiding this comment.
PeriodTimespan is obsolete, will be removed in version 25
There was a problem hiding this comment.
Yes, it’s obsolete! Curious to know why?
Have you reviewed the documentation and the Deprecated options section?
I suggest deprecating PeriodTimespan in favor of Wait due to the confusion it causes. Even Tom made a mistake during a previous bug fix.
PeriodTimespan is not a Period ❗
It represents the Quota Exceeded period, which users can configure as additional behavior for the fixed window algorithm.
I am confident that PeriodTimespan should be renamed to Wait. If you are concerned about the deprecation process and migration, I will personally clarify it while working on version 25.0. Agreed?
Read the footnote-2, and it will clarify things for you a bit more.
| var baseArr = base.ToString().ToCharArray(); | ||
| if (DisableRateLimitHeaders.HasValue) | ||
| { | ||
| baseArr[1] = DisableRateLimitHeaders.Value ? '-' : '+'; // replace hdr sign | ||
| } | ||
|
|
||
| string baseString = new(baseArr); |
There was a problem hiding this comment.
| var baseArr = base.ToString().ToCharArray(); | |
| if (DisableRateLimitHeaders.HasValue) | |
| { | |
| baseArr[1] = DisableRateLimitHeaders.Value ? '-' : '+'; // replace hdr sign | |
| } | |
| string baseString = new(baseArr); | |
| var baseString = base.ToString(); | |
| // Replace the second character (+/-) only if necessary | |
| if (DisableRateLimitHeaders is bool hdr) | |
| { | |
| baseString = string.Create(baseStr.Length, (baseStr, hdr), static (dst, state) => | |
| { | |
| // Copy the original string | |
| state.baseStr.AsSpan().CopyTo(dst); | |
| // Overwrite index 1 with '+' or '-' depending on the flag | |
| if (dst.Length > 1) | |
| dst[1] = state.hdr ? '-' : '+'; | |
| }); | |
| } |
There was a problem hiding this comment.
Really curious about this suggestion ❕ Is this AI-generated code? 🤨
- Why add 6 extra lines of code instead of 3 clean lines? I understand it's for performance improvement, but there's no point in debating much since this code will be removed once
DisableRateLimitHeadersis deprecated. - This code appears to be AI-generated and is not compilable!
The good version is:Ray, be cautious with AI; it might have unexpected effects on your project 😉- baseString = string.Create(baseStr.Length, (baseStr, hdr), static (dst, state) => - state.baseStr.AsSpan().CopyTo(dst); + baseString = string.Create(baseString.Length, (baseString, hdr), static (dst, state) => + state.baseString.AsSpan().CopyTo(dst);
- The aim of this memory optimization is to ensure that nothing is allocated on the heap (avoiding GC pressure caused by converting to an
Arrayinstance). Therefore, I suggest an alternative in-stack solution:Span<byte> span = stackalloc byte[Encoding.ASCII.GetByteCount(baseString)]; Encoding.ASCII.GetBytes(baseString, span); span[1] = (byte)hdrSign; // replace hdr sign baseString = Encoding.ASCII.GetString(span);
I've decided to stick with your version. This code might be available in Ocelot.Infrastructure.Extensions.StringExtensions as useful helpers
| var options = downstreamRoute.RateLimitOptions ?? new(false); | ||
| if (!options.EnableRateLimiting) | ||
| { | ||
| Logger.LogInformation(() => $"Rate limiting is disabled for route '{downstreamRoute.Name()}' via the {nameof(RateLimitOptions.EnableRateLimiting)} option."); |
There was a problem hiding this comment.
In our case, with high-speed traffic, this type of log is just noise (completely unusable in bulk and costly), so it would be good to avoid it
There was a problem hiding this comment.
Ray, this isn't my code; to be fair, this logging has been around for ages! → Check out commit 9774580.
However, if you're looking for optimizations, that's fine, let's discuss it. The log level is Information, which is closely related to Debug. For reference: Debug (1) < Information (2) < Warning (3).
I'm curious about your case. Could you please share more details?
If your team is using the Information level in production, they should immediately increase the logging level to Error. For the testing environment, it's fine to keep the Warning level.
OK, not an issue.
| { | ||
| var headers = _limiter.GetHeaders(originalContext, options, now, counter); | ||
| originalContext.Response.OnStarting(SetRateLimitHeaders, state: headers); | ||
| Logger.LogInformation(() => $"Route '{downstreamRoute.Name()}' must return rate limiting headers with the following data: {headers}"); |
There was a problem hiding this comment.
Which one? I can't hear any noise. Debug level?
Not an issue!
| { | ||
| var req = context.Request; | ||
| var rule = options.Rule; | ||
| Logger.LogWarning(() => |
There was a problem hiding this comment.
This type of logging should be optional, again for the same reason of high traffic intensity (we handle this in metrics)
There was a problem hiding this comment.
Request your team to set up metrics and adjust the log level to Error in production. Keep in mind, this isn't my code, and the warning has been present for years.
Since the method is virtual, your development team can override it and implement any logging they prefer, including disabling it entirely.
Not an issue!
Co-authored-by: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com>
raman-m
had a problem deploying
to
build.cake
GitHub Actions
Failure
raman-m
removed
in progress
6 participants
Closes #585 #1229 #1915
Proposed Changes
This PR implements global rate limiting per #1229.
RouteKeysarray into the globalRateLimitOptionsto consolidate global options into a single group.RateLimitOptionsCreatorclass implements a merging algorithm that performs merging on a per-option basis.DynamicsCreatorenables the merging of dynamic routeRateLimitOptions, accomplished through models and their interfaces.Filemodels and introduced new ones to incorporate additional RL rules and potentially implement rate limiter algorithms.RateLimitOptionsto serve as a unified business model, consolidating all configuration.ClientRequestIdentityuniqueness by eliminating incorrect path and method data (as they don't ensure a unique key) and substituting them with the stable routeLoadBalancerKey, which is inherently unique.RateLimitingcore service by separating fixed window algorithms and adding a newDateTime nowparameter for precise calculations, effectively resolving previously unstable buggy behavior.RateLimitingMiddlewareworkflow by introducing newBreakandBlockflows. TheBlockflow, in particular, enhances security by disallowing unknown clients; however, the ultimate security bottleneck remains unresolved.*.csproj, includingMicrosoft.*, to the latest version 9.0.9.Successor
Next, this pull request will be merged.
Predecessors (feature history)