chore: Add Benchmarks for IsProxyTrusted() #2933
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe update introduces benchmark scenarios for testing the Changes
Possibly related issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2933 +/- ##
==========================================
+ Coverage 82.80% 82.85% +0.04%
==========================================
Files 115 115
Lines 8409 8409
==========================================
+ Hits 6963 6967 +4
+ Misses 1108 1105 -3
+ Partials 338 337 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ctx_test.go (1 hunks)
Additional comments: 12
ctx_test.go (12)
- 5762-5771: The benchmark
NoProxyChecktests the performance ofIsProxyTrustedwithout any proxy check. This scenario is straightforward and correctly sets up the context for the test.- 5775-5887: The parallel version of the
NoProxyCheckbenchmark,NoProxyCheckParallel, correctly utilizesb.RunParallelfor concurrent execution. This is an appropriate use of parallel benchmarking to simulate concurrent requests.- 5790-5801: The
WithProxyCheckSimplebenchmark testsIsProxyTrustedwith a simple proxy check enabled but without specifying any trusted proxies. This scenario is valid for testing the overhead of enabling the proxy check feature.- 5806-5921: Similar to the previous comment, the parallel version
WithProxyCheckSimpleParallelcorrectly usesb.RunParallelto test the simple proxy check feature under concurrent load. This ensures the benchmark reflects real-world usage patterns.- 5823-5889: The
WithProxyCheckbenchmark tests theIsProxyTrustedmethod with a specific trusted proxy configured. This scenario is crucial for understanding the performance impact of validating against a list of trusted proxies.- 5892-5929: The parallel version
WithProxyCheckParallelis correctly set up to test the trusted proxy check feature under concurrent requests. This benchmark is essential for assessing the scalability of the proxy trust mechanism.- 5923-5930: The
WithProxyCheckSubnetbenchmark tests theIsProxyTrustedmethod with a subnet as a trusted proxy. This scenario is important for understanding the performance when using CIDR notation for trusted proxies.- 5931-5938: The parallel version
WithProxyCheckParallelSubnetcorrectly benchmarks the subnet-based trusted proxy check under concurrent load. This is a valuable scenario for assessing the performance of CIDR-based proxy trust checks.- 5939-5946: The
WithProxyCheckMultipleSubnetbenchmark tests theIsProxyTrustedmethod with multiple subnets as trusted proxies. This scenario is essential for understanding the performance implications of having a complex list of trusted proxies.- 5947-5954: The parallel version
WithProxyCheckParallelMultipleSubnetis well-constructed to test the performance of multiple subnet-based trusted proxy checks under concurrent requests. This benchmark is crucial for evaluating the scalability of the feature.- 5955-5962: The
WithProxyCheckAllSubnetsbenchmark tests theIsProxyTrustedmethod with a comprehensive list of subnets as trusted proxies. This scenario is critical for understanding the performance when a wide range of trusted proxies is configured.- 5963-5970: The parallel version
WithProxyCheckParallelAllSubnetscorrectly benchmarks the comprehensive list of subnet-based trusted proxy checks under concurrent load. This is an important scenario for assessing the performance and scalability of the proxy trust mechanism with a complex configuration.
gaby
changed the title
ReneWerner87
approved these changes
Apr 2, 2024
|
I believe I have found the reason for the slow benchmark results for IsProxyTrusted(). I would like to open a pull request but I am waiting on this one to be merged into main branch. Is there anything we're waiting on before merging this? |
|
@gaby ^^^ |
|
@xEricL I have two fixes:
Did you find something else? I have my stuff in my VM, just been busy with life 😂 |
|
@gaby I've been thinking about the slow benchmarks separately from my suggestion in #2930. My proposed solution is to use an array for The downside to my solution is that if a developer adds 20+ IP addresses to I guess it comes down to whether it's more realistic that most users will want to whitelist 20+ individual IP addresses or if they will be whitelisting ranges instead. This only solves half the problem though. I suspect the other half of the problem is from Here are my benchmarks for reference: Current implementationMy proposed solution |
I've never heard of an IP Trie before, but after googling it a bit it appears that might be a great solution for efficiently checking if the IP is within a range. In any case, that still leaves the overheard from ip.String() if we stick with the trustedProxiesMap for storing individual IPs. |
|
@gaby I found a better solution than using an arrray, since it probably isn't ideal to sacrifice performance if developers specify 30+ ip addresses. Perhaps we keep the map but use Here is implementation: 3d2c8e1 Here are the benchmarks: |
|
@xEricL I will check in 30mins my VM to see the difference with IPTrie. |
ReneWerner87
approved these changes
Apr 17, 2024
|
@gaby is it ready ? |
|
@ReneWerner87 Not yet @xEricL These are the benchmark results using an |
|
@xEricL Summary:
The parallel tests being faster in your CPU makes sense since it's a Ryzen 9. What i can determine is:
|
@gaby I agree. These benchmarks look promising. Nice work 👍🏻 |
|
So it's ready for review, right? |
|
@ReneWerner87 Yes, i will do the changes to improve performance in a separate PR once the IPTree funcs are added to gofiber/utils |
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
ctx_test.go (1)
5760-5986: Ensure consistent use of comments and formatting in the benchmarkBenchmark_Ctx_IsProxyTrusted.It's good practice to maintain consistency in comment styles and code formatting. This helps in improving the readability and maintainability of the code.
ReneWerner87
approved these changes
Apr 25, 2024
14 tasks
18 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add benchmarks for
IsProxyTrusted()with different configurations to compare performance between: No proxies, proxies, multiple subnets, and all subnets.These benchmarks will be relevant when #2930 gets implemented.
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Summary by CodeRabbit
IsProxyTrustedfunction to cover a wider range of trusted proxy configurations.