Skip to content
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

Add log configuration #63

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add log configuration #63

wants to merge 7 commits into from

Conversation

yzj0911
Copy link

@yzj0911 yzj0911 commented Jun 29, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #63 (770bffd) into master (949046a) will increase coverage by 1.10%.
The diff coverage is 72.17%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   45.73%   46.83%   +1.10%     
==========================================
  Files          48       48              
  Lines        2683     2797     +114     
==========================================
+ Hits         1227     1310      +83     
- Misses       1374     1394      +20     
- Partials       82       93      +11     
Impacted Files Coverage Δ
log/logger.go 58.47% <72.17%> (+6.39%) ⬆️
buffer/iobuffer.go 77.97% <0.00%> (+0.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@taoyuanyuan
Copy link
Contributor

add testcase

log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
@yzj0911 yzj0911 force-pushed the master branch 2 times, most recently from 0d885dd to e8596e9 Compare August 5, 2022 07:10
@yzj0911 yzj0911 changed the title [wip] Add log configuration Add log configuration Aug 5, 2022
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Show resolved Hide resolved
@yzj0911 yzj0911 force-pushed the master branch 2 times, most recently from ca00f87 to 6410a02 Compare August 9, 2022 06:20
coverage.txt Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger.go Outdated Show resolved Hide resolved
log/logger_test.go Show resolved Hide resolved
}

func (l *Logger) mill() {
if l.roller.MaxBackups != defaultRotateKeep || l.roller.MaxAge != defaultRotateAge || l.roller.Compress {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个判断套件不太对啊。
如果配置成和default 一样的值就不执行,这个行为应该是不对的。
为了保持之前默认不执行的逻辑,是不是新加一个开关比较合适。

@@ -400,3 +400,61 @@ WAIT:
t.Logf("received %d reopens", reopens)
close(l.stopRotate)
}

func TestLogRollerTimeAndCompress(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

测试应该覆盖如下场景:

  1. 配置按时间轮转,Compress与MaxBackup(比如1),那么至少应该有3个文件,第一个文件正常写,第二个文件写的时候,第一个被轮转,第三个写的时候第二个被轮转,第一个被压缩
  2. 同样的情况,没有配置Compress,那第一个应该是被删除
  3. 配置按照大小轮转,其他同1)和2)

@@ -205,6 +209,7 @@ func (l *Logger) start() error {
l.create = time.Now()
}
l.writer = file
l.mill()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里执行mill ,看上去可能会有一些耗时。在这期间这个logger实际上是没有办法正常工作的(后面的l.handler没有执行)。如果这里mill 卡太久,可能会导致日志丢失等情况出现,这是需要考虑的风险。

是否可以考虑将这个mill 做成异步的,可以按照日志路径有锁避免并发冲突?但是不影响start的时候及时可以执行写新日志,可以讨论一下

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants