Bench web/infrastructure #3
Conversation
Reviewer's Guide by SourceryThis pull request sets up the core infrastructure and benchmarking framework for web performance testing. It includes Docker configurations, database setup scripts, test implementations, and verification utilities. ER diagram for database setuperDiagram
WORLD {
int id PK
int randomNumber
}
FORTUNE {
int id PK
varchar message
}
WORLD ||--o{ FORTUNE : contains
FORTUNE ||--o{ WORLD : contains
Class diagram for Benchmarker and FrameworkTestclassDiagram
class Benchmarker {
-config
-time_logger
-metadata
-audit
-tests
-results
-docker_helper
-last_test
+run()
+stop(signal, frame)
}
class FrameworkTest {
-name
-directory
-benchmarker
-runTests
-approach
-classification
-database
-framework
-language
-orm
-platform
-webserver
-os
-database_os
-display_name
-notes
-port
-versus
+start()
+is_accepting_requests()
+verify_urls()
}
Benchmarker --> FrameworkTest : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @gitworkflows - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting common test verification logic into shared utility functions to reduce code duplication between test type implementations
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 6. Fix this `README.md` and open a pull request | ||
|
|
||
| Starting on line 49 is your actual `README.md` that will sit with your test implementation. Update all the dummy values to their correct values so that when people visit your test in our Github repository, they will be greated with information on how your test implementation works and where to look for useful source code. | ||
|
|
||
| After you have the real `README.md` file in place, delete everything above line 59 and you are ready to open a pull request. |
There was a problem hiding this comment.
issue (documentation): Found typo: 'greated' should be 'greeted'
| return problems | ||
|
|
||
|
|
||
| def verify_updates(old_worlds, new_worlds, updates_expected, url): |
There was a problem hiding this comment.
issue (complexity): Consider extracting database validation logic into a dedicated DatabaseValidator class to improve separation of concerns.
The verification logic can be simplified by extracting database validation into a focused DatabaseValidator class. This separates concerns while maintaining functionality:
class DatabaseValidator:
def __init__(self, database, config):
self.database = database
self.config = config
def verify_updates(self, old_worlds, new_worlds, updates_expected):
successful_updates = 0
for entry_id in range(1, 10001):
str_id = str(entry_id)
if (str_id in old_worlds[0] and str_id in new_worlds[0] and
old_worlds[0][str_id] != new_worlds[0][str_id]):
successful_updates += 1
if successful_updates == 0:
return [("fail", "No items were updated")]
if successful_updates <= (updates_expected * 0.90):
return [("fail", f"Only {successful_updates} items updated out of {updates_expected}")]
return []
def verify_query_metrics(self, table, queries, rows, expected_queries, expected_rows):
problems = []
if queries > expected_queries * 1.05:
problems.append(("warn", f"Excessive queries: {queries} vs expected {expected_queries}"))
elif queries < expected_queries:
problems.append(("fail", f"Too few queries: {queries} vs expected {expected_queries}"))
# Add similar validation for rows
return problemsThis allows the main verification code to focus on HTTP/JSON validation while delegating database checks:
def verify_query_cases(self, cases, url, check_updates=False):
problems = []
db_validator = DatabaseValidator(self.database, self.config)
if check_updates:
world_db_before = databases[self.database.lower()].get_current_world_table(self.config)
for query, max_infraction in cases:
problems.extend(self._verify_single_query(query, url, max_infraction))
if check_updates and int(query) >= 500:
world_db_after = databases[self.database.lower()].get_current_world_table(self.config)
problems.extend(db_validator.verify_updates(world_db_before, world_db_after, 500))
return problemsThis improves maintainability by:
- Separating database validation concerns
- Reducing nesting depth
- Making the validation flow easier to follow
- Enabling focused testing of database validation logic
| @@ -0,0 +1,25 @@ | |||
| db = db.getSiblingDB('hello_world') | |||
| db.world.drop() | |||
| for (var i = 1; i <= 10000; i++) { | |||
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| raise Exception( | ||
| "Unknown error - test did not pass,warn,or fail") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| if entry_id in old_worlds[n] and entry_id in new_worlds[n]: | ||
| if old_worlds[n][entry_id] != new_worlds[n][entry_id]: | ||
| successful_updates += 1 |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if entry_id in old_worlds[n] and entry_id in new_worlds[n]: | |
| if old_worlds[n][entry_id] != new_worlds[n][entry_id]: | |
| successful_updates += 1 | |
| if entry_id in old_worlds[n] and entry_id in new_worlds[n] and old_worlds[n][entry_id] != new_worlds[n][entry_id]: | |
| successful_updates += 1 | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
|
|
||
| # json_url should be at least "/json" | ||
| if len(self.json_url) < 5: | ||
| problems.append( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace call to format with f-string (
use-fstring-for-formatting) - Replace if statement with if expression (
assign-if-exp)
| 'max_concurrency': | ||
| max(self.config.concurrency_levels), | ||
| 'name': | ||
| name, | ||
| 'duration': | ||
| self.config.duration, | ||
| 'levels': | ||
| " ".join( | ||
| "{}".format(item) for item in self.config.concurrency_levels), | ||
| 'server_host': | ||
| self.config.server_host, | ||
| 'url': | ||
| url, | ||
| 'accept': | ||
| "application/json,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7" |
There was a problem hiding this comment.
suggestion (code-quality): Replace call to format with f-string (use-fstring-for-formatting)
| 'max_concurrency': | |
| max(self.config.concurrency_levels), | |
| 'name': | |
| name, | |
| 'duration': | |
| self.config.duration, | |
| 'levels': | |
| " ".join( | |
| "{}".format(item) for item in self.config.concurrency_levels), | |
| 'server_host': | |
| self.config.server_host, | |
| 'url': | |
| url, | |
| 'accept': | |
| "application/json,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7" | |
| 'max_concurrency': max(self.config.concurrency_levels), | |
| 'name': name, | |
| 'duration': self.config.duration, | |
| 'levels': " ".join( | |
| f"{item}" for item in self.config.concurrency_levels | |
| ), | |
| 'server_host': self.config.server_host, | |
| 'url': url, | |
| 'accept': "application/json,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7", |
|
|
||
| # plaintext_url should be at least "/plaintext" | ||
| if len(self.plaintext_url) < 10: | ||
| problems.append( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace call to format with f-string (
use-fstring-for-formatting) - Replace if statement with if expression (
assign-if-exp)
| 'max_concurrency': | ||
| max(self.config.concurrency_levels), | ||
| 'name': | ||
| name, | ||
| 'duration': | ||
| self.config.duration, | ||
| 'levels': | ||
| " ".join("{}".format(item) | ||
| for item in self.config.pipeline_concurrency_levels), | ||
| 'server_host': | ||
| self.config.server_host, | ||
| 'url': | ||
| url, | ||
| 'pipeline': | ||
| 16, | ||
| 'accept': | ||
| "text/plain,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7" |
There was a problem hiding this comment.
suggestion (code-quality): Replace call to format with f-string (use-fstring-for-formatting)
| 'max_concurrency': | |
| max(self.config.concurrency_levels), | |
| 'name': | |
| name, | |
| 'duration': | |
| self.config.duration, | |
| 'levels': | |
| " ".join("{}".format(item) | |
| for item in self.config.pipeline_concurrency_levels), | |
| 'server_host': | |
| self.config.server_host, | |
| 'url': | |
| url, | |
| 'pipeline': | |
| 16, | |
| 'accept': | |
| "text/plain,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7" | |
| 'max_concurrency': max(self.config.concurrency_levels), | |
| 'name': name, | |
| 'duration': self.config.duration, | |
| 'levels': " ".join( | |
| f"{item}" for item in self.config.pipeline_concurrency_levels | |
| ), | |
| 'server_host': self.config.server_host, | |
| 'url': url, | |
| 'pipeline': 16, | |
| 'accept': "text/plain,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7", |
| ("fail", | ||
| "Route for queries must be at least 9 characters, found '{}' instead".format(self.query_url), | ||
| url)) |
There was a problem hiding this comment.
suggestion (code-quality): Replace call to format with f-string (use-fstring-for-formatting)
| ("fail", | |
| "Route for queries must be at least 9 characters, found '{}' instead".format(self.query_url), | |
| url)) | |
| ( | |
| "fail", | |
| f"Route for queries must be at least 9 characters, found '{self.query_url}' instead", | |
| url, | |
| ) | |
| ) |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
PR Type
enhancement, configuration changes, documentation
Description
Changes walkthrough 📝
13 files
wrk.dockerfile
Add Dockerfile for benchmarking with wrk on Ubuntubenchmarks/load-testing/wrk/wrk.dockerfile
image.
postgres.dockerfile
Add Dockerfile for PostgreSQL database setupinfrastructure/docker/databases/postgres/postgres.dockerfile
mysql.dockerfile
Add Dockerfile for MySQL database setupinfrastructure/docker/databases/mysql/mysql.dockerfile
mongodb.dockerfile
Add Dockerfile for MongoDB database setupinfrastructure/docker/databases/mongodb/mongodb.dockerfile
Dockerfile
Add Dockerfile for BW environment setupinfrastructure/docker/Dockerfile
60-postgresql-shm.conf
Add PostgreSQL shared memory configurationinfrastructure/docker/databases/postgres/60-postgresql-shm.conf
.siegerc
Add Siege configuration file for performance testinginfrastructure/docker/databases/.siegerc
for performance testing.
connection limits.
simulation.
my.cnf
Add MySQL configuration file with performance tuninginfrastructure/docker/databases/mysql/my.cnf
collation.
postgresql.conf
Add PostgreSQL configuration file with performance settingsinfrastructure/docker/databases/postgres/postgresql.conf
bw.service
Add systemd service file for BenchWeb managementinfrastructure/docker/services/bw.service
Vagrantfile
Add Vagrantfile for VM setup and provisioninginfrastructure/vagrant/Vagrantfile
benchmark_config.json
Add JSON configuration for benchmark test settingsbenchmarks/pre-benchmarks/benchmark_config.json
60-database-shm.conf
Add shared memory configuration for MySQLinfrastructure/docker/databases/mysql/60-database-shm.conf
31 files
verifications.py
Implement verification logic for benchmark testsbenchmarks/verifications.py
benchmarker.py
Implement benchmark management and execution logicbenchmarks/benchmarker.py
fortune_html_parser.py
Add HTML parser for fortune test verificationbenchmarks/fortune/fortune_html_parser.py
framework_test.py
Implement framework test management and verificationbenchmarks/framework_test.py
abstract_test_type.py
Define abstract base class for test typesbenchmarks/abstract_test_type.py
fortune.py
Implement fortune benchmark test typebenchmarks/fortune/fortune.py
abstract_database.py
Define abstract base class for database interactionsinfrastructure/docker/databases/abstract_database.py
db.py
Implement database benchmark test typebenchmarks/db/db.py
postgres.py
Implement PostgreSQL database interaction logicinfrastructure/docker/databases/postgres/postgres.py
data.
mongodb.py
Implement MongoDB database interaction logicinfrastructure/docker/databases/mongodb/mongodb.py
data.
mysql.py
Implement MySQL database interaction logicinfrastructure/docker/databases/mysql/mysql.py
data.
plaintext.py
Implement plaintext benchmark test typebenchmarks/plaintext/plaintext.py
cached-query.py
Implement cached query benchmark test typebenchmarks/cached-query/cached-query.py
query.py
Implement query benchmark test typebenchmarks/query/query.py
update.py
Implement update benchmark test typebenchmarks/update/update.py
json.py
Implement JSON benchmark test typebenchmarks/json/json.py
__init__.py
Add initialization script for database modulesinfrastructure/docker/databases/init.py
__init__.py
Add initialization script for benchmark test typesbenchmarks/init.py
create.js
Add MongoDB database creation scriptinfrastructure/docker/databases/mongodb/create.js
pipeline.sh
Add shell script for pipeline benchmarksbenchmarks/load-testing/wrk/pipeline.sh
concurrency.sh
Add shell script for concurrency benchmarksbenchmarks/load-testing/wrk/concurrency.sh
query.sh
Add shell script for query benchmarksbenchmarks/load-testing/wrk/query.sh
bw-startup.sh
Add startup script for BW servicesinfrastructure/docker/services/bw-startup.sh
bootstrap.sh
Add bootstrap script for Vagrant environmentinfrastructure/vagrant/bootstrap.sh
bw-shutdown.sh
Add shutdown script for BW servicesinfrastructure/docker/services/bw-shutdown.sh
entry.sh
Add entry script for Docker containerinfrastructure/docker/entry.sh
config.sh
Add configuration script for PostgreSQLinfrastructure/docker/databases/postgres/config.sh
core.rb
Add Vagrant provider configuration scriptinfrastructure/vagrant/core.rb
pipeline.lua
Add Lua script for wrk pipeline benchmarkingbenchmarks/load-testing/wrk/pipeline.lua
create-postgres.sql
Add PostgreSQL database creation scriptinfrastructure/docker/databases/postgres/create-postgres.sql
create.sql
Add MySQL database creation scriptinfrastructure/docker/databases/mysql/create.sql
1 files
custom_motd.sh
Add placeholder for custom MOTD scriptinfrastructure/vagrant/custom_motd.sh
2 files
README.md
Add README for Vagrant development environment setupinfrastructure/vagrant/README.md
README.md
Add README for pre-benchmark setup instructionsbenchmarks/pre-benchmarks/README.md