Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

utility: refactor logging module #224

Closed
wants to merge 11 commits into from
Closed

Conversation

neverchanje
Copy link
Contributor

@neverchanje neverchanje commented Feb 27, 2019

Many fancy stuff is there in logging module. So in this refactor the principle is to make the logging API as simple as possible.
What we did are:

  • Removed all the fancy alternatives (i.e screen_logger, hpc_logger) except for simple_logger, so that the implementation doesn't need to be determined through configuration(logging_factory_name), though it is still pluggable through interface (in case we want to use glog or faster alternative instead, or to mock in unit testing, e.g. collecting the logging messages and verify if they are logged as expected).

  • Decoupled logger from service_engine

See #141

@qinzuoyan
Copy link
Member

这个重构的好处在哪里?以前可以换不同的provider,现在不能了。

@neverchanje
Copy link
Contributor Author

这个重构的好处在哪里?以前可以换不同的provider,现在不能了。

@qinzuoyan hpc logger 之前已经删了,现在只有 screen logger,这个没有用,如果有测试需求可以把 stderr_start_level 调到最低级别。log provider 不应该通过配置做,一个系统应该只有一种实现。

另一个方面,我们之前定的重构目标是把 log 从 service engine 里移到 utility,理论上 log 不应该依赖 engine,不然就只有在 engine 起来以后才能打日志。后面会尝试解藕,让我们即使 service engine 不起来,也可以跑 aio 或者 rpc,这样 pegasus client 就可以做的更轻量。

Copy link
Contributor

@shengofsun shengofsun left a comment

Choose a reason for hiding this comment

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

总的来说,想把log和engine解耦合,其实不太容易。

@@ -159,7 +159,6 @@ struct service_spec
std::string rwlock_nr_factory_name;
std::string semaphore_factory_name;
std::string nfs_factory_name;
Copy link
Contributor

Choose a reason for hiding this comment

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

这个nfs其实也没用了,我忘了删了。下个pr可以删了


// register log flush on exit
bool logging_flush_on_exit = dsn_config_get_value_bool(
"core", "logging_flush_on_exit", true, "flush log when exit system");
if (logging_flush_on_exit) {
::dsn::tools::sys_exit.put_back(log_on_sys_exit, "log.flush");
tools::sys_exit.put_back(log_on_sys_exit, "log.flush");
Copy link
Contributor

Choose a reason for hiding this comment

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

后面可以进一步重构,把sys_exit这个功能单独抽成一个utility:注册一些函数,允许用户在某个时机执行它们,比如说程序退出时。函数执行的顺序按栈来:后注册的先执行,这样符合析构的直觉。

这个功能一般两个地方调用:

  1. dassert的时候,在abort之前要执行一下。
  2. 程序段错误时候,在退出之前也要执行一下。

这么一整:log,程序退出时的挂钩,signal_handle就是三个分开的模块了。现在程序以一种奇妙的方式耦合到了一起。

当前我们需要在sys_exit时候挂的钩子也有两个:

  1. simulator模块,在退出时候打印一下自己的随机数种子。
  2. log模块,在退出的时候做flush。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

下一个 PR 会 review log 模块对 tool-api 代码的依赖

{
static char s_level_char[] = "IDWEF";

uint64_t ts = 0;
if (::dsn::tools::is_engine_ready())
if (tools::is_engine_ready())
Copy link
Contributor

Choose a reason for hiding this comment

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

就是这个地方,让现在的log模块还和engine耦合在一起:我们的时钟是依赖engine的。

如果进一步重构,需要把时钟抽成一个接口,log模块在依赖一个接口。这样才能和engine解耦。

不过最简单的做法是:这个地方索性放开用真实时间,最主要的影响就是simulator的时间打印。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clock 后面会解耦出来,这个在 todo 里


#define dreturn_not_ok_logged(err, ...) \
do { \
if ((err) != dsn::ERR_OK) { \
Copy link
Contributor

@shengofsun shengofsun Mar 1, 2019

Choose a reason for hiding this comment

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

这个地方应该不能这么写。因为如果err是个表达式的话,可能会执行两遍。
很难想象这段代码居然是我写的…… 我总觉得应该是留洋给我搞的,因为我和他专门讨论过这个问题。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

哈哈哈哈哈哈

Copy link
Contributor

Choose a reason for hiding this comment

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

顺手把这个地方改了吧,别的我这边没啥问题。

@shengofsun shengofsun self-requested a review March 1, 2019 15:17
@neverchanje neverchanje deleted the master branch February 28, 2020 02:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants