-
Couldn't load subscription status.
- Fork 1.1k
Logging improvements #1004
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
base: master
Are you sure you want to change the base?
Logging improvements #1004
Conversation
|
Anyone? |
|
Did you see my review comments? |
Hi @Josverl , I do not see any comment made by anyone else but me in this ticket. I would love to review any comment, can you please refer me where? Thanks, |
|
|
||
| class LogRecord: | ||
| def set(self, name, level, message): | ||
| def __init__(self, name, level, message, extra=None): |
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.
If the intend of extra is to provide the same functionality as CPythons LogRecord( ..., args, ...)
,then it makes sense to name it the same. That makes it simpler to learn.
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.
Hi,
There is a inconsistency between LogRecord[1] and Logger.makeRecord[2].
It is "unknown" how extra reaches the LogRecord once constructed.
I can make extra as property which is also not fully compatible.
I can have setExtra().
I do not think this is that critical how we pass the extra into the record, as the interface is clearly not compatible, for example, we do not have makeRecord or event population between loggers.
If you have a better notation, please let me know.
Thanks!
[1] https://docs.python.org/3/library/logging.html#logging.LogRecord
[2] https://docs.python.org/3/library/logging.html#logging.Logger.makeRecord
| self.ct = time.time() | ||
| self.msecs = int((self.ct - int(self.ct)) * 1000) | ||
| self.asctime = None | ||
| if extra is not None: |
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.
can be simplified to if extra:
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.
Correct, but if you look closely in this source the pattern is to test for explicit None without having empty additional path. If it is important I will replace.
|
Hi Alon, |
Thanks! I will create an example. |
Extra context is usable to enrich log record with concrete context additions. Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
The keyword parameters are populated to common log and exc_info should be common to all methods anyway. This change the default to be exc_info=False for all cases similar to the standard python. Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
logging: Support extra context for LogRecord.
Extra context is usable to enrich log record with concrete context
additions.
logging: move exc_info to common log.
The keyword parameters are populated to common log and exc_info should be
common to all methods anyway.
This change the default to be exc_info=False for all cases similar to the
standard python.