-
Notifications
You must be signed in to change notification settings - Fork 129
chore(scripts): tee run logs to file #3314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(scripts): tee run logs to file #3314
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: chore(scripts): tee run logs to fileSummaryThis PR adds log file output to the engine run scripts using Positive Points
Issues & Suggestions1. Log File Naming CollisionSeverity: Medium Both scripts write to the same log file Recommendation: # engine-postgres.sh
cargo run --bin rivet-engine -- start "$@" | tee /tmp/rivet-engine-postgres.log
# engine-rocksdb.sh
cargo run --bin rivet-engine -- start "$@" | tee /tmp/rivet-engine-rocksdb.log2. Consider Append ModeSeverity: Low Using cargo run --bin rivet-engine -- start "$@" | tee -a /tmp/rivet-engine-postgres.logThough overwrite mode is also valid if you only care about the most recent run. 3. Stderr Not CapturedSeverity: Low Error output (stderr) is not being captured in the log file, only stdout. Consider using: cargo run --bin rivet-engine -- start "$@" 2>&1 | tee /tmp/rivet-engine-postgres.logThis ensures both stdout and stderr are captured, which is particularly important for debugging errors. 4. Minor: Trailing NewlineSeverity: Very Low The Code Quality: ✅ Good
Security: ✅ No concerns
Performance: ✅ No impact
Test Coverage: N/A
RecommendationApprove with suggestions. The core change is solid. I recommend addressing issue #1 (naming collision) before merging. Issues #2 and #3 are optional improvements that could be addressed now or in a follow-up. |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
26c768a to
ca84581
Compare
e58c8b0 to
3751863
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Pull Request ReviewSummaryThis PR adds log file output to the engine run scripts by piping output through tee to /tmp/rivet-engine.log. This allows developers to both see logs in the terminal and have them saved to a file for later inspection. Code Quality - Positive
Potential Issues and Recommendations1. File Path Collision Recommendation: Consider using distinct filenames like /tmp/rivet-engine-postgres.log and /tmp/rivet-engine-rocksdb.log, or include a timestamp for each run to preserve history. 2. stderr Not Captured Recommendation: Use 2>&1 to capture both stdout and stderr. 3. Log File Growth 4. Documentation Security ConsiderationsNo security concerns. Using /tmp for development logs is appropriate. Performance ConsiderationsMinimal impact. tee is efficient and the I/O overhead is negligible for log output. Overall AssessmentThis is a useful improvement for debugging and development workflows. The implementation is clean and simple. Addressing the file collision and stderr capture issues would make it even more robust. Recommendation: Approve with minor suggestions for improvement. |
ca84581 to
cf3b3e3
Compare
3751863 to
5e3dd14
Compare
Code ReviewSummaryThis PR adds log file output via Positive Aspects✅ Simple, focused change that adds value Concerns & Recommendations1. Log File Conflict
|
Merge activity
|
5e3dd14 to
7549ac0
Compare
cf3b3e3 to
620ef31
Compare
PR Review: Tee run logs to fileThis PR adds log file output to the engine startup scripts using tee, allowing logs to be both displayed in the terminal and written to /tmp/rivet-engine.log. Positive Aspects:
Suggestions:
Verdict: Useful quality-of-life improvement. Approve with optional enhancements for future iteration. |
PR Review: chore(scripts): tee run logs to fileSummaryThis PR adds log file output to the engine run scripts by piping the output through tee to save logs to /tmp/rivet-engine.log. This aligns well with the testing guidelines in CLAUDE.md that recommend piping test output to files for easier analysis. Code Quality
Potential Issues & Considerations1. Log File CollisionBoth scripts write to the same log file (/tmp/rivet-engine.log). If a user runs both scripts (or runs one after another), the second invocation will overwrite the first log file. Consider:
2. Error Stream HandlingThe current implementation only captures stdout. stderr is not being captured to the log file. Consider adding 2>&1 before the pipe to capture both stdout and stderr, which is crucial for debugging issues. 3. Exit Code PreservationWhen using tee in a pipeline, the exit code of cargo run might be masked. However, this is already handled correctly by the existing set -euo pipefail at the top of both scripts. 4. Disk Space ConsiderationsLong-running engines with verbose logging could generate large log files in /tmp/. This is generally fine for development scripts, but consider documenting this behavior. 5. Whitespace ConsistencyMinor: engine-rocksdb.sh adds an extra blank line at the end. This is cosmetic but could be cleaned up for consistency. RecommendationsHigh Priority:
Low Priority: VerdictThis is a useful quality-of-life improvement for debugging. With the suggested stderr capture and unique filenames, this will be even more robust. The change is functional as-is for basic use cases. |

No description provided.