Repository: apache/airflow
Pull request: Modify SmtpNotifier to accept template with defaults
<!--
 Licensed to the Apache Software Foundation (ASF) under one
 or more contributor license agreements.  See the NOTICE file
 distributed with this work for additional information
 regarding copyright ownership.  The ASF licenses this file
 to you under the Apache License, Version 2.0 (the
 "License"); you may not use this file except in compliance
 with the License.  You may obtain a copy of the License at

   http://www.apache.org/licenses/LICENSE-2.0

 Unless required by applicable law or agreed to in writing,
 software distributed under the License is distributed on an
 "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 KIND, either express or implied.  See the License for the
 specific language governing permissions and limitations
 under the License.
 -->

<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.

Feel free to ping committers for the review!

In case of an existing issue, reference it using one of the following:

related: #ISSUE

How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
closes: https://github.com/apache/airflow/issues/35381

Creating a TemplatedSmtpNotifier class that simplifies sending emails based on the discussion in https://github.com/apache/airflow/issues/35381. This class simply extends from the SmtpNotifier and defaults a number of the required arguments (like email_from, subject, and html_content). The latter two variables are pulled from airflow/settings.py.

On the template itself, I was initially thinking of having at least three distinct templates following https://github.com/apache/airflow/issues/35381#issuecomment-1792108885 but it's simple to add a condition to detect whether we are sending an SLA miss or a failure/success/retry email so I decided on using a single template. Happy to change that approach if the community thinks having distinct templates are cleaner.

Will work on tests after the approach is generally agreed upon.

## Examples

Sample dag used to test emails -

```py
import datetime

from airflow import DAG
from airflow.operators.bash import BashOperator
from airflow.providers.smtp.notifications.smtp import SmtpNotifier
from datetime import timedelta


with DAG(
    dag_id="my_dag",
    start_date=datetime.datetime(2021, 1, 1),
    schedule="*/5 * * * *",
    max_active_tasks=1,
    max_active_runs=1,
    sla_miss_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
):
    BashOperator(
        task_id="task",
        bash_command="sleep 180 & exit 0",
        retries=2,
        sla=timedelta(seconds=30),
        on_success_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
        on_failure_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
        on_retry_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
    )

```

on_failure_callback /  on_success_callback / on_retry_callback - 

<img width="1032" alt="Screenshot 2023-12-14 at 11 41 37 AM" src="https://github.com/apache/airflow/assets/9200263/a36c8a17-edb0-447f-b521-32f07e3edee7">

sla_miss_callback -

<img width="1017" alt="Screenshot 2023-12-14 at 11 42 05 AM" src="https://github.com/apache/airflow/assets/9200263/71686fdb-1de9-44c6-af4b-4c060407f641">


<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).


Problem statement:
Extend Notifier interface
### Body

Notifiers is a really cool feature. I have been using it for a while and it's simplify the use case for writing callable functions for alerts.

But... it has also one big drawback.
When I do:

`BashOperator(task_id="mytask",bash_command="fail",on_failure_callback=SmtpNotifier(...))`

i must implement on my own the logic for the message. In the case of Smtp that would be the `title` and the `html_content` for this message. As you may expect this is "pretty static" and include information about the failing dag_id/task_id, traceback of the error and link to the log. This is why probably most users do.

```
BashOperator(
    task_id="mytask",
    bash_command="fail",
    on_failure_callback=SmtpNotifier(
        subject="The task {{ ti.task_id }}  failed",
        html_content=
        """ 
        "Try {{ti.try_number}} out of {{ti.max_tries + 1}}<br>"
        "Log: <a href="{{ti.log_url}}">Link</a><br>'
        "Host: {{ti.hostname}}<br>"
        'Mark success: <a href="{{ti.mark_success_url}}">Link</a><br>'
        """ 
    )
)
```

This makes the notifier code very very big (same goes for `on_retry_callback` / `on_success_callback`)
Thus some users may decide to subclass the notifier and create `FailureSmtpNotifier`, `SuccessSmtpNotifier`, `RetrySmtpNotifier`

For example:


```
class FailureSmtpNotifier(SmtpNotifier):
    def __init__(
        self,
        to:str,
        from_email='noone@none.com'
    ):
        super().__init(to=to, from_email=from_email, subject='', html_content='')
        
    def notify(self, context):
        task_instance = context["task_instance"]
        self.subject = f"airflow dag {task_instance.dag_id} has failed"
        self.html_content= """ 
                "Try {{ti.try_number}} out of {{ti.max_tries + 1}}<br>"
                "Log: <a href="{{ti.log_url}}">Link</a><br>'
                "Host: {{ti.hostname}}<br>"
                'Mark success: <a href="{{ti.mark_success_url}}">Link</a><br>'
                                        """
        super().notify(context)
```
       
Then on usage user needs to provider only the `to` parameter:

```
 BashOperator(
    task_id="mytask",
    bash_command="fail",
    on_failure_callback=MySmtpNotifier(to="someemail@someone.com")
)
```

but that also is not very nice. It requires users to write this class in their own deployment and have at least 3 subclasses per each notifier.

**My proposal:**

We should provide presets for failure/success/retry for each notifier. Users may use the preset if they want.
To give an example - we have `email_on_success`, `email_on_failure`, `email_on_retry` that users can set in BaseOperator (these existed from the days of airflow 1.8?) their template can be the exact template for `SmtpNotifier` and for other notifiers we can author templates.
The choice between templates may be provided by a simple mode parameter for example `SmtpNotifier(mode=failure, to=...)` or maybe by checking the parameter it was used with `on_failure_callback=SmtpNotifier()` obviously mean mode=failure


### Committer

- [X] I acknowledge that I am a maintainer/committer of the Apache Airflow project.

Hints from issue discussion:
I confirm that most Airflow users create subclasses to add their default values to the official classes, and there are no problems with that, especially for the users who need to customize the notifications

However, for common needs, creating some ready-to-use notifiers in Airflow is a good idea. Still, maybe we should not touch the class `SmtpNotifier ` and instead create a subclass with the mode param that provides the values for the superclass based on the chosen mode.
I understand the ask for email "templates", but don't think a "mode" argument with a limited set of options will suit. For example, for one project I made subclasses such as `Team1SuccessNotifier`, `Team2SuccessNotifier`, etc... People will always want to customize things in their own way so a feature for predefined emails should support any number of options.

How about an argument such as "email_template" that you can point to a file? I don't know if there's a common email template file format though...
I think the ideas of the three of  you should be combined into one :) 


We can have predefined set of templates based on "action" that triggered the notification. And allow users to override them with their own. 

I think we should make a maximum use of the "configurabilty" - the whole notion of notifier is to use something "ready" rather than write your own custom version of it. 


Why don't we simply have a 

`TemplatedSmtpNotifier` that extends SMTPNotifier in the following way:

1) We define three predefined templates for each case
2) We allow the users to override those (how to do it that's a different story - it could be done in local settings for example
3) The templates should be just plain jinja2 templates

I think it would also be useful to add a way to selectively disable/enable sending email based on JINJA or even better `python_callable`  condition. 

I saw recently a user (on our slack I think) asking " how do I send notification email only on the last retry" .  I think this is a valid point - someone would iike maybe to retry 10 times but send the email only every 5 times. And of course we could ask the user to write custom notifier for that (why not) but having a way where you can encode the condition to send the email without having to subclass the notifier would be really useful for DAG authors whou would just want to use it as "black-box". 

Sorry clolsed it by total mistake
> I understand the ask for email "templates", but don't think a "mode" argument with a limited set of options will suit. For example, for one project I made subclasses such as `Team1SuccessNotifier`, `Team2SuccessNotifier`, etc... People will always want to customize things in their own way so a feature for predefined emails should support any number of options.

That will still be supported. My suggestion it to add defaults that are useable for all users. Companies that need specific customizations can still do that with the current mechanisem.
> We define three predefined templates for each case

How do you suggest to use it?
How can user point to a specific template?
> > We define three predefined templates for each case
> 
> How do you suggest to use it? How can user point to a specific template?

I thought about three cases we have  - success/failure/retry and passing information to notifier which case we are currently "Serving. 

But IMHO - we do not need to discuss too much if wa agree on general direction. IMHO lots of details of those kind of changes os really up to the who who takes on the task of implementing it. Quite often i find out that idea should be just hashed out "good enough" to get everyone's "this is a good direction" and then hash out details in PR- especially when it comes to such a "Small" feature. I often feel that you need just a high level direction of such change before writing first PR Draft by somoeone who will actually take an  ownership on implementing it and starts POC and PR. Discussion seeing the code and someone actually commited to do do and lead it to completion - are simply way more productive IMHO. 

In this case I think having a general agreement on:

> Let's have an extension of SMTPNotifier that has predefined templates to handle the three different usages and allows to easily customize the messages in declarative way with easily configurable conditions of sending the notifications.

would be where I would end up a discussion before seeing someone produce a firs draft proposal and ask for comments and feedback,


Happy to give this a shot. Will likely start work on it next week.

Commit message: Add TemplatedSmtpNotifier

Patch to review:
diff --git a/airflow/providers/smtp/notifications/templated_smtp.py b/airflow/providers/smtp/notifications/templated_smtp.py
new file mode 100644
index 0000000000000..e52d931123577
--- /dev/null
+++ b/airflow/providers/smtp/notifications/templated_smtp.py
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from pathlib import Path
+from typing import Iterable
+
+from airflow.configuration import conf
+from airflow.providers.smtp.notifications.smtp import SmtpNotifier
+from airflow.settings import SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH, SMTP_DEFAULT_TEMPLATED_SUBJECT
+
+# Read once and cache - is this desirable? If users edit local settings, their changes will not take effect
+# On the other hand, if we put this in the constructor, then we need to read from disk on each instantiation
+SMTP_DEFAULT_TEMPLATED_HTML_CONTENT = Path(SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH).read_text()
+
+
+class TemplatedSmtpNotifier(SmtpNotifier):
+    """Templated version of the SMTP Notifier.
+
+    Accepts minimal arguments and uses a generic subject line / template that can be overridden
+    by replacing the variable SMTP_DEFAULT_TEMPLATED_SUBJECT or
+    SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH in airflow local settings.
+    """
+
+    def __init__(
+        self,
+        to: str | Iterable[str],
+        **kwargs,
+    ):
+        from_email = kwargs.pop("from_email", conf.get("smtp", "smtp_mail_from"))
+        subject = kwargs.pop("subject", SMTP_DEFAULT_TEMPLATED_SUBJECT).replace("\n", "").strip()
+        html_content = SMTP_DEFAULT_TEMPLATED_HTML_CONTENT
+        super().__init__(from_email=from_email, to=to, subject=subject, html_content=html_content, **kwargs)
+
+
+send_templated_smtp_notification = TemplatedSmtpNotifier
diff --git a/airflow/providers/smtp/notifications/templates/__init__.py b/airflow/providers/smtp/notifications/templates/__init__.py
new file mode 100644
index 0000000000000..13a83393a9124
--- /dev/null
+++ b/airflow/providers/smtp/notifications/templates/__init__.py
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
diff --git a/airflow/providers/smtp/notifications/templates/email.html b/airflow/providers/smtp/notifications/templates/email.html
new file mode 100644
index 0000000000000..e0f78eccf2893
--- /dev/null
+++ b/airflow/providers/smtp/notifications/templates/email.html
@@ -0,0 +1,54 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
+        <meta name="viewport" content="width=device-width">
+    </head>
+<body>
+    <table role="presentation">
+        {% if ti is defined %}
+        <tr>
+            <td>Try:</td>
+            <td>{{ ti.try_number }} of {{ ti.max_tries + 1 }}</td>
+        </tr>
+        <tr>
+            <td>Task State:</td>
+            <td>{{ ti.state }}</td>
+        </tr>
+        <tr>
+            <td>Host:</td>
+            <td>{{ ti.hostname }}</td>
+        </tr>
+        <tr>
+            <td>Log Link:</td>
+            <td><a href="{{ ti.log_url }}" style="text-decoration:underline;">{{ ti.log_url }}</a></td>
+        </tr>
+        <tr>
+            <td>Mark Success Link:</td>
+            <td><a href="{{ ti.mark_success_url }}" style="text-decoration:underline;">{{ ti.mark_success_url }}</a></td>
+        </tr>
+        {% elif slas is defined %}
+        <tr>
+            <td>Dag:</td>
+            <td>{{ dag.dag_id }}</td>
+        </tr>
+        <tr>
+            <td>Task List:</td>
+            <td>{{ task_list }}</td>
+        </tr>
+        <tr>
+            <td>Blocking Task List:</td>
+            <td>{{ blocking_task_list }}</td>
+        </tr>
+        <tr>
+            <td>SLAs:</td>
+            <td>{{ slas }}</td>
+        </tr>
+        <tr>
+            <td>Blocking TI's</td>
+            <td>{{ blocking_tis }}</td>
+        </tr>
+        {% endif %}
+    </table>
+</body>
+</html>
diff --git a/airflow/settings.py b/airflow/settings.py
index 1a38a59ed301c..a1f9e4b4c05f5 100644
--- a/airflow/settings.py
+++ b/airflow/settings.py
@@ -629,6 +629,18 @@ def initialize():
 
 DAEMON_UMASK: str = conf.get("core", "daemon_umask", fallback="0o077")
 
+SMTP_DEFAULT_TEMPLATED_SUBJECT = """
+{% if ti is defined %}
+DAG {{ ti.dag_id }} - Task {{ ti.task_id }} - Run ID {{ run_id }} in State {{ ti.state }}
+{% elif slas is defined %}
+SLA Missed for DAG {{ dag.dag_id }} - Task {{ slas[0].task_id }}
+{% endif %}
+"""
+
+SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH = os.path.join(
+    os.path.dirname(__file__), "providers", "smtp", "notifications", "templates", "email.html"
+)
+
 
 # AIP-44: internal_api (experimental)
 # This feature is not complete yet, so we disable it by default.
diff --git a/setup.py b/setup.py
index ee60c885207ae..9dad27f45e928 100644
--- a/setup.py
+++ b/setup.py
@@ -902,6 +902,8 @@ def parse_config_files(self, *args, **kwargs) -> None:
                 self.package_data["airflow"].append(provider_relative_path)
             # Add python_kubernetes_script.jinja2 to package data
             self.package_data["airflow"].append("providers/cncf/kubernetes/python_kubernetes_script.jinja2")
+            # Add default email template to package data
+            self.package_data["airflow"].append("providers/smtp/notifications/templates/email.html")
         else:
             self.install_requires.extend(
                 [


Code review comments:
diff_hunk: @@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from pathlib import Path
+from typing import Iterable
+
+from airflow.configuration import conf
+from airflow.providers.smtp.notifications.smtp import SmtpNotifier
+from airflow.settings import SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH, SMTP_DEFAULT_TEMPLATED_SUBJECT
+
+# Read once and cache - is this desirable? If users edit local settings, their changes will not take effect
+# On the other hand, if we put this in the constructor, then we need to read from disk on each instantiation
+SMTP_DEFAULT_TEMPLATED_HTML_CONTENT = Path(SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH).read_text()
+
+
+class TemplatedSmtpNotifier(SmtpNotifier):
path: airflow/providers/smtp/notifications/templated_smtp.py
text: @user2:
I really like the idea of templates!

However I think we should try to keep the current user API.
meaning that users should still use `SmtpNotifier`.
Thus when users do:

`MyOperator(on_success_callable=SmtpNotifier(to='myemail@user1.com),...)`
It will use the default template of success unless user overrides it

`MyOperator(on_success_callable=SmtpNotifier(to='myemail@user1.com), template='my_template.html'...)`

Similarly if user does:

`MyOperator(on_failure_callable=SmtpNotifier(to='myemail@user1.com),...)`
It will use the default template of failure unless user overrides it

@author:
@user3-awala , @user4 - what do you think about @user2 's idea? Asking because you both suggested something slightly different in https://github.com/apache/airflow/issues/35381#issuecomment-1791741821 and https://github.com/apache/airflow/issues/35381#issuecomment-1792108885

I'm fine either way.