feat(tools/benchmark): introduce benchmark module #22778
Conversation
kocubinski
requested review from
auricom,
julienrbrt,
tac0turtle,
a team and
facundomedica
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive benchmarking module for the Cosmos SDK, comprising several new files and modifications across existing files. Key components include Protobuf message types for benchmark operations, configuration for load testing, and enhancements to the application’s initialization logic. The changes also implement a command-line interface for executing benchmark tests, alongside the establishment of a generator for creating benchmark data. Additionally, several configuration files for dependency management and project setup are added, ensuring the new module integrates seamlessly into the Cosmos SDK ecosystem. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
|
Won't be able to review today, but could you apply this checklist: https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#gomod |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (17)
x/benchmark/module/depinject.go (3)
16-17: Group constants into a single block for improved readabilityConsider grouping the constants
ModuleNameandmaxStoreKeyGenIterationsinto a singleconstblock to enhance code readability and maintainability.Apply this diff to group the constants:
-const ModuleName = "benchmark" -const maxStoreKeyGenIterations = 100 +const ( + ModuleName = "benchmark" + maxStoreKeyGenIterations = 100 +)
20-20: Address the TODO comment or remove itThe TODO comment suggests trying the depinject gogo API. Please resolve this by implementing the intended functionality or removing the comment if it's no longer relevant.
Would you like assistance in exploring the depinject gogo API or creating a GitHub issue to track this task?
65-69: Avoid using the 'unsafe' package for string to byte slice conversionUsing the
unsafepackage can lead to unexpected behavior and is generally discouraged unless absolutely necessary. Consider using a safe alternative like[]byte(s)for converting a string to a byte slice.Apply this diff to use a safe conversion:
-func unsafeStrToBytes(s string) []byte { - return unsafe.Slice(unsafe.StringData(s), len(s)) // ref https://github.com/golang/go/issues/53003#issuecomment-1140276077 +func strToBytes(s string) []byte { + return []byte(s) }If performance is a critical concern, ensure thorough benchmarking and documentation to justify the use of
unsafe.x/benchmark/module/keeper.go (1)
16-21: Ensure consistent naming for metric keysThe variable names for metric keys are inconsistent. To improve code clarity, consider appending
Keyto all metric variable names.Apply this diff to make variable names consistent:
var ( _ benchmark.MsgServer = &Keeper{} metricOpKey = []string{"benchmark", "op"} metricGetKey = append(metricOpKey, "get") - metricDelete = append(metricOpKey, "delete") - metricInsertKey = append(metricOpKey, "insert") - metricUpdateKey = append(metricOpKey, "update") + metricDeleteKey = append(metricOpKey, "delete") + metricInsertKey = append(metricOpKey, "insert") + metricUpdateKey = append(metricOpKey, "update") metricTotalKey = []string{"benchmark", "total"} metricMissKey = []string{"benchmark", "miss"} )x/benchmark/module/module.go (2)
14-14: Clarify the purpose of the blank import with a more informative commentThe comment
// for some reason this is required to make msg server registration workis vague. Please provide a clearer explanation of why this import is necessary.Consider updating the comment for better clarity:
_ "cosmossdk.io/api/cosmos/benchmark/v1" - // for some reason this is required to make msg server registration work + // Required to register benchmark message service implementations
70-70: Use the logger instead of fmt.Printf for loggingAvoid using
fmt.Printffor logging purposes. Use the provided logger to ensure consistent logging practices across the codebase.Apply this diff to replace
fmt.Printfwith the logger:if i%100_000 == 0 { - fmt.Printf("init genesis: %d/%d\n", i, a.genesisParams.GenesisCount) + a.log.Info("init genesis", "progress", i, "total", a.genesisParams.GenesisCount) a.log.Warn("init genesis", "progress", i, "total", a.genesisParams.GenesisCount) }api/cosmos/benchmark/v1/benchmark.pulsar.go (1)
1-1: Generated Code AcknowledgmentThis file is generated by
protoc-gen-go-pulsar. As per best practices, generated code should not be manually modified. Ensure that the code generation process is correctly configured and documented.api/cosmos/benchmark/v1/tx.pulsar.go (1)
1-1: Generated Code AcknowledgmentThis file is generated by
protoc-gen-go-pulsar. Typically, generated code is not manually reviewed for style adherence. Please confirm that the generated code is up-to-date and that the generation process is well-documented.api/cosmos/benchmark/module/v1/module.pulsar.go (1)
1-1: Generated Code AcknowledgmentThis file is generated by
protoc-gen-go-pulsar. It's standard practice not to manually edit generated files. Please ensure that the generation process is correctly set up and any necessary configurations are tracked.x/benchmark/proto/buf.yaml (1)
4-4: Recommend Pinning the Cosmos SDK Dependency VersionThe dependency
buf.build/cosmos/cosmos-sdkis not pinned to a specific version. For reproducibility and to prevent potential issues from upstream changes, it's advisable to pin this dependency to a specific version or commit hash, similar to howcometbftis pinned.x/benchmark/proto/cosmos/benchmark/module/v1/module.proto (2)
28-31: Reorder operation weight fields for better readabilityThe field numbers and ordering of operation weights could be improved for better maintainability:
- Field numbers have gaps (10-12)
delete_weightis placed afterget_weightdespite having a lower field number- float insert_weight = 9; - float update_weight = 10; - float get_weight = 12; - float delete_weight = 11; + float insert_weight = 9; + float update_weight = 10; + float delete_weight = 11; + float get_weight = 12;
17-32: Add field constraints for GeneratorParamsThe GeneratorParams message lacks necessary constraints:
- Operation weights should sum to 1.0
- Statistical parameters (mean, std_dev) need range validations
Consider adding protobuf validations using the validate.rules option. Example:
import "validate/validate.proto"; message GeneratorParams { // ... other fields ... float insert_weight = 9 [(validate.rules).float = {gte: 0, lte: 1}]; float update_weight = 10 [(validate.rules).float = {gte: 0, lte: 1}]; float delete_weight = 11 [(validate.rules).float = {gte: 0, lte: 1}]; float get_weight = 12 [(validate.rules).float = {gte: 0, lte: 1}]; }x/benchmark/proto/cosmos/benchmark/v1/tx.proto (1)
21-25: Enhance MsgLoadTestResponse with detailed metricsThe current response only includes basic metrics. Consider adding:
- Individual operation type counts
- Average operation latency
- Memory usage metrics
- Error breakdown by operation type
Also, please document the unit of measurement for
total_time(milliseconds/seconds?).x/benchmark/generator/gen_test.go (2)
13-75: Consider adding error test cases and improving test documentation.The test is comprehensive for happy paths but could benefit from:
- Testing error scenarios (e.g., invalid parameters)
- Adding documentation about the test's purpose and methodology
- Testing edge cases (e.g., zero counts, maximum values)
77-128: Reduce code duplication with Test_Genesis.Consider extracting common test setup and verification logic into helper functions to improve maintainability.
+func setupTestGenerator(t *testing.T) (*Generator, map[uint64]map[uint64]bool) { + params := &benchmarkmodulev1.GeneratorParams{ + Seed: 34, + BucketCount: 10, + GenesisCount: 2_000_000, + KeyMean: 64, + KeyStdDev: 8, + ValueMean: 1024, + ValueStdDev: 256, + } + g := NewGenerator(Options{GeneratorParams: params}) + db := make(map[uint64]map[uint64]bool) + return g, db +}simapp/app_config.go (1)
302-315: Document genesis parameters and verify module ordering.The benchmark module configuration should:
- Include comments explaining the purpose and impact of each genesis parameter
- Consider whether the module needs to be included in BeginBlockers/EndBlockers lists
{ Name: benchmark.ModuleName, Config: appconfig.WrapAny(&benchmarkmodulev1.Module{ GenesisParams: &benchmarkmodulev1.GeneratorParams{ + // Initial seed for deterministic generation Seed: 34, + // Number of store buckets for data distribution BucketCount: 2, + // Number of initial key-value pairs GenesisCount: 500_000, + // Target mean length for keys KeyMean: 64, + // Standard deviation for key length KeyStdDev: 12, + // Target mean length for values ValueMean: 1024, + // Standard deviation for value length ValueStdDev: 256, }, }), },simapp/v2/go.mod (1)
263-263: Consider adding a comment about the testing-only nature of the benchmark moduleSince this module is specifically designed for testing and not intended for production use, consider adding a comment above the replace directive to document this important detail.
+// x/benchmark is a testing module and should not be used in production cosmossdk.io/x/benchmark => ../../x/benchmark
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
api/cosmos/benchmark/v1/tx_grpc.pb.gois excluded by!**/*.pb.gox/benchmark/benchmark.pb.gois excluded by!**/*.pb.gox/benchmark/go.sumis excluded by!**/*.sumx/benchmark/proto/buf.lockis excluded by!**/*.lockx/benchmark/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
api/cosmos/benchmark/module/v1/module.pulsar.go(1 hunks)api/cosmos/benchmark/v1/benchmark.pulsar.go(1 hunks)api/cosmos/benchmark/v1/tx.pulsar.go(1 hunks)scripts/init-simapp-v2.sh(1 hunks)simapp/app_config.go(4 hunks)simapp/go.mod(2 hunks)simapp/v2/app_config.go(5 hunks)simapp/v2/go.mod(2 hunks)tests/go.mod(2 hunks)x/benchmark/client/cli/tx.go(1 hunks)x/benchmark/generator/gen.go(1 hunks)x/benchmark/generator/gen_test.go(1 hunks)x/benchmark/go.mod(1 hunks)x/benchmark/module/depinject.go(1 hunks)x/benchmark/module/keeper.go(1 hunks)x/benchmark/module/module.go(1 hunks)x/benchmark/proto/buf.gen.gogo.yaml(1 hunks)x/benchmark/proto/buf.gen.pulsar.yaml(1 hunks)x/benchmark/proto/buf.yaml(1 hunks)x/benchmark/proto/cosmos/benchmark/module/v1/module.proto(1 hunks)x/benchmark/proto/cosmos/benchmark/v1/benchmark.proto(1 hunks)x/benchmark/proto/cosmos/benchmark/v1/tx.proto(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/benchmark/proto/buf.gen.gogo.yaml
- x/benchmark/proto/buf.gen.pulsar.yaml
🧰 Additional context used
📓 Path-based instructions (12)
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
simapp/app_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/benchmark/module/depinject.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
api/cosmos/benchmark/v1/tx.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/benchmark/client/cli/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/app_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/benchmark/generator/gen_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/benchmark/module/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/benchmark/module/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
api/cosmos/benchmark/v1/benchmark.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
api/cosmos/benchmark/module/v1/module.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/benchmark/generator/gen.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf (1.47.2)
x/benchmark/proto/cosmos/benchmark/v1/tx.proto
5-5: import "cosmos/benchmark/v1/benchmark.proto": file does not exist
(COMPILE)
x/benchmark/proto/cosmos/benchmark/module/v1/module.proto
5-5: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
x/benchmark/proto/cosmos/benchmark/v1/benchmark.proto
3-3: Files with package "cosmos.benchmark.v1" must be within a directory "cosmos/benchmark/v1" relative to root but were in directory "x/benchmark/proto/cosmos/benchmark/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (12)
x/benchmark/proto/cosmos/benchmark/module/v1/module.proto (1)
5-5: Fix missing proto import
The import "cosmos/app/v1alpha1/module.proto" is reported as missing. This needs to be resolved to ensure proper compilation.
✅ Verification successful
Import path is valid and file exists
The import "cosmos/app/v1alpha1/module.proto" is correctly specified as the file exists at proto/cosmos/app/v1alpha1/module.proto in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the proto file exists in the expected locations
fd -t f "module.proto" | grep -i "cosmos/app"
Length of output: 170
🧰 Tools
🪛 buf (1.47.2)
5-5: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
x/benchmark/proto/cosmos/benchmark/v1/tx.proto (1)
5-5: Fix missing proto import
The import "cosmos/benchmark/v1/benchmark.proto" is reported as missing. This needs to be resolved to ensure proper compilation.
✅ Verification successful
Import statement is correctly defined
The import statement cosmos/benchmark/v1/benchmark.proto is valid as the file exists at the expected location: x/benchmark/proto/cosmos/benchmark/v1/benchmark.proto.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the benchmark proto file exists
fd -t f "benchmark.proto" | grep -i "cosmos/benchmark"
Length of output: 106
🧰 Tools
🪛 buf (1.47.2)
5-5: import "cosmos/benchmark/v1/benchmark.proto": file does not exist
(COMPILE)
scripts/init-simapp-v2.sh (1)
13-13: Review Prometheus retention time setting
The 600-second (10-minute) retention time might be too short for comprehensive benchmark analysis, especially for long-running tests generating large datasets (up to 20 million entries as mentioned in PR objectives).
Consider:
- Making this configurable via environment variable
- Increasing the default value
- Adding a comment explaining the chosen value
Example modification:
-$SIMD_BIN config set app telemetry.prometheus-retention-time 600
+# Set Prometheus metrics retention time (default: 1 hour)
+PROM_RETENTION=${PROM_RETENTION:-3600}
+$SIMD_BIN config set app telemetry.prometheus-retention-time $PROM_RETENTIONx/benchmark/generator/gen_test.go (1)
130-138: LGTM!
The test effectively verifies the deterministic behavior of the Bytes function.
simapp/app_config.go (1)
179-179: Verify module initialization order.
The benchmark module is added to InitGenesis. Ensure this ordering is correct relative to its dependencies.
✅ Verification successful
Let me gather more information about the module initialization order.
Let me try another approach to verify the module initialization order.
Let me check for any dependencies of the benchmark module.
Module initialization order appears safe
The benchmark module has minimal dependencies and acts as a message server only. From the InitGenesis order in app_config.go, it's being added after all core modules (consensus, accounts, auth, bank, etc.) which is appropriate for an auxiliary module that doesn't have critical dependencies. The module doesn't maintain its own state (no StoreKey usage found) and only implements the MsgServer interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any dependencies on the benchmark module
rg -l "benchmark\." --type go | grep -v "_test.go" | xargs -I {} rg "init.*genesis" {} -A 5
Length of output: 1676
Script:
#!/bin/bash
# Search for module initialization order in app configuration
rg -B 5 -A 10 "InitGenesis.*\[" --type go
# Also check for any direct dependencies of the benchmark module
ast-grep --pattern 'import ($$$"github.com/cosmos/cosmos-sdk/x/benchmark"$$$)'
Length of output: 65747
Script:
#!/bin/bash
# Check the InitGenesis order in app configuration
rg -A 20 'InitGenesis.*\[.*\]' simapp/app_config.go
Length of output: 655
Script:
#!/bin/bash
# Search for any dependencies of the benchmark module
rg "benchmark.*Keeper" --type go
# Also check for store key dependencies
rg "benchmark.*StoreKey" --type go
Length of output: 149
simapp/v2/app_config.go (4)
15-15: LGTM: Import statements are properly organized.
The new benchmark module imports are correctly added and organized according to the codebase conventions.
Also applies to: 43-43
178-178: LGTM: Benchmark module correctly added to InitGenesis sequence.
The benchmark module is properly integrated into the initialization sequence.
187-189:
Security consideration: Significantly increased gas limit.
The ValidateTxGasLimit has been increased by 100x from the default value. While this aligns with the benchmark testing requirements, it could pose security risks if accidentally used in production environments.
307-320: Verify benchmark configuration parameters.
The benchmark module is configured with:
- 5M entries for genesis (GenesisCount)
- Key size parameters: mean=64, stddev=12
- Value size parameters: mean=1024, stddev=256
While these parameters align with the PR objectives for performance testing, consider:
- The memory implications of generating 5M entries during genesis
- The disk space requirements for the generated data
- The impact on node startup time
✅ Verification successful
Let me gather more information about the benchmark module's usage and context.
Benchmark configuration appears reasonable for performance testing
The current configuration of 5M entries is appropriate for the benchmark module based on the following findings:
- The configuration in simapp/app_config.go shows this is an intentional increase from 500K to 5M entries
- The module is specifically designed for performance testing with configurable parameters
- Test configurations in generator/gen_test.go demonstrate similar scale (2M entries) being used
- The estimated storage requirement of ~5.2GB (5.44B bytes) is reasonable for a benchmark environment
The current parameters strike a good balance between:
- Meaningful dataset size for performance measurements
- Manageable resource requirements
- Consistent with test configurations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Calculate approximate storage requirements
echo "Estimated storage calculation:"
echo "Average key size: 64 bytes"
echo "Average value size: 1024 bytes"
echo "Number of entries: 5,000,000"
echo "Estimated total size: $((5000000 * (64 + 1024))) bytes"
Length of output: 358
Script:
#!/bin/bash
# Search for benchmark module configuration and usage
echo "=== Searching for benchmark module configuration ==="
rg -A 5 "benchmark.*Module" --type go
echo -e "\n=== Checking benchmark module documentation ==="
fd -e md -e txt . | xargs rg -l "benchmark" | while read -r file; do
echo -e "\n=== Content of $file ==="
cat "$file" | rg -A 3 -i "benchmark"
done
echo -e "\n=== Looking for test configurations ==="
rg "GenesisCount|BucketCount" --type go
Length of output: 20879
tests/go.mod (1)
77-77: LGTM: Module dependencies properly configured.
The benchmark module is correctly added as a dependency and properly configured with a replace directive for local development.
Also applies to: 268-268
simapp/go.mod (1)
54-54: LGTM: Module dependencies properly configured.
The benchmark module dependency and replace directive are correctly configured, maintaining consistency with the testing configuration.
Also applies to: 264-264
simapp/v2/go.mod (1)
49-49: LGTM: Benchmark module dependency properly added
The dependency declaration for the benchmark module follows the correct format and version pattern for local module development.
x/benchmark/go.mod
Outdated
| require ( | ||
| cosmossdk.io/api v0.7.6 | ||
| cosmossdk.io/core v1.0.0-alpha.6 | ||
| cosmossdk.io/depinject v1.1.0 | ||
| cosmossdk.io/log v1.5.0 | ||
| github.com/cespare/xxhash/v2 v2.3.0 | ||
| github.com/cosmos/cosmos-sdk v0.53.0 | ||
| github.com/cosmos/gogoproto v1.7.0 | ||
| github.com/grpc-ecosystem/grpc-gateway v1.16.0 | ||
| github.com/spf13/cobra v1.8.1 | ||
| github.com/stretchr/testify v1.10.0 | ||
| google.golang.org/grpc v1.68.0 | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Security vulnerabilities found in dependencies - updates recommended
The verification revealed several security concerns:
-
cosmos-sdk v0.53.0:
- HIGH severity vulnerability (ASA-2024-006) affecting versions <= 0.50.4
- Multiple MODERATE severity issues in recent versions
-
google.golang.org/grpc v1.68.0:
- LOW severity issue with private tokens in logs (>= 1.64.0, < 1.64.1)
- Previous HIGH severity HTTP/2 Rapid Reset vulnerability
-
grpc-gateway v1.16.0 is significantly outdated (current stable is v2.x)
Recommend updating to:
- cosmos-sdk: Latest v0.50.5+ or newer stable version
- grpc: v1.64.1 or newer
- grpc-gateway: Consider upgrading to v2.x series
🔗 Analysis chain
Verify dependency versions for security and compatibility.
Some dependencies might have known vulnerabilities or compatibility issues.
Let me check the versions of other dependencies as well to ensure a comprehensive security review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for known vulnerabilities in direct dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/cosmos/cosmos-sdk") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 2959
Script:
#!/bin/bash
# Check versions of other critical dependencies
rg -A1 "require \(" go.mod
rg "^require " -A20 x/benchmark/go.mod
# Check for any security advisories for gRPC
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "google.golang.org/grpc") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 3277
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
simapp/app_config.go (1)
302-315: Document the benchmark generator parameters and their implications.The hardcoded generator parameters need documentation to explain their purpose and impact on the benchmarking process. Additionally, consider making these values configurable through environment variables for flexibility in different testing scenarios.
Add comments to explain each parameter:
{ Name: benchmark.ModuleName, Config: appconfig.WrapAny(&benchmarkmodulev1.Module{ GenesisParams: &benchmarkmodulev1.GeneratorParams{ + // Seed ensures deterministic generation of test data Seed: 34, + // Number of buckets to distribute the data across BucketCount: 1, + // Number of key-value pairs to generate in genesis GenesisCount: 100, + // Mean length of generated keys (bytes) KeyMean: 64, + // Standard deviation of key length KeyStdDev: 12, + // Mean length of generated values (bytes) ValueMean: 1024, + // Standard deviation of value length ValueStdDev: 256, }, }), },Consider making these values configurable:
// At the top of the file const ( defaultSeed = 34 defaultBucketCount = 1 defaultGenesisCount = 100 // ... other defaults ) // Use environment variables with defaults Seed: getEnvInt("BENCHMARK_SEED", defaultSeed), BucketCount: getEnvInt("BENCHMARK_BUCKET_COUNT", defaultBucketCount), // ... other parameters
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
runtime/module.go(2 hunks)simapp/app_config.go(4 hunks)simapp/v2/app_config.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/app_config.go
🧰 Additional context used
📓 Path-based instructions (2)
simapp/app_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
simapp/app_test.go (1)
197-206: Add test cases for benchmark module removalThe new code that removes the benchmark module from migrations lacks explicit test coverage. Consider adding test cases to verify:
- Successful removal of the benchmark module when present
- Correct behavior when the benchmark module is not in the list
- Verification that the resulting migration order is correct
Example test case:
func TestBenchmarkModuleMigrationRemoval(t *testing.T) { migrations := []string{"auth", "bank", "benchmark", "staking"} expected := []string{"auth", "bank", "staking"} // Test removal when present filtered := removeBenchmarkModule(migrations) require.Equal(t, expected, filtered) // Test idempotency when already removed filtered = removeBenchmarkModule(filtered) require.Equal(t, expected, filtered) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
simapp/app_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
simapp/app_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
|
linting is failing |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tools/benchmark/module/depinject.go (4)
3-13: Consider documenting unsafe package usage rationaleThe use of the
unsafepackage should be documented at the package level, explaining why it's necessary and what precautions users should take.Add a package-level comment:
+// Package module implements the benchmark module for performance testing. +// Note: This package uses unsafe operations for performance-critical string to bytes conversion. +// See unsafeStrToBytes function for details and warnings. package module
15-18: Document the maxStoreKeyGenIterations constantThe purpose and implications of the
maxStoreKeyGenIterationsconstant should be documented.const ( ModuleName = "benchmark" + // maxStoreKeyGenIterations defines the maximum number of iterations + // allowed when generating store keys to prevent infinite loops maxStoreKeyGenIterations = 100 )
29-40: LGTM: Well-structured dependency injection setupThe interface and struct definitions follow good design principles:
- Single responsibility interface
- Clear dependency injection pattern
- Proper use of optional field tagging
Consider adding validation for the configuration in a constructor function to fail fast if required fields are missing or invalid.
42-61: Enhance error handling and documentationThe function implementation is solid, but could benefit from:
- More detailed error wrapping
- Better documentation of the "app v2 case" comment
Consider these improvements:
func ProvideModule( in Input, ) (appmodule.AppModule, error) { cfg := in.Cfg kvMap := make(KVServiceMap) storeKeys, err := gen.StoreKeys(ModuleName, cfg.GenesisParams.Seed, cfg.GenesisParams.BucketCount) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to generate store keys: %w", err) } for _, sk := range storeKeys { - // app v2 case + // Register keys only for app v2 which requires explicit key registration if in.Registrar != nil { in.Registrar.RegisterKey(sk) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
tools/benchmark/client/cli/tx.go(1 hunks)tools/benchmark/module/depinject.go(1 hunks)tools/benchmark/module/module.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/benchmark/client/cli/tx.go
- tools/benchmark/module/module.go
🧰 Additional context used
📓 Path-based instructions (1)
tools/benchmark/module/depinject.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
tools/benchmark/module/depinject.go (2)
20-27: LGTM: Module registration follows SDK patterns
The module registration is properly implemented using the standard dependency injection pattern.
65-69:
Add safety measures for unsafe string conversion
The use of unsafe operations requires additional safety measures:
- Add runtime checks to prevent potential segmentation faults
- Consider using safe alternatives where performance is not critical
- Add documentation about thread-safety implications
// unsafeStrToBytes uses unsafe to convert string into byte array. Returned bytes
// must not be altered after this function is called as it will cause a segmentation fault.
+// WARNING: This function:
+// - Is not thread-safe
+// - Returns a byte slice that must never be modified
+// - Should only be used in performance-critical paths
+// Consider using []byte(s) for safety where performance is not critical.
func unsafeStrToBytes(s string) []byte {
+ if len(s) == 0 {
+ return nil
+ }
return unsafe.Slice(unsafe.StringData(s), len(s))
}
github-actions
bot
added
C:server/v2
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/grpc.go (1)
Line range hint
392-392: Handle RPC client error and implement proper cleanupThe RPC client creation error is ignored, and the client is not properly cleaned up. This could lead to resource leaks and hidden errors.
Consider applying this change:
- rpcClient, _ := rpchttp.New(c.cfg.ConfigTomlConfig.RPC.ListenAddress) + rpcClient, err := rpchttp.New(c.cfg.ConfigTomlConfig.RPC.ListenAddress) + if err != nil { + return nil, fmt.Errorf("failed to create RPC client: %w", err) + } + defer rpcClient.Stop()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/benchmark/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
server/v2/cometbft/grpc.go(1 hunks)tools/benchmark/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/benchmark/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
server/v2/cometbft/grpc.go (1)
11-11: LGTM!
The import statement is correctly placed and follows Go naming conventions.
(cherry picked from commit 14c841c) # Conflicts: # .github/dependabot.yml # go.work.example # simapp/app.go # simapp/go.mod # simapp/simd/cmd/testnet.go # simapp/v2/go.mod # tests/go.mod
…22851) Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Introduces a benchmark module to test store performance. It can be used to holistically test the end to end performance of a node. Given an initial configuration tools/benchmark provides:
InitGenesisdistributed acrossnstorekeys, e.g. 20M keys across 5 store keysClient invocation looks like:
On exit it dumps the generator state so that running again should still be in sync. It assumes that any transaction accepted by the network was processed, which may not be the case, so miss rate will probably increase over time. This isn't really a problem for tests.
Obviously this module is built to DOS a node by testing the upper bounds of chain performance; I had massively increase gas limits in tests, so it shouldn't be included by default. It's enabled in simapp and simapp/v2 in this PR though.
closes #750
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Tests
Chores