Creating on_warning_callback arg for better warning management

[CorsettiS]

In airflow, errors are handled by default with callback functions and/or email alerts, however when we are integrating DBT with airflow, there is an extra case to consider: warnings
When running tests against a dataset, it would be really interesting to also add the possibility to communicate the WARN messages that got triggered in some way, since so far the WARNs can only be found inside the logs, which is no different from a blackbox for companies dealing with many DAGs. We should communicate if a warn is observed in the logs in a more transparent way.

To accomplish that, we can change the DbtTestOperator by adding 1 extra param to it

on_warning_callback (default = None)

(Updating after suggestion)

[jlaneve]

Hey @CorsettiS, this is an amazing idea! I think it may be a bit too opinionated in terms of how we're handling the behavior when there's a warning. What do you think about the following:

• instead of baking the send_slack_alert function directly into the package, what if we make a more general on_warning_callback? Or even an on_completion_callback where we pass in context about the dbt output and let the user decide what they'd like to do with it
• add documentation about how to use the callback to send alerts on failure - that's where we can provide the code snippet on how to send a message in slack, for example

I think that'd be helpful to extend this functionality past slack - maybe someone wants to use the same pattern to send an email, etc.

What do you think?

[CorsettiS]

Hey @CorsettiS, this is an amazing idea! I think it may be a bit too opinionated in terms of how we're handling the behavior when there's a warning. What do you think about the following:

• instead of baking the send_slack_alert function directly into the package, what if we make a more general on_warning_callback? Or even an on_completion_callback where we pass in context about the dbt output and let the user decide what they'd like to do with it
• add documentation about how to use the callback to send alerts on failure - that's where we can provide the code snippet on how to send a message in slack, for example

I think that'd be helpful to extend this functionality past slack - maybe someone wants to use the same pattern to send an email, etc.

What do you think?

hey @jlaneve I really liked the idea of creating a on_warning_callback. Guess I was too focused on my specific problem with slack. I will check during the week how feasible it is to implement it, adding some documentation on the way !

[CorsettiS]

Since the image will only be rendered once the PR is merged, I will attach here an image of how the documentation would look like with it.

Update (2022-04-08):

• I have run many tests and so far everything worked
• Recently improved the syntax to increase code performance

@jlaneve I am done on this side. If there is anything you would recommend me to change, I am all ears.

Update (2022-04-13):

• I have merged the latest commits (including the Docker & K8s versions).
• For the Local executor, the changes are working fine. I could not create the test env locally for the docker & k8s ones tho, which made it not possible for me to create the proper adaptions. Furthermore, I feel that even if I could test locally, I would have to tackle the problem in a total different way due to the way the new operators work. As of now, I will leave this feature available only for the Local execution mode then. If anyone feels interested in adapting the code, feel free to do it, otherwise I may look it up in a future date to adapt it to the other execution modes

[codecov]

Codecov Report

Patch coverage: 56.31% and project coverage change: -2.34 ⚠️

Comparison is base (400c01a) 79.76% compared to head (07ff0d3) 77.42%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
- Coverage   79.76%   77.42%   -2.34%     
==========================================
  Files          28       30       +2     
  Lines        1003     1094      +91     
==========================================
+ Hits          800      847      +47     
- Misses        203      247      +44     

Impacted Files	Coverage Δ
...providers/dbt/core/utils/adapted_subprocesshook.py	30.23% <30.23%> (ø)
cosmos/providers/dbt/core/operators/local.py	48.50% <40.90%> (-2.78%)	⬇️
cosmos/providers/dbt/core/utils/warn_parsing.py	92.85% <92.85%> (ø)
cosmos/providers/dbt/core/operators/docker.py	69.76% <100.00%> (+0.35%)	⬆️
cosmos/providers/dbt/core/operators/kubernetes.py	72.04% <100.00%> (+0.30%)	⬆️
cosmos/providers/dbt/dag.py	75.00% <100.00%> (ø)
cosmos/providers/dbt/render.py	94.56% <100.00%> (+0.05%)	⬆️
cosmos/providers/dbt/task_group.py	75.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jlaneve]

just one more small change - then this is good to merge! thank you for iterating on this