Facilitate automatic running of load testing tool - #38
Merged
Merged
Conversation
Commit 609aa8c imported cmd/tx-load-test/gcs in bench_cmd.go but the package directory was never staged, breaking the image build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ecr-push only auto-creates (and the push role only allows) repositories under dev/, stg/, or prd/; a top-level tx-load-test repo would require Terraform. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shaptic
changed the title
6 tasks
There was a problem hiding this comment.
Pull request overview
Automates containerized load-test execution, metrics export, and Soroban state maintenance.
Changes:
- Adds container publishing, GCS metrics uploads, and an all-mode
runcommand. - Adds TTL restoration/extension tooling and compact persisted account ranges.
- Lowers benchmark inclusion-fee bids.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Promotes GCS storage to a direct dependency. |
Dockerfile |
Builds a minimal, unprivileged image. |
.dockerignore |
Restricts container build context. |
.github/workflows/push-image.yml |
Builds and publishes the image. |
cmd/tx-load-test/bench_cmd.go |
Uploads benchmark metrics to GCS. |
cmd/tx-load-test/run_cmd.go |
Runs all benchmark modes sequentially. |
cmd/tx-load-test/root_cmd.go |
Registers new commands. |
cmd/tx-load-test/extend_ttl_cmd.go |
Defines the TTL maintenance CLI. |
cmd/tx-load-test/extendttl/extendttl.go |
Implements TTL classification and maintenance. |
cmd/tx-load-test/extendttl/extendttl_test.go |
Tests TTL logic. |
cmd/tx-load-test/gcs/upload.go |
Implements GCS uploads. |
cmd/tx-load-test/gcs/upload_test.go |
Tests upload behavior. |
cmd/tx-load-test/state/ranges.go |
Implements compact index ranges. |
cmd/tx-load-test/state/ranges_test.go |
Tests range persistence. |
cmd/tx-load-test/state/state.go |
Integrates compact state encoding. |
cmd/tx-load-test/state/soroban_submit.go |
Adds TTL and restore submissions. |
cmd/tx-load-test/state/loader.go |
Adds the TTL runtime phase. |
cmd/tx-load-test/ledger/keys.go |
Adds shared ledger-key builders. |
cmd/tx-load-test/ledger/keys_test.go |
Tests ledger-key builders. |
cmd/tx-load-test/ledger/ledger.go |
Reuses shared key builders. |
cmd/tx-load-test/benchmark/sac_transfer.go |
Lowers benchmark fee bids. |
cmd/tx-load-test/benchmark/tx_builder.go |
Updates heavy-fee documentation. |
cmd/tx-load-test/tools/derive-oz-accounts/main.go |
Supports compact state files. |
cmd/tx-load-test/README.md |
Documents TTL, GCS, and range features. |
cmd/tx-load-test/docs/PLAN.md |
Updates the implementation plan. |
cmd/tx-load-test/setup/soroswap_core_actions.go |
Formatting-only adjustment. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| logger.Infof("loaded state from %s (%d accounts, rpc=%s)", stateFile, len(loaded.Persisted.AccountIndices), loaded.RPCURL) | ||
| return extendttl.Run(ctx, logger, loaded.Live, extendttl.Options{ | ||
| ExtendToLedgers: uint32(extendToDays * extendttl.LedgersPerDay), |
Comment on lines
+605
to
+609
| case !ok || f.data == nil: | ||
| items[i].category = categoryMissing | ||
| case f.liveUntil <= latestLedger: | ||
| items[i].liveUntil = f.liveUntil | ||
| items[i].category = categoryArchived |
Comment on lines
+69
to
+74
| if len(indices)+(end-start+1) > maxDecodedIndices { | ||
| return nil, fmt.Errorf("range[%d] %q expands the index list past the %d-entry limit", i, r, maxDecodedIndices) | ||
| } | ||
| for idx := start; idx <= end; idx++ { | ||
| indices = append(indices, idx) | ||
| } |
Comment on lines
+15
to
+18
| pull_request: | ||
| branches: [load-tester] | ||
| push: | ||
| branches: [main, load-tester] |
Comment on lines
+141
to
+146
| if uploadErr := uploadMetricsIfRequested(logger, metricsGCSURL, cfg.MetricsFile); uploadErr != nil && err == nil { | ||
| // The bench itself succeeded; surface the upload failure as the run | ||
| // error so automated runs (k8s Jobs) alert instead of silently losing | ||
| // the metrics when the pod's volume is reclaimed. | ||
| err = uploadErr | ||
| } |
| if runErr != nil { | ||
| scoped.WithError(runErr).Error("benchmark failed") | ||
| } | ||
| if uploadErr := uploadMetricsIfRequested(scoped, metricsGCSURL, cfg.MetricsFile); uploadErr != nil && runErr == nil { |
|
|
||
| - Use `cobra` for the command tree. | ||
| - Provide a root command `tx-load-test` with subcommands `setup`, `restore`, `bench`, `teardown`, and `sync`. | ||
| - Provide a root command `tx-load-test` with subcommands `setup`, `restore`, `extend-ttl`, `bench`, `teardown`, and `sync`. |
Previously, the union pointers were shared across all rewrites causing an extra two LedgerKeys to get added to most Soroswap transactions. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Shaptic
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Range parsing can hang or derive incorrect accounts, metrics can be overwritten or stale, and PR workflows can publish unreviewed images.
Review effort: Balanced
Findings: 4
Open (8)
"0-9223372036854775807"parses…getLedgerEntriesonly returns entries in current ledger state; an evicted persistent entry is…extendToDaysbefore converting it touint32. A negative value loses its sign during…Uploadsupports an exact object URL as well as a prefix, but this loop reuses the same URL for…runsubcommand, so the plan no longer…
Comment on lines
+80
to
+97
| func parseIndexRange(r string) (int, int, error) { | ||
| lo, hi, isRange := strings.Cut(r, "-") | ||
| start, err := strconv.Atoi(lo) | ||
| if err != nil || start < 0 { | ||
| return 0, 0, fmt.Errorf("invalid start index %q", lo) | ||
| } | ||
| if !isRange { | ||
| return start, start, nil | ||
| } | ||
| end, err := strconv.Atoi(hi) | ||
| if err != nil || end < 0 { | ||
| return 0, 0, fmt.Errorf("invalid end index %q", hi) | ||
| } | ||
| if end < start { | ||
| return 0, 0, fmt.Errorf("end %d is below start %d", end, start) | ||
| } | ||
| return start, end, nil | ||
| } |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Range decoding can hang on overflow, TTL input conversion is unsafe, and pull requests can execute the privileged image-publishing workflow.
Review effort: Balanced
Findings: 4
Open (8)
"0-9223372036854775807"parses…getLedgerEntriesonly returns entries in current ledger state; an evicted persistent entry is…extendToDaysbefore converting it touint32. A negative value loses its sign during…Uploadsupports an exact object URL as well as a prefix, but this loop reuses the same URL for…runsubcommand, so the plan no longer…
This file contains hidden or 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
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.
runcommand that just runs all three modes