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: add logger #68

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

feat: add logger #68

wants to merge 3 commits into from

Conversation

ocavue
Copy link

@ocavue ocavue commented Mar 29, 2021

这个 PR 添加了以下功能

  • 提供了一个默认 logger,将日志打印在 stdout / stderr 中
  • 提供了让用户自定义 logger 的方式
  • 在 getAccessToken 获取不到 token 以及退出时打印日志

@ocavue ocavue marked this pull request as draft March 29, 2021 12:50
@ocavue ocavue marked this pull request as ready for review March 29, 2021 12:54
@xen0n
Copy link
Owner

xen0n commented Mar 29, 2021

建议默认不要提供 logger 实现,有需求的人总会自己写一个,企业微信的功能接入本身就已经需要一定的开发技能储备;也就是说,不需要考虑连 logger 都不会实现的小白用户。

"os"
)

type Logger interface {
Copy link
Author

Choose a reason for hiding this comment

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

这里我只给 Logger 设置了两个方法,目前来看似乎是足够的。但是考虑到 Go 的特点,如果我们未来向这个 interface 中添加更多方法,这就会变成 breaking change。所以我在犹豫是否需要提前添加更多方法(比如 Debug、Warning)。

Copy link
Owner

@xen0n xen0n Mar 29, 2021

Choose a reason for hiding this comment

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

  1. 函数签名建议搞成仿照 fmt.Printf 的形式,这样用起来会简单一些。 既然都是临时解决方案,想了想,只打 string 也没问题
  2. 可以添加也可以不添加,毕竟到时候 bump 主版本号即可,这个 breaking change 不属于那种很难修复的。
  3. 这么搞实质上阻断了使用结构化 logging 的可能性(打出结构化数据而非格式化完的字符串,例如 logrus 等库所倡导的那样),但考虑到 2 的因素,先简单写一写,也不是不可以。

Copy link
Author

@ocavue ocavue Mar 29, 2021

Choose a reason for hiding this comment

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

函数签名建议搞成仿照 fmt.Printf 的形式,这样用起来会简单一些。

我们是不是可以考虑同时实现两种调用 logger 的方法:InfoInfof?比如像 grpclog 这样。从方便 logger 使用者的角度考虑,有时候我们并不想要一个模版字符串作为日志结构的主体。举例来说下面这几种写法都是蛮常见的:

grpclog.Info("message count:", len(msgs))
grpclog.Infof("received %d messages", len(msgs))

毕竟到时候 bump 主版本号即可

考虑到 Go 升级一个库当中的主版本号(不包含 v0 => v1 的升级)还需要全局替换代码中的包名,我觉得我们还是最好一次把事情做好

这么搞实质上阻断了使用结构化 logging 的可能性

有什么更好的建议吗?

@ocavue
Copy link
Author

ocavue commented Mar 29, 2021

@xen0n 我已经将默认的 stdout / stderr 的 logger 实现删除了。根据我对 Go 的理解,如果我不提供一个默认的 logger,那么我好像需要在每次使用 logger 的时候判断 logger 是否存在:

- t.logger.Info("tokenRefresher terminated")
+ if t.logger != nil {
+     t.logger.Info("tokenRefresher terminated")
+ }

出于这个考虑,我给 logger 提供了一个默认的空实现,实现了 Logger interface,但是实际上并不会真正地打印日志。

@ocavue
Copy link
Author

ocavue commented Apr 7, 2021

@xen0n 我想继续推进这个 PR,有什么我可以做的吗?

@xen0n
Copy link
Owner

xen0n commented Apr 7, 2021

@ocavue 这个 issue 没有动静的原因是我自己没有时间,你这边暂时应该不用怎么动

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

Successfully merging this pull request may close these issues.

2 participants