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

Alerting: Preserve new-lines from custom email templates in rendered email #52253

Merged
merged 2 commits into from
Jul 19, 2022

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Jul 14, 2022

What this PR does / why we need it:

Right now, any customized email messages from alerting display with the newlines destroyed:

image

This is because the newlines aren't changed to <br>'s, nor is any stying applied to that block aside from the parent styling.

Which issue(s) this PR fixes:

This PR adds some simple styling to this message block (if it's present) that preserves newlines transparently. Now, newlines from the Alerting template show up in the actual email by default, completely as the user would expect.

image

rel #42946

Fixes #52310

Special notes for your reviewer:

There are a lot of unrelated changes in this PR. Essentially, several changes have gone into the code-generated email templates that were implemented by hand. So, if I run the template generator, all these changes get un-done (!!!)

I dug through the git history and found every such PR, and hand-recreated them in the generator side. Ran the generator and now everything is back in alignment!

There are still a few minor changes that the generator added, which are all ones that we actually do want to preserve or have no effect. These include:

  • Whitespace produced by the generator
  • border -> border-width changes
  • Some inherited class names (whose contents are inlined, so this doesn't affect anything)

@alexweav alexweav added this to the 9.1.0 milestone Jul 14, 2022
@@ -101,7 +116,7 @@
[[ end ]]

[[ if gt (len .Message) 0 ]]
[[ .Message ]]
<span style="white-space: pre-line;">[[ .Message ]]</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only real change that relates to this PR. The rest of the changes are recreations of past PRs, in the generator.

@alexweav alexweav requested a review from a team July 14, 2022 16:45
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but how do we make sure this stays up to day always?

@grafanabot
Copy link
Contributor

@alexweav
Copy link
Contributor Author

@gotjosh The typical way of doing this is to never check generated code into git. Instead the makefile should do the generation, and developers are essentially forced to make the change in the right place.

However, in this particular case, this is unusually difficult. This generator has a lot of unusual dependencies that are not typical for the grafana project. It requires an installation of Ruby, etc. IMO it's unreasonable to suddenly make local builds of Grafana require an entirely new programming language just for this, so I did not go down this route in this PR.

@joeblubaugh
Copy link
Contributor

To keep things up to date, we could add a CI step that runs the generator and errors if there’s a difference.

@alexweav what do you think about left-aligning the lines?

Copy link
Member

@JacobsonMT JacobsonMT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think I agree with @joeblubaugh on left aligning the lines, but either way it's a good improvement.

@alexweav alexweav added backport v9.0.x and removed no-backport Skip backport of PR labels Jul 18, 2022
@alexweav alexweav modified the milestones: 9.1.0, 9.0.4 Jul 18, 2022
@alexweav
Copy link
Contributor Author

After trying it, definitely agree with @joeblubaugh and @JacobsonMT, left aligning was a great suggestion and it looks even better. Thanks!

@alexweav
Copy link
Contributor Author

Holding off on the CI step or anything of that nature for now, to keep this PR targeted.

@grafanabot
Copy link
Contributor

@alexweav alexweav merged commit 39d7fdb into main Jul 19, 2022
@alexweav alexweav deleted the alexweav/tmpl-line-break branch July 19, 2022 16:05
grafanabot pushed a commit that referenced this pull request Jul 19, 2022
…email (#52253)

* Add line break, regenerate, and fix

* Left-align custom text in addition to preserving newlines

(cherry picked from commit 39d7fdb)
alexweav added a commit that referenced this pull request Jul 19, 2022
…email (#52253) (#52470)

* Add line break, regenerate, and fix

* Left-align custom text in addition to preserving newlines

(cherry picked from commit 39d7fdb)

Co-authored-by: Alexander Weaver <weaver.alex.d@gmail.com>
leventebalogh added a commit that referenced this pull request Jul 20, 2022
…nnections-datasources

* origin/main: (36 commits)
  Dashboard: Fix iteration property change triggering unsaved changes warning  (#51272)
  Public Dashboards: Add Public Tag to Dashboard Title (#52351)
  Secrets: Add logging to track secrets migration (#52481)
  Adds documentation for public dashboards under the dashboards section. (#52126)
  Update upgrade-grafana.md (#52406)
  FolderPage: Improve folder page to work with new nav breadcrumbs and modify Dashboard page show path based breadcrumbs"  (#52428)
  Alerting: Rename sender.Sender to sender.ExternalAlertmanagers (#52463)
  CloudWatch: fix log explorer context (#52118)
  Secret migration from Sql KV Store to Secret Plugin (#52191)
  Tree navigation in inline editor (#52427)
  StyleGuide: Update frontend style guide (#52471)
  Alerting: Fix Slack push notifications (#52391)
  Loki: Fix adding of multiple label filters when parser (#52335)
  Alerting: Preserve new-lines from custom email templates in rendered email (#52253)
  Scene: Fixing state issue with useState when SceneObject instance changes (#52372)
  Scenes: Improve typing of scene state to avoid type guards and casting (#52422)
  ServiceAccounts: Updates the service accounts list page to look good in new top nav design (#52425)
  Dashboard Links: Fix styles for very long dashboard titles (#52443)
  Storage: remove orgId from sql config (#52426)
  Docker: Install `git` in `grafana/grafana-ci-deploy` image (#52466)
  ...
@basvdl
Copy link

basvdl commented Jul 21, 2022

Is my understanding correct that this will be part of the 9.0.4 release?

@MrDibbley
Copy link

MrDibbley commented Jul 25, 2022

Running 9.0.4 (ng_alert_notification.html has above changes) and emails still wrapping without new lines? does something need configuring in the template to enable this ?

image

my testing is only with Outlook for Office 365 which seem to have a poor html engine I believe.

Thanks

@anwu
Copy link

anwu commented Aug 12, 2022

Seconding @MrDibbley here.
I'm on 9.0.5 and still am having issues with the newlines not being preserved.
@alexweav I see in your screenshot that your version is 9.1.0-pre. Is it going to be in 9.1.0 and the notes in 9.0.4 are incorrect?

@alexweav
Copy link
Contributor Author

alexweav commented Aug 16, 2022

@MrDibbley @anwu Sorry to hear there are still issues! Firstly, this change definitely made it into 9.0.4 - although the screenshots above say 9.1.0, we did also backport this to the 9.0 releases.

Our email templating system is fairly confusing right now, we're working on improving the whole template experience as a whole. There are two layers of templating happening and this change only affects the second layer. If no newlines made it to this file in the first place, this PR won't appear to change anything. This is the first thing to check - is your original alert template sending newlines?

FWIW, I installed a 9.0 version (happened to be 9.0.7, but this has not changed since 9.0.4 so it should be the same). I used the following test template:

{{ len .Alerts.Firing }} firing:

{{-  range .Alerts.Firing }}
[{{.Status}}] {{ .Labels.alertname }}:
Labels:
  {{- range .Labels.SortedPairs }}
  - {{ .Name }}: {{ .Value }}
  {{- end }}
{{ end }}

It showed the newlines (and left-align) as expected:
2022-08-16-113707_420x270_scrot

The other thing to check, is that this ng_alert_notification file is only used by Unified Alerting. If you are using Legacy alerting, it uses yet another different templating system and thus you might not see this change take effect. The solution for legacy might not be the same as this.

Hopefully this helps you both! If you are still having problems, please feel free to open an issue and we can take a deeper look.

@jamunam
Copy link

jamunam commented Nov 10, 2022

I have installed version 9.1.7, but still facing the issue of newlines not appearing in the email alert. Any suggestions? I know newlines are in the template because the MS Teams notification is working as expected.
Also, I'm running this in a AKS cluster, and it is not allowing me to edit ng_alert_notification.html. I mention this because the code seems to be missing closing span tag from the template

{{ if gt (len .Message) 0 }}
  <div style="white-space: pre-line;" align="left">{{ .Message }}
{{ else }}

@iStep2Step
Copy link

iStep2Step commented Nov 16, 2022

Is there a chance that this is going to be fixed for older versions too (8.0.4)?

@yzaltz
Copy link

yzaltz commented Dec 28, 2022

Using latest version 9.3.2 - and the problem exist for me

My template:
{{ with $values }}
{{ range $k, $v := . }}
{{ if $v.Labels.action }}
- Action: {{ $v.Labels.action }}
{{ end }}
{{ if $v.Labels.service }}
- Service: {{ $v.Labels.service }}
{{ end }}
Value: {{ $v }}
{{ end }}
{{ end }}

The same notification is sent to Outlook and Telegram -
The Outlook email output doesn't include the newline while the Telegram does (see below) is this bug really fixed?
Outlook:
outlook
Telegram:
telegram

@gillesdemey
Copy link
Member

Outlook does not support white-space: pre-line introduced in the PR. We did not know that Outlook and some other mail clients do not support this CSS property.

Grafana v9.4 will be released with updated email templates and those will parse newlines in the template renderer instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

New lines are remove from Alerting templates when used with e-mail contact point.