-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Minor Cleanup of Structured Logging Module #4266
Conversation
6252d49
to
d4bf338
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.
Love how much cleaner it is.
ab25bfe
to
c5ca29f
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.
The adapter fixes you implemented made me realize we should have generic adapter types, similar to what's in test_types.py
for new adapters(or anyone inclined to make more updates than the few lines). Seems off to have new adapters use the old logging format.
core/dbt/events/adapter_endpoint.py
Outdated
|
||
fire_event(event) | ||
|
||
|
||
def or_none(x: dict, key: str) -> Optional[Any]: |
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.
There's a builtin for this:
https://docs.python.org/3/library/stdtypes.html#dict.get
# this class shouldn't be createable, but we can't make it an ABC because of a mypy bug | ||
if type(self).__name__ == 'AdapterEventBase': | ||
raise Exception( | ||
'attempted to create a message for AdapterEventBase which cannot be created' |
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.
Love the more explicit exception msg!
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.
Just swap to using the built-in get method on those dicts and 👍
@emmyoop if I'm understanding you correctly, I think that's exactly what we have already with the subclasses of |
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.
LGTM!
* cleanup structured logging module * update adapter logger to preserve new-style logging formatting automatic commit by git-black, original commits: 719b202
Description
1 - Cleaned up some of the code.
2- Fixed the adapter interface in here since post-cleanup it wasn't type checking. In the new system, the following logger call would have thrown a runtime exception before this change but is almost certainly present in the wild since it was found in our adapter tests:
logger.info("ran this sql: {}", sql)
Rather than re-implement every way a python logger could be called, I just make the std lib logger construct the message component of the log line and hand it back with a stringio buffer.<- this strategy did not work because the std lib logger does not use new-style string formatting, only old-style.I dug into some logger libraries and found that this string formatting is really the only thing that
*args
is doing. Python prefers it this way so it can be done lazily which we are already doing via a function. I apply these arguments with new-style formatting viastr::format
to maintain compatibility with the former logbook logger.Checklist
CHANGELOG.md
and added information about my change