-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
WIP: Add hook information to the log #1203
WIP: Add hook information to the log #1203
Conversation
This commit adds logging info for on-run-start/end hooks (Fixes dbt-labs#696)
hey @vijaykiran - thanks for this PR -- it looks like a great start. I think you're right that we will want to make hooks return a I'm also not sure that I love the output shown here (and described in the original issue). I think that for any non-trivial SQL, showing the full statement is going to be overly verbose. Maybe we can do something like:
|
@@ -348,6 +350,8 @@ def run_hooks(cls, config, adapter, manifest, hook_type, extra_context): | |||
dbt.contracts.graph.parsed.Hook(**hook_dict) | |||
|
|||
sql = hook_dict.get('sql', '') | |||
dbt.ui.printer.print_timestamped_line("{} of {} {} ". | |||
format(i+1, hook_count, sql)) |
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.
you can construct a RunModelResult
object here with something like:
res = RunModelResult(hook, status=status)
You can get a status from the adapter.execute
call below. That will return a tuple of a status and an Agate dataframe, as shown here: https://github.com/fishtown-analytics/dbt/blob/dev/stephen-girard/dbt/adapters/base/impl.py#L138
eg:
status, _ = adapter.execute(sql, model_name=model_name, auto_begin=False, fetch=False)
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 think this will work, but I haven't tested it. Let me know if you hit any more snags
@drewbanin Thanks for the feedback!
I agree and I think adding SQL become unreadable quickly. I'll update the code. I'll dig a bit deeper into Results stuff this weekend. |
b451a55
to
262f093
Compare
I'll open a new one with the Results work, thanks for the review @drewbanin ! |
This commit adds logging info for on-run-start/end hooks (Fixes #696)
Here's the sample output.
Adding the hook count to the final line (Finished ...) seems more invasive because as far as I understand, hooks don't return
results
object. @drewbanin any pointers on how to do this in a clean way?