-
Notifications
You must be signed in to change notification settings - Fork 109
fix: [#847] resolve log file rotation issue in daily logger #1321
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
Conversation
|
|
||
| func (r *Writer) Debug(args ...any) { | ||
| r.instance.WithField("root", r.toMap()).Debug(args...) | ||
| r.instance.WithTime(carbon.Now().StdTime()).WithField("root", r.toMap()).Debug(args...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The time is incorrect when using carbon.SetTestNow(), so it should be reset here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.16.x #1321 +/- ##
==========================================
Coverage ? 67.97%
==========================================
Files ? 216
Lines ? 11613
Branches ? 0
==========================================
Hits ? 7894
Misses ? 3339
Partials ? 380 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 49374df | Previous: 06202a1 | Ratio |
|---|---|---|---|
Benchmark_Debug |
153469 ns/op 43556 B/op 674 allocs/op |
81915 ns/op 23407 B/op 384 allocs/op |
1.87 |
Benchmark_Debug - ns/op |
153469 ns/op |
81915 ns/op |
1.87 |
Benchmark_Debug - B/op |
43556 B/op |
23407 B/op |
1.86 |
Benchmark_Debug - allocs/op |
674 allocs/op |
384 allocs/op |
1.76 |
Benchmark_Info |
155492 ns/op 43407 B/op 672 allocs/op |
82307 ns/op 23358 B/op 384 allocs/op |
1.89 |
Benchmark_Info - ns/op |
155492 ns/op |
82307 ns/op |
1.89 |
Benchmark_Info - B/op |
43407 B/op |
23358 B/op |
1.86 |
Benchmark_Info - allocs/op |
672 allocs/op |
384 allocs/op |
1.75 |
Benchmark_Warning |
161586 ns/op 44701 B/op 688 allocs/op |
83765 ns/op 23403 B/op 384 allocs/op |
1.93 |
Benchmark_Warning - ns/op |
161586 ns/op |
83765 ns/op |
1.93 |
Benchmark_Warning - B/op |
44701 B/op |
23403 B/op |
1.91 |
Benchmark_Warning - allocs/op |
688 allocs/op |
384 allocs/op |
1.79 |
Benchmark_Error |
193183 ns/op 56501 B/op 831 allocs/op |
114249 ns/op 32745 B/op 515 allocs/op |
1.69 |
Benchmark_Error - ns/op |
193183 ns/op |
114249 ns/op |
1.69 |
Benchmark_Error - B/op |
56501 B/op |
32745 B/op |
1.73 |
Benchmark_Error - allocs/op |
831 allocs/op |
515 allocs/op |
1.61 |
Benchmark_Panic |
207290 ns/op 62051 B/op 869 allocs/op |
114612 ns/op 36604 B/op 546 allocs/op |
1.81 |
Benchmark_Panic - ns/op |
207290 ns/op |
114612 ns/op |
1.81 |
Benchmark_Panic - B/op |
62051 B/op |
36604 B/op |
1.70 |
Benchmark_Panic - allocs/op |
869 allocs/op |
546 allocs/op |
1.59 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a log file rotation issue in the daily logger by ensuring that all logging operations consistently use the current time from the carbon package, particularly when time mocking is enabled via carbon.SetTestNow(). This is critical for proper log rotation behavior in testing scenarios.
Key changes:
- Modified all logging methods to explicitly set log entry time using
carbon.Now().StdTime() - Implemented a custom clock for the rotatelogs library that respects mocked time
- Added comprehensive test coverage for daily log rotation across different days
- Refactored import naming for better consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| log/logrus_writer.go | Added carbon import and modified all 12 logging methods to use carbon.Now().StdTime() for consistent time handling |
| log/logger/daily.go | Implemented custom rotatelogsClock type to ensure log rotation respects mocked time when using carbon.SetTestNow() |
| log/logrus_writer_test.go | Refactored import naming for consistency, added new test for daily log rotation, and improved test cleanup assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rotatelogs.WithClock(rotatelogs.NewClock(carbon.Now().StdTime())), | ||
| // When using carbon.SetTestNow(), carbon.Now().StdTime() should always be used to get the current time. | ||
| // Hence, WithLocation cannot be used here. | ||
| rotatelogs.WithClock(NewRotatelogsClock()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using rotatelogs.WithLocation(carbon.Now().StdTime().Location()) instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the annotation said, rotatelogs.WithLocation cannot cover the carbon.SetTestNow() case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is carbon.SetTestNow(), the time may be fake. The location is fixed when using rotatelogs.WithLocation(carbon.Now().StdTime().Location()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I forgot carbon.SetTestNow.
📑 Description
Closes goravel/goravel#847
This pull request improves the logging system by ensuring that all log entries consistently use the current time from the
carbonpackage, which is especially important when using time mocking (e.g.,carbon.SetTestNow()). It also adds a new test for daily log rotation and refactors some test and import naming for clarity.Key changes include:
Logging Time Consistency:
logrus_writer.gonow explicitly set the log entry time usingcarbon.Now().StdTime(), ensuring logs reflect the correct (potentially mocked) time.logger/daily.gonow uses a customrotatelogs.Clockimplementation that always returnscarbon.Now().StdTime(), maintaining correct rotation behavior when time is mocked. [1] [2]Testing Improvements:
TestLogrus_DailyLogWithDifferentDaysto verify daily log rotation works correctly across simulated days usingcarbon.SetTestNow().Code and Naming Refactoring:
logrus_writer_test.gofor better clarity and consistency (e.g.,configmock→mocksconfig,logcontracts→contractslog). [1] [2] [3] [4] [5]github.com/dromara/carbon/v2inlogrus_writer.goto support the new time handling.✅ Checks