chore: change log levels #3333
chore: change log levels #3333
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code ReviewThis PR systematically adjusts logging levels across the codebase, primarily changing ✅ Positive Aspects
🔍 Areas of Concern1. Critical Event Visibility
|
MasterPtato
force-pushed
the
11-03-chore_misc_bug_fixes_add_logs_for_outbound_req
branch
from
564f956 to
58f931d
Compare
MasterPtato
force-pushed
the
11-03-chore_change_log_levels
branch
from
64217ec to
9c1bfa7
Compare
PR Review: Log Level AdjustmentsSummaryThis PR adjusts logging levels across multiple packages in the Rivet engine, primarily converting Code Quality & Best Practices ✅Strengths:
Minor Observations:
Specific Changes Worth NotingEPaxos Protocol Logs (Multiple files in
Gasoline Workflow Engine (
Service Manager (
Tracing Reconfigure (
Potential Issues 🔍No critical issues identified, but a few considerations:
Performance Considerations ✅
Security Concerns ✅No security concerns identified. The changes only affect logging levels, not application logic. Test CoverageQuestion: Are there any tests that assert on specific log messages at cargo test -- --nocapture 2>&1 | grep -i "assert.*log\|expect.*log\|verify.*log"Recommendations
ConclusionOverall Assessment: ✅ Approved with minor suggestions This is a well-executed cleanup PR that improves production log quality by moving verbose operational logs to debug level. The changes are consistent, preserve important information, and follow project conventions. The specific improvements to EPaxos, Gasoline, and service manager logs are particularly valuable for reducing noise in production environments. Suggested follow-ups (not blockers):
Great work on improving the logging hygiene! 🎉 |
Code ReviewThank you for this PR! I've reviewed the changes to standardize log levels across the codebase. Here's my feedback: OverviewThis PR systematically changes log levels from Positive Aspects
Concerns and Suggestions1. Sleep Duration Warning Threshold Change (
|
NathanFlurry
force-pushed
the
11-03-chore_change_log_levels
branch
from
9c1bfa7 to
e78aa83
Compare
NathanFlurry
force-pushed
the
11-03-chore_misc_bug_fixes_add_logs_for_outbound_req
branch
from
58f931d to
867707c
Compare
PR Review: Log Level ChangesSummaryThis PR adjusts logging levels across the codebase, primarily downgrading Positive Observations
Issues & Concerns1. Critical: Potential Loss of Important Operational VisibilitySeveral downgraded logs may be important for production operations: Service Manager (
Workflow Worker (
Epoxy Configuration (
2. Inconsistency: Selective DowngradesSome similar operations have different log levels:
3. Code Style: Adheres to CLAUDE.md Guidelines ✅Per the project's logging patterns, the structured logging format changes are correct:
4. Specific Concern: Sleep Duration Warning Threshold
-if !replay && duration < -50 {
+if !replay && duration < -25 {
5. Removed Logs Without Replacement
Recommendations
Testing CoverageNo test changes included. Consider:
Security ConcernsNone identified - purely logging changes. Performance ImpactPositive - reducing log volume should improve performance slightly, especially in high-throughput paths like EPaxos message handling. VerdictThe PR is generally well-intentioned and improves log hygiene, but please address:
Overall: Approve with minor suggestions ✅ The core changes are sound, but some specific downgrades may need reconsideration based on operational requirements. |
PR Review: Log Level ChangesThanks for this PR @MasterPtato! I've reviewed the changes and have the following feedback: ✅ Positive Aspects
📊 Key Changes Summary
🔍 Observations & Suggestions1. Sleep Duration Warning Threshold Change (gasoline/src/ctx/workflow.rs:1054)-if !replay && duration < -50 {
+if !replay && duration < -25 {Question: This changes the warning threshold from -50ms to -25ms for negative sleep durations. This makes the warning more sensitive (will trigger more often). Was this intentional? If so, it would be helpful to understand the reasoning in the PR description. 2. Critical Path Logging (epoxy/src/http_client.rs)The changes downgrade several HTTP message sending logs from
Consideration: These are core consensus protocol operations. While I understand the desire to reduce log noise, consider whether you want visibility into message passing for debugging distributed consensus issues in production. You might want to keep some of these at 3. Removed Service Lifecycle Logs (cache-purge/src/lib.rs)-tracing::info!("starting cache purge subscriber service");
// ...
-tracing::warn!("cache purge subscriber service stopped");Suggestion: While the "starting" message was redundant (there's already a subscription log), the "stopped" message could be valuable for diagnosing unexpected service terminations. Consider keeping it at 4. Service Manager Warning (gasoline/src/worker.rs:155-158) if wf.stop.send(()).is_err() {
tracing::warn!(
?workflow_id,
- "stop channel closed"
+ "stop channel closed, workflow likely already stopped"
);
}Good change: The improved message provides better context for this expected race condition. 5. Performance Impact✅ Positive: Downgrading verbose logs to 🐛 Potential IssuesNone identified. The changes are mechanical and don't alter control flow or logic. 🔒 Security ConcernsNone identified. The changes don't affect authentication, authorization, or data handling. 🧪 Test CoverageNote: This PR doesn't include test changes, which is appropriate since it only modifies logging. However, you might want to:
📝 Recommendations
✨ Overall AssessmentThis is a solid cleanup PR that will improve log quality by reducing noise. The changes are mechanical, safe, and follow established patterns. The main suggestion is to add context in the PR description and reconsider a few strategically important logs. Recommendation: ✅ Approve with minor suggestions - The changes are good, just add some context to the PR description. Generated with Claude Code |
NathanFlurry
force-pushed
the
11-03-chore_change_log_levels
branch
from
e78aa83 to
f41d8a5
Compare
NathanFlurry
force-pushed
the
11-03-chore_misc_bug_fixes_add_logs_for_outbound_req
branch
from
867707c to
f73435d
Compare
Code Review - PR #3333: Change Log LevelsSummaryThis PR makes systematic changes to logging levels throughout the codebase, primarily converting Strengths
Issues & Concerns1. Potentially Important Logs Demoted (Minor)Some demoted logs might still be valuable at
Recommendation: Consider keeping these at 2. Sleep Duration Warning Threshold Change (Needs Clarification)
-if !replay && duration < -50 {
+if !replay && duration < -25 {This changes the threshold for warning about negative sleep durations from 50ms to 25ms. This will produce more warnings for smaller timing discrepancies. Questions:
3. Missing Context in Commit MessageThe commit message "chore: change log levels" doesn't explain:
Recommendation: Add more context to help future maintainers understand the rationale. 4. Worker Stop Channel Warning (Good Fix)
Code Quality✅ Follows CLAUDE.md conventions:
✅ No functionality changes (except the sleep warning threshold) ✅ No security concerns ✅ No performance impact Test Coverage
Recommendations
VerdictApprove with minor suggestions ✅ This PR improves the signal-to-noise ratio in production logs. The changes are safe and follow good practices. The sleep threshold change should be clarified, but the overall direction is sound. Generated by Claude Code - Review based on CLAUDE.md conventions and Rust best practices |
PR Review: Log Level ChangesOverall AssessmentThis PR makes systematic changes to adjust log levels across the codebase. The changes are consistent and follow a clear pattern. Overall, this is a clean refactoring that improves observability by reducing log noise. Positive Observations✅ Consistent Pattern: The PR consistently moves verbose operational logs from
✅ CLAUDE.md Compliance: The changes follow the structured logging convention correctly:
✅ Removes Redundant Logs: Appropriately removes service start/stop logs in ✅ Improves Message Clarity: In Areas of Concern
// Changed from:
runner_name = ?path.runner_name
// To:
runner_name=?path.runner_name // Missing space around =This is inconsistent with the rest of the codebase which uses
if !replay && duration < -25 { // Was -50
tracing::warn!(%duration, "tried to sleep for a negative duration");
}Question: Was this an intentional tightening of the tolerance, or related to the log level changes? This seems orthogonal to the log level refactoring. Consider explaining this change in the PR description or commit message. Impact AnalysisPerformance: ✅ Positive - Reducing log verbosity will improve performance in production by reducing I/O and serialization overhead. Observability: ✅ Positive - Moving routine operational logs to debug level reduces noise while keeping important information available when needed. Security: ✅ No concerns - No security-sensitive data is being exposed or hidden. Test Coverage: ℹ️ N/A - Log level changes don't require test coverage changes. Recommendations
VerdictApprove with minor suggestions - This is a solid cleanup that improves the signal-to-noise ratio in production logs. The inconsistency and undocumented threshold change are minor issues that should be addressed, but they don't block the PR. 🤖 Generated with Claude Code |
Merge activity
|
No description provided.