chore: using v0.20.0 tag of tastora and removing temporary duplication#3326
Conversation
Replace local spamoor_node.go and victoriatraces_node.go with tastora's spamoor.NewNodeBuilder and victoriatraces.New. Fixes macOS compatibility by replacing 0.0.0.0 with 127.0.0.1 for external endpoints and using UTC timestamps in trace query URLs to avoid broken timezone offsets.
Remove the local filesystem replace directive for tastora in test/e2e/go.mod now that v0.20.0 is published.
📝 WalkthroughWalkthroughThis PR consolidates a Go toolchain upgrade (1.25.7 → 1.25.8) across all modules with a significant refactoring of the benchmark infrastructure. Custom Docker lifecycle implementations for VictoriaTraces and Spamoor nodes are removed and replaced with external library constructors. The suite and trace query methods are updated to handle new URL patterns and resource extraction logic. ChangesBenchmark Infrastructure Upgrade and Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
Claude finished @chatton's task in 1m 10s —— View job Code Review
This is a clean, well-scoped refactoring PR. Removing ~510 lines of duplicated Docker lifecycle code in favor of tastora's upstream implementations is the right call. A few observations below. SummaryThe main changes are:
Issues
for scanner.Scan() {
line := scanner.Bytes()
if len(line) == 0 {
continue
}
attrs := extractResourceAttrs(line)
return &attrs // ← returns even if all fields are empty strings
}
return nil
Positives
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Required by tastora v0.20.0. Aligns root and execution/evm with the other modules already at 1.25.8.
chatton
force-pushed
the
cian/use-tastora-spamoor
branch
from
e5fe0e1 to
779631d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3326 +/- ##
=======================================
Coverage 60.84% 60.84%
=======================================
Files 127 127
Lines 13762 13762
=======================================
+ Hits 8373 8374 +1
+ Misses 4477 4476 -1
Partials 912 912
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/benchmark/traces.go (1)
266-282:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t stop at the first row unless attrs are actually present, and handle scanner errors.
This currently returns on the first non-empty line even if all extracted fields are empty, and it drops
scanner.Err(). That can silently miss valid attrs or hide stream-read failures.Suggested fix
func (v *victoriaTraceProvider) fetchResourceAttrs(ctx context.Context, serviceName string) *resourceAttrs { scanner, body, err := v.fetchLogStream(ctx, serviceName) if err != nil { v.t.Logf("warning: failed to fetch resource attrs: %v", err) return nil } defer body.Close() for scanner.Scan() { line := scanner.Bytes() if len(line) == 0 { continue } attrs := extractResourceAttrs(line) - return &attrs + if attrs != (resourceAttrs{}) { + return &attrs + } } + if err := scanner.Err(); err != nil { + v.t.Logf("warning: failed reading resource attrs stream: %v", err) + } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/benchmark/traces.go` around lines 266 - 282, In victoriaTraceProvider.fetchResourceAttrs, don’t return on the first non-empty line blindly: iterate through all scanned lines from fetchLogStream, call extractResourceAttrs(line) for each, and only return when the resulting resourceAttrs contains actual populated fields (e.g., non-empty service/host/other identifying fields); after the scan loop check scanner.Err() and surface/log that error (instead of dropping it) and ensure body.Close() still runs in defer — this avoids silently missing valid attrs and hides stream-read failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/e2e/benchmark/traces.go`:
- Around line 266-282: In victoriaTraceProvider.fetchResourceAttrs, don’t return
on the first non-empty line blindly: iterate through all scanned lines from
fetchLogStream, call extractResourceAttrs(line) for each, and only return when
the resulting resourceAttrs contains actual populated fields (e.g., non-empty
service/host/other identifying fields); after the scan loop check scanner.Err()
and surface/log that error (instead of dropping it) and ensure body.Close()
still runs in defer — this avoids silently missing valid attrs and hides
stream-read failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fae65c87-3c46-4f66-b2ce-09078898d75a
⛔ Files ignored due to path filters (3)
execution/evm/test/go.sumis excluded by!**/*.sumtest/docker-e2e/go.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
apps/evm/go.modapps/grpc/go.modapps/testapp/go.modexecution/evm/go.modexecution/evm/test/go.modgo.modtest/docker-e2e/go.modtest/e2e/benchmark/spamoor_node.gotest/e2e/benchmark/suite_test.gotest/e2e/benchmark/traces.gotest/e2e/benchmark/victoriatraces_node.gotest/e2e/go.mod
💤 Files with no reviewable changes (2)
- test/e2e/benchmark/victoriatraces_node.go
- test/e2e/benchmark/spamoor_node.go
chatton
changed the title
chatton
changed the title
2 participants
Overview
There was some temporary duplication of code that lives in tastora due to it not being included in a correct tag, this is now included and can be removed.
Summary by CodeRabbit