Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DM-30996: Implement log4cxx appender which logs to Python logging #55
DM-30996: Implement log4cxx appender which logs to Python logging #55
Changes from 8 commits
faf8b6c
5af06f0
6598c04
011c47f
accbc96
686aa24
4d24fb6
e327c4b
4ca26c6
50c70a7
efc1f92
ab3c920
51d3620
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
"too."? Or "to keys"? Or something else?
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.
I was obviously distracted by something important (squirrels or 🐱), will expand 🙂
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.
I am actually inclined to drop attribute-like access and only keep indexing access. It may be convenient to say
MDC.key
instead ofMDC[key]
in format string but it does not work for keys that do not look like identifiers, and it sort of adds additional requirement on other implementations of MDC dictionary.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.
I'm fine with just doing
**MDC
and having the formatting find the right element. We can always add the.key
later. In python logging I would probably add**MDC
directly into the kwargs in the LoggingAdapter and pull them out of theLogRecord
directly later on.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.
But that exposes you to potential attribute collision.
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.
Yes. If someone sets MDC to
lineno
or something. People can indeed shoot themselves in the foot like that but why would they? I don't think python lets me use a format string of%(extra['LABEL'])s
though so how would I ever include it in a logging format string?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.
Use
style='{'
.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.
Okay. Hmm. So we are okay with a LogFormatter using f-style but all the Loggers using %-style?
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.
Loggers don't have an option for style (would be nice to have but probably hard to implement). Formatter style is set in the same place as format string so we should be OK if we do it consistently.