Skip to content
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

[CT-803] Serialization errors in AdapterLogger #5436

Closed
jtcohen6 opened this issue Jul 5, 2022 · 1 comment · Fixed by #5874
Closed

[CT-803] Serialization errors in AdapterLogger #5436

jtcohen6 opened this issue Jul 5, 2022 · 1 comment · Fixed by #5874
Labels
bug Something isn't working logging Team:Adapters Issues designated for the adapter area of the code

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 5, 2022

Two places we've run into this:

To quote @nathaniel-may from dbt-labs/dbt-spark#371 (comment), we should solve this in dbt-core, rather than pre-stringifying all values which might be passed into the AdapterLogger, because:

This works because you're explicitly getting the value's string representation when you are making the log call. The reason the Python standard library doesn't do this, and instead uses the style of the original code is so that that string function is only called for the additional arguments if the log line is actually logged. For example, if you have a ton of debug lines with big values but you're only logging info level messages, those potentially expensive string conversions never actually happen.

The place this should be handled is here in core which was the original intent. If this is not working we should investigate modifying this line in core so that we can avoid calling string functions unnecessarily both to comply with a standard Python logging interface in adapters, and to avoid potential performance issues.

msg = self.base_msg if len(self.args) == 0 else self.base_msg.format(*self.args)

@jtcohen6 jtcohen6 added bug Something isn't working Team:Adapters Issues designated for the adapter area of the code logging labels Jul 5, 2022
@github-actions github-actions bot changed the title Serialization errors in AdapterLogger [CT-803] Serialization errors in AdapterLogger Jul 5, 2022
@nathaniel-may
Copy link
Contributor

nathaniel-may commented Jul 22, 2022

As @jtcohen6 calls out in this comment the event should be serializable via to_dict() which is being called here for json log lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working logging Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants