エラーログ出力middlewareの作成 #30
Conversation
…to feat/14/createErrorHandlingRule
|
#26 がマージされてからレビュー依頼する。 |
TakumaKurosawa
force-pushed
the
feat/28/createLoggerMiddleware
branch
from
1181d35 to
077aedd
Compare
| // エラーログ出力 | ||
| uid, ok := c.Get(AuthCtxKey) | ||
| if !ok { | ||
| log.GetAppLogger().Error(fmt.Sprintf("<error:[Unknown]\n %+v>", err.Err)) | ||
| } else { | ||
| log.GetAppLogger().Error(fmt.Sprintf("<error:[%s]\n %+v>", uid, err.Err)) | ||
| } |
There was a problem hiding this comment.
ログフォーマットは、「<error:[user_id] エラー内容>」という形にしています。
There was a problem hiding this comment.
めっちゃフォーマットの話だけど、[error]: userID: error内容みたいなフォーマットのが馴染み深かったりするのだけど、このフォーマットにした理由とかあったりする？ログファイルを真面目に管理したことなくてなんともいえねえ。。。
スタックトレースで改行挟まる分、<>で囲った方が終わりがわかりやすいとかかなと思ったり。
There was a problem hiding this comment.
あと、zapでログレベルを指定しているなら、フォーマットにerrorって文言を追加する意味があまりなくなってしまう気がする。errorをemergencyと、errorに分けるとかでなければ、[userID] ***って感じで良いと思う！
There was a problem hiding this comment.
確かに、zapのロギングの方でログレベルは出力してくれるから、こっちで [error]: userID: error内容みたいな文言にする必要はなさそうだね！
【変更前の出力】
{"level":"error","ts":1589696159.2012851,"logger":"AppLogger","caller":"middleware/error.go:22","msg":"<[error]Unknown:トークンの有効期限が過ぎています。token has expired.: ID token has expired at: 1588209213>"}
【変更後の出力】
{"level":"error","ts":1589697883.347734,"logger":"AppLogger","caller":"middleware/error.go:22","msg":"<[Unknown]:トークンの有効期限が過ぎています。token has expired.: ID token has expired at: 1588209213>"}
There was a problem hiding this comment.
いまさっき出力自分で確認して思ったんだけど、json形式にしている理由ってあったりする？ログファイルに出力するにしても、stdoutとかに出すにしても、jsonよりconsole形式のがいいかなって思ったんだけど、どうだろう。
consoleの方がテキスト処理(awkとかgrepとか)しやすかったり、読みやすさみたいな点で利点あるかなーって思ったりなどした！
ただconsole形式の出力だと、構造化の部分が結構jsonライクに出力されてしまうから、違和感あるけど。。。
akubi0w1
left a comment
akubi0w1
left a comment
There was a problem hiding this comment.
コメントした！zap、使ったことあるがわからない侍なので、勉強します！
とりあえず、今気になったところだけでも。。。
| // エラーログ出力 | ||
| uid, ok := c.Get(AuthCtxKey) | ||
| if !ok { | ||
| log.GetAppLogger().Error(fmt.Sprintf("<error:[Unknown]\n %+v>", err.Err)) | ||
| } else { | ||
| log.GetAppLogger().Error(fmt.Sprintf("<error:[%s]\n %+v>", uid, err.Err)) | ||
| } |
There was a problem hiding this comment.
めっちゃフォーマットの話だけど、[error]: userID: error内容みたいなフォーマットのが馴染み深かったりするのだけど、このフォーマットにした理由とかあったりする？ログファイルを真面目に管理したことなくてなんともいえねえ。。。
スタックトレースで改行挟まる分、<>で囲った方が終わりがわかりやすいとかかなと思ったり。
| // エラーログ出力 | ||
| uid, ok := c.Get(AuthCtxKey) | ||
| if !ok { | ||
| log.GetAppLogger().Error(fmt.Sprintf("<error:[Unknown]\n %+v>", err.Err)) | ||
| } else { | ||
| log.GetAppLogger().Error(fmt.Sprintf("<error:[%s]\n %+v>", uid, err.Err)) | ||
| } |
There was a problem hiding this comment.
あと、zapでログレベルを指定しているなら、フォーマットにerrorって文言を追加する意味があまりなくなってしまう気がする。errorをemergencyと、errorに分けるとかでなければ、[userID] ***って感じで良いと思う！
|
|
||
| func init() { | ||
| config := zap.NewProductionConfig() | ||
| config.Level = zap.NewAtomicLevelAt(zap.ErrorLevel) |
There was a problem hiding this comment.
これってデバックログ吐く時も適用される感じか...？
そうなるとデバッグでログ挟むときえげつないことになりそう。ログレベルの変更も結構動的にできそうではありそう(あやふや)だけど、あまりやりたくないので、developとconfig分けられそうなら分けた方がいいと思う。
あと、ログレベルの話で、info, warnとかはどう扱うんやろ？って気になりました！
ロギング周り自分もあまりピンときてないので、、、 zap勉強します...！！！！！！！
There was a problem hiding this comment.
この NewAtomicLevelAt()でやっていることは、簡単に言うとそのloggerが出力するログレベルを定義する物で、この引数が zap.ErrorLevel になっていると、tlog.GetAppLogger().Error() 以外の例えば、tlog.GetAppLogger().Info()で出力しようとした物に関しては無視される。
現状1種類（エラーログを出力するためのもの）しかないからわかりにくいと思うんだけど、例えばデバッグ用（debugレベルのみ出力）のロガーだったり、開発者向け用（warnレベルのみ出力）のロガーを別々のインスタンスとして管理したいみたいな要件が出てきたときに簡単に移行できる。
やから、
あと、ログレベルの話で、info, warnとかはどう扱うんやろ？って気になりました！
この質問に対する答えとしても、それぞれの用途に合わせたloggerをappLogger以外に作成していくって言うイメージを持ってもらうと良いかも！
There was a problem hiding this comment.
やっぱそうよなw手元でやってみたらなんも出んかったわw
あと、デバッグ用とかのロガーを別インスタンスで作る場合は、指定レベル以上とかのがいいと思う。~のみ、だとロガーがえらい数になってしまって、ログ挟むのが大変になってしまう気がするので！
pkg/log/logger.go
Outdated
| @@ -0,0 +1,21 @@ | |||
| package log | |||
There was a problem hiding this comment.
パッケージ名logだと標準パッケージとごちゃりそうな気がする、、、
loggerとかのが良いのでは？と思ったりなどした。
There was a problem hiding this comment.
なるほど！
そしたら、terrorと合わせて、tlogっていうパッケージ名にするね！
| } | ||
|
|
||
| // getter | ||
| func GetAppLogger() *zap.Logger { |
There was a problem hiding this comment.
Getterにしたんって何か理由あったりする...？
var appLogger ~~の部分を公開の変数にしちゃえば良いのでは？って思ったんだけども。。。
There was a problem hiding this comment.
公開変数にすることのデメリットとして、どこからでも init()で設定したConfigを書き換えられてしまうというのが考えられて、ログみたいに出力するデータに統一性を持たせたい物や、 init()で初期化処理されている物についてはオブジェクト指向のカプセル化を使って外部からの変更を加えられないようにした方が良いのではないかという理由で今回Getterにしている！
There was a problem hiding this comment.
なるほどー！確かにzapの仕様見た感じ結構柔軟になってるから、Getterのがいいかも！
| authedUserToken, err := fa.client.VerifyIDToken(c, jwtToken) | ||
| if err != nil { | ||
| c.AbortWithError(http.StatusBadRequest, err) | ||
| c.AbortWithError(http.StatusBadRequest, terrors.Wrapf(err, http.StatusUnauthorized, "トークンの有効期限が過ぎています。", "token has expired.")) |
There was a problem hiding this comment.
ロギングとは関係ないですが、JWTパッケージの標準では、JWTトークン有効期限切れの場合は500が返ってしまう仕様だったので、terrors.Wrapfで独自エラーをラップするようにしました！
| func init() { | ||
| config := zap.NewProductionConfig() | ||
| config.Level = zap.NewAtomicLevelAt(zap.ErrorLevel) | ||
| config.DisableStacktrace = true // スタックトレースONにしたい場合はfalseにする |
There was a problem hiding this comment.
そういえばだけど、loggerでスタックトレースしないのって、エラーハンドリングの時にすでにやっているから、って感じの認識で良い？
エラーにスタックトレースの情報が載ってるから、出力はエラー部分でよしなにやってくれる感覚がある。。。
close #28
【zapパッケージを使用した】
使い方など↓
https://qiita.com/emonuh/items/28dbee9bf2fe51d28153