Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @chatton's task in 3m 15s —— View job

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze code quality, architecture, error handling, security, performance, testing
• Post comprehensive review

Summary

This PR introduces a dual-mode benchmark harness: tests can run against locally-spun-up Docker infrastructure (the existing path) or against pre-deployed external infrastructure (new, via BENCH_ETH_RPC_URL). It also extracts benchmark config into benchConfig, adds a benchmarkResult type for aggregated metrics, and introduces rich span/flowchart rendering from VictoriaTraces.

Overall the architecture is clean and the goal is well-executed. There are a few actionable issues and several nits worth addressing before merge.

Medium Issues

1. TestERC20Throughput ignores benchConfig env vars — hardcoded constants

test/e2e/benchmark/spamoor_erc20_test.go:19-24

The test creates a benchConfig and calls setupEnv(cfg), but uses its own hardcoded constants for numSpammers, countPerSpammer, waitTimeout, and erc20Config["throughput"]. This means BENCH_NUM_SPAMMERS, BENCH_COUNT_PER_SPAMMER, BENCH_THROUGHPUT, and BENCH_WAIT_TIMEOUT have no effect on this test — inconsistent with TestGasBurner which uses cfg.* throughout. Fix this →

2. waitForMetricTarget doesn't respect context cancellation during sleep

test/e2e/benchmark/helpers.go:454-466

for time.Now().Before(deadline) {
    v, err := poll()
    if err == nil && v >= target {
        return
    }
    time.Sleep(2 * time.Second)  // blocks for 2s ignoring context cancellation
}

If the test context is cancelled while sleeping (e.g., test timeout), this loop won't wake up for up to 2 additional seconds. Use a time.NewTimer in a select to be responsive to cancellation. Fix this →

3. Dead code: truncateID defined but never called

test/e2e/benchmark/flowchart.go:289-294

func truncateID(id string) string { ... }

This function is defined but has no callers. Remove it or use it. Fix this →

4. fetchAllSpans and fetchAllRichSpans duplicate HTTP boilerplate

test/e2e/benchmark/traces.go:151-207 and :209-262

Both functions construct the same URL, create the same request, handle the same status check, and configure identical scanner buffers. The only differences are the JSON struct and the output type. A shared fetchLogStream(ctx, url) (io.ReadCloser, error) or generic helper would eliminate ~40 lines of duplication and a potential future divergence. Fix this →

Minor Issues

5. extractHostName allocates a full map per span — hot path inefficiency

test/e2e/benchmark/traces.go:282-293

func extractHostName(line []byte) string {
    var raw map[string]string  // allocates for every span
    if err := json.Unmarshal(line, &raw); err != nil { ... }

This deserializes the entire JSON line (already parsed once into logsqlRichSpan) just to find one non-standard field. For large trace collections this is wasteful. Consider a bytes.Contains pre-check or map[string]json.RawMessage on the original parse.

6. Fragile hardcoded string comparison in result.go

test/e2e/benchmark/result.go:80

if e.Name == r.prefix+" - ProduceBlock avg" {

This reconstructs a derived string at call-site to compare against engineSpanEntries's output. If the suffix format in engineSpanEntries changes, this silently stops matching. Use the same constant (spanProduceBlock) and a shared format function.

7. time.Sleep(3 * time.Second) after spammer creation is fragile

test/e2e/benchmark/gasburner_test.go:55-56

A fixed 3-second sleep is brittle under CI load. The subsequent assertSpammersRunning call only confirms spammers haven't already failed — a slow environment could still report status > 0 while the spammer is still in its startup phase. Consider polling assertSpammersRunning with retries or relying solely on the warmup tx check (which already waits for a meaningful number of sends).

8. Stale "Jaeger" comment

test/e2e/benchmark/spamoor_smoke_test.go:67

// so that the expected tracing spans appear in Jaeger.

The project has migrated to VictoriaTraces. Fix this →

9. envInt silently swallows parse errors

test/e2e/benchmark/config.go:68-77

If a user sets BENCH_NUM_SPAMMERS=two by mistake, it silently falls back to the default with no warning. This is acceptable but t.Logf isn't available at package init time — at minimum document the silent-fallback behaviour in a comment.

10. WaitTimeout is not configurable via env var

test/e2e/benchmark/config.go:46

All other benchConfig fields are env-var-driven, but WaitTimeout is hardcoded to 10 minutes. In external/dedicated environments with different network characteristics, this may need tuning. Consider envDuration("BENCH_WAIT_TIMEOUT", 10*time.Minute).

Architecture Observations

Dual-mode dispatch is clean. The setupEnv → setupLocalEnv | setupExternalEnv dispatch in suite_test.go:60-65 is minimal and easy to follow. The contract for external mode (requires BENCH_PRIVATE_KEY + BENCH_TRACE_QUERY_URL) is clearly guarded with s.Require().NotEmpty.

traceProvider / richSpanCollector interface split is appropriate. The optional upgrade via type assertion (if rc, ok := e.traces.(richSpanCollector)) is idiomatic Go and keeps the interface minimal.

buildTree multiple-root heuristic. flowchart.go:187-212 picks the longest-duration root when multiple spans have no parent in the set. This is a reasonable heuristic but silently drops orphan spans. A comment explaining this trade-off would help future readers.

SetupTest vs SetupSuite for Docker client. Each test in SpamoorSuite calls tastoradocker.Setup — creating a new Docker client and network per test. If tests are run individually this is fine, but if the full suite runs this creates unnecessary setup overhead. Worth considering SetupSuite + TearDownSuite if the Docker client is safe to share.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.20%. Comparing base (abb6b71) to head (87348de).

@@           Coverage Diff           @@
##             main    #3157   +/-   ##
=======================================
  Coverage   60.20%   60.20%           
=======================================
  Files         115      115           
  Lines       11896    11896           
=======================================
  Hits         7162     7162           
  Misses       3923     3923           
  Partials      811      811           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.