-
Notifications
You must be signed in to change notification settings - Fork 0
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
ログを構造化ログに変更 #17
ログを構造化ログに変更 #17
Conversation
b9a0cfb
to
19b07e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとう🐱
一個だけコメントしたよ👍
ちなみに
標準のLoggerのようにextraを利用して任意のキーでログに値を出力できない点が、今回作成したLoggerの少し気になるところ...
この点について、許容範囲なのか、それとも問題なのか
これは必要な情報が全て出力出来ているから許容範囲かなと👍
各機能毎にロギングで出力する項目が大きく異なる場合は外から extra
を受け取れたほうが良いけどそれが無さそうであればこれで良いかなと思う👌
src/infrastructure/logging.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppLogger
はDomainでもPresentationでも使うから logger
もしくは log
って独自packageを切っても良いかなと思った🐱
ロギングはどの層にも当てはまらないし、どの層から依存されるか分からないのが大きな理由かな、これならどの層でも気軽にログを使えるから良いかと👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keitakn ありがとう!
確かに、どの層からも利用されることを考えるとlog
パッケージがあった方が良いかと思った!
以下で修正したよ!
現状だとログの出力項目が変わることはないから extra はなくても良さそうだね! |
@keitakn LGTM画像を作成した際に画像の形式をPNGに変換しているのに、画像をアップロードする際の拡張子が |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issueURL
#14
この PR で対応する範囲 / この PR で対応しない範囲
以下について対応する
変更点概要
Loggerを定義し、既存のログ出力箇所を変更。
以下の点が実現できるようのLoggerを作成した。
出力内容: infoの場合
出力内容: errorの場合
レビュアーに重点的にチェックして欲しい点
標準のLoggerのようにextraを利用して任意のキーでログに値を出力できない点が、今回作成したLoggerの少し気になるところ...
この点について、許容範囲なのか、それとも問題なのか判断がついていないから、コメントしてもらえると嬉しい!