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

can use zap in kitex? #218

Closed
liu-song opened this issue Nov 17, 2021 · 5 comments
Closed

can use zap in kitex? #218

liu-song opened this issue Nov 17, 2021 · 5 comments

Comments

@liu-song
Copy link
Contributor

liu-song commented Nov 17, 2021

Is your feature request related to a problem? Please describe.
zap has good performance in processing log. now,kitex is using klog which is golang native log.

Describe the solution you'd like
if it is suitable, I can try use zap to replace klog in kitex and make a pull request.

Additional context
https://github.com/uber-go/zap , which use sync.Pool to reuse object , use encoder to avoid reflect(fmt.Sprintf(interface))

@YangruiEmma
Copy link
Member

Replace klog with zap?
For api, Kitex won't depend on any log lib, you can use WithLogger to replace log implementation, zap supports printf-style. And it is what we hope users do.
Of course, changing API to promote performance is a good suggestion, but we need to define a new API that can adapt most of the structured log lib, you can give your new log api proposal and we discuss it together. ; )

@liu-song
Copy link
Contributor Author

liu-song commented Nov 22, 2021

@YangruiEmma ,哈喽, 我重新看了一下kitex 中klog被调用的情况,目前kitex 对klog 主要的调用场景是可能出现Warn,Error 的地方,info级别的信息较少,似乎不存在一些热点函数里频繁调用klog的场景 ,且日志被调用的次数相对较少。针对目前而言,感觉上似乎是没有太大必要引入zap? 也看了一下其他开源库(etcd,tidb,dubbo-go等)使用zip来进行封装的log 的一些设计,如果重新设计感觉这里会有更多的细节要讨论,也许你们来做这个功能的完善会更加方便一些。同时,如果kitex目前不打算引入zap 的话,我觉得也许可以先用sync.pool 对https://github.com/cloudwego/kitex/blob/develop/pkg/klog/default.go#L119 来进行一个简单优化。; )

var defaultLogger = &localLogger{
	logger: log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds),
	pool: &sync.Pool{
		New: func() interface{} {
			return new(bytes.Buffer)
		},
	},
}

type localLogger struct {
	logger *log.Logger
	pool   *sync.Pool
}

func (ll *localLogger) logf(lv Level, format *string, v ...interface{}) {
	if level > lv {
		return
	}
	buf := ll.pool.Get().(*bytes.Buffer)
	buf.WriteString(lv.toString())
	if format != nil {
		buf.WriteString(fmt.Sprintf(*format, v...))
	} else {
		buf.WriteString(fmt.Sprint(v...))
	}
	ll.logger.Output(3, buf.String())
	buf.Reset()
	ll.pool.Put(buf)
	if lv == LevelFatal {
		os.Exit(1)
	}
}

@YangruiEmma
Copy link
Member

你好 @liu-song ,谢谢你的提议,klog/default.go其实不是一个建议在线上使用的log实现,klog主要是kitex定义的log api,defaut log用于测试,内部也是有单独的日志库,我们希望使用者通过WithLogger指定其他的log库。可以在 https://github.com/kitex-contrib/ 提供zap的klog扩展实现呀 :)。

@YangruiEmma
Copy link
Member

@liu-song 抱歉,这块我描述错了,部分日志是通过制定logger输出,直接通过klog.Info这样输出日志会执行defaultLogger。这块我们内部会沟通做一些调整,你有优化建议可以直接提pr

@liu-song
Copy link
Contributor Author

@YangruiEmma hi,不知道在是否存在理解上的一些偏差,尝试修改了一下。;)

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

No branches or pull requests

2 participants