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

on_warning_callback for source freshness #914

Open
fabiomx opened this issue Apr 17, 2024 · 6 comments
Open

on_warning_callback for source freshness #914

fabiomx opened this issue Apr 17, 2024 · 6 comments
Assignees
Labels
area:config Related to configuration, like YAML files, environment variables, or executer configuration customer request An Astronomer customer made requested this dbt:source Primarily related to dbt source command or functionality good first issue Good for newcomers triage-needed Items need to be reviewed / assigned to milestone
Milestone

Comments

@fabiomx
Copy link

fabiomx commented Apr 17, 2024

It would be good to make the on_warning_callback argument available for the source freshness command since, similarly to the test command, you can configure the severity (see freshness), and you might need to use a callback to send notifications for the "warn_after" cases or to store its results.

It looks like a similar work to the one done for the test command (Creating on_warning_callback arg for better warning management) might be done.

I mention @CorsettiS, as he is the original author of this feature for the test command, and it would be good know his opinion on this.

@dosubot dosubot bot added area:config Related to configuration, like YAML files, environment variables, or executer configuration dbt:source Primarily related to dbt source command or functionality labels Apr 17, 2024
@corsettigyg
Copy link

It would be good to make the on_warning_callback argument available for the source freshness command since, similarly to the test command, you can configure the severity (see freshness), and you might need to use a callback to send notifications for the "warn_after" cases or to store its results.

It looks like a similar work to the one done for the test command (Creating on_warning_callback arg for better warning management) might be done.

I mention @CorsettiS, as he is the original author of this feature for the test command, and it would be good know his opinion on this.

hey @fabiomx . it's been a long time I have implemented this feature so I do not remember in details the issues I faced with the freshness check. if I am not wrong, it is because the source freshness has a very specific warn message that it was tricky to deal with regex and at the time of implementation I was only interested in using this feature for tests so I did not put too much thought on it. Probably you just need to create an equivalent regex to extract it and add a new if statement just like that one

@tatiana
Copy link
Collaborator

tatiana commented Apr 22, 2024

Hi, @fabiomx, this is a very relevant feature! Thanks a lot to @corsettigyg for implementing it for tests!
Would you be interested in contributing to Cosmos with this work?

@tatiana tatiana added good first issue Good for newcomers customer request An Astronomer customer made requested this labels Apr 22, 2024
@corsettigyg
Copy link

@fabiomx @tatiana it is important to mention that on_warning_callback already works for freshness, it just doesn't capture the freshness message. I am in the process of implementing cosmos-dbt in the current company I am working at, and once I am done with that I can work on that feature

@tatiana
Copy link
Collaborator

tatiana commented Apr 22, 2024

That's brilliant, @corsettigyg , thank you very much!

@fabiomx
Copy link
Author

fabiomx commented Apr 22, 2024

That's great, @corsettigyg, thanks!

Regarding "on_warning_callback already works for freshness", please, correct me if I'm wrong, but I understand that a new operator should be created (specific for the source freshness command).

@corsettigyg
Copy link

@fabiomx no need for that. As I have written,
If warnings that are not associated with tests occur (e.g. freshness warnings), they will still trigger the on_warning_callback method above. However, these warnings will not be included in the test_names and test_results context variables, which are specific to test-related warnings.
If no one changed the behavior of my implementation, this remains true. The only difference is that there is no context being passed down for freshness checks, which I could add potentially (although I do not know how useful it would be tbh)

@tatiana tatiana added this to the 1.5.0 milestone May 17, 2024
@tatiana tatiana added the triage-needed Items need to be reviewed / assigned to milestone label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config Related to configuration, like YAML files, environment variables, or executer configuration customer request An Astronomer customer made requested this dbt:source Primarily related to dbt source command or functionality good first issue Good for newcomers triage-needed Items need to be reviewed / assigned to milestone
Projects
None yet
Development

No branches or pull requests

3 participants