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

feat: reload loggers after rotating #5

Merged
merged 1 commit into from
Aug 29, 2016
Merged

feat: reload loggers after rotating #5

merged 1 commit into from
Aug 29, 2016

Conversation

popomore
Copy link
Member

@popomore popomore commented Aug 18, 2016

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

将 egg 功能移过来,rotate 后重新 reload loggers

@codecov-io
Copy link

codecov-io commented Aug 18, 2016

Current coverage is 98.31% (diff: 100%)

Merging #5 into master will increase coverage by 0.12%

@@             master         #5   diff @@
==========================================
  Files             3          5     +2   
  Lines           111        119     +8   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            109        117     +8   
  Misses            2          2          
  Partials          0          0          

Powered by Codecov. Last update b1eaf18...a7cad3a

@atian25
Copy link
Member

atian25 commented Aug 18, 2016

+1

@popomore
Copy link
Member Author

多进程覆盖率有问题


module.exports = agent => {
// reload logger to new fd after rotating
agent.messenger.on('log-reload', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

挂在 agent 上的 logger 也需要 reload

// reload logger to new fd after rotating
agent.messenger.on('log-reload', () => {
agent.loggers.reload('got log-reload message');
agent.loggers.coreLogger.info('[egg-logrotator] agent logger reload: got log-reload message');
Copy link
Member

Choose a reason for hiding this comment

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

agent.coreLogger 有这个getter的

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtyjty99999
Copy link
Member

哦。。这个mr相当于把egg reload的工作交给logrotator做了

@fengmk2
Copy link
Member

fengmk2 commented Aug 19, 2016

嗯,logrotator 的功能是插件提供的,那么就应该插件完成。egg 不需要关注 logrotator 的逻辑。

@@ -43,7 +43,7 @@
"test-local": "egg-bin test",
"cov": "egg-bin cov",
"lint": "eslint --ext js --fix test app config *.js",
"ci": "npm run lint && npm run cov",
"ci": "echo $_ && npm run lint && DEBUG=coffee npm run cov",
Copy link
Member

Choose a reason for hiding this comment

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

那这里其他类库也要这样改么? 还有 egg-init 生成的骨架也要.

Copy link
Member Author

Choose a reason for hiding this comment

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

后面几个 commit 都是测试

@popomore popomore force-pushed the logger-reload branch 2 times, most recently from dbd88f9 to a7cad3a Compare August 19, 2016 15:13
@popomore
Copy link
Member Author

终于过了

@popomore
Copy link
Member Author

@@ -0,0 +1,9 @@
'use strict';

module.exports = app => {
Copy link
Member Author

Choose a reason for hiding this comment

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

这个就是 codecov/patch

@popomore
Copy link
Member Author

这个还没合?

@jtyjty99999
Copy link
Member

这个没问题和掉了?

@popomore
Copy link
Member Author

ping @fengmk2

@fengmk2
Copy link
Member

fengmk2 commented Aug 29, 2016

+1

@fengmk2 fengmk2 merged commit d529a9e into master Aug 29, 2016
@fengmk2 fengmk2 deleted the logger-reload branch August 29, 2016 09:02
@fengmk2
Copy link
Member

fengmk2 commented Aug 29, 2016

@popomore 发版本吧。

@popomore
Copy link
Member Author

  • egg-logrotator@2.1.0

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.

5 participants