Extend Discord Notifier #3761

LucaDev · 2024-03-12T07:36:04Z

This PR extends the Discord notifier to improve the readability of alerts.
It changes 5 key elements:

A configureable icon as bot Logo (I'm currently using the GitHub Avatar. Is that alright? Is there a better way for doing this?)
A configurable name for the bot
Alert-ID in the footer
Start & end times in the footer (Even if the message is sent with a delay, the correct time will be displayed)
It uses Discords fields-feature to display labels
It sends one message per alert

It also fixes UI builds on apple silicon by forcing the linux/amd86 platform for the elm container

Before:

gotjosh · 2024-03-12T10:03:55Z

This looks nice! Thank you very much for your contribution -- can you please show me an example of the after with multiple labels?

LucaDev · 2024-03-12T10:18:07Z

This looks nice! Thank you very much for your contribution -- can you please show me an example of the after with multiple labels?

Sure! Desktop:

I'm also planing to change the title to not include the labels anymore

LucaDev · 2024-03-12T11:01:42Z

Small update:

LucaDev · 2024-03-14T15:31:35Z

I'd love to get some first feedback @gotjosh

LucaDev · 2024-04-26T19:03:01Z

Hey. I‘d love to get some feedback on this.

LucaDev · 2024-04-26T19:06:06Z

I‘ll correct a few minor things this weekend. Afterwards it should be ready to be merged

grobinson-grafana · 2024-05-12T21:53:13Z

The visual changes look nice! You'll need to remove the one message per alert change though as it interferes with alert grouping behavior. For example, if a user wants one message per alert then grouping can be disabled in the route, like so:

route:
    receiver: test
    group_by: ['...']

You can find it documented here.

LucaDev · 2024-05-23T15:57:45Z

Good catch! Thank you!

The visual changes look nice! You'll need to remove the one message per alert change though as it interferes with alert grouping behavior. For example, if a user wants one message per alert then grouping can be disabled in the route, like so:
route:
    receiver: test
    group_by: ['...']
You can find it documented here.

Could you please have a look at the latest changes? Grouping is now allowed by only sending one message per alert - containing as much 'embeds' (those alert boxes you're seeing) as allowed per Discord API limits. Is it okay like that?

I still have to clean the code up and make it a bit more readable before this is ready to be merged tho. (Argh.. and signing off the past commits. I always forget to do it.. I will add a signed-off git message template now 😄)

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

grobinson-grafana · 2024-05-29T16:09:29Z

config/notifiers.go

+	Title                string `yaml:"title,omitempty" json:"title,omitempty"`
+	Message              string `yaml:"message,omitempty" json:"message,omitempty"`
+	TitleURL             string `yaml:"title_url,omitempty" json:"title_url,omitempty"`
+	SkipFields           bool   `yaml:"skip_fields,omitempty" json:"skip_fields,omitempty"`


I think we should remove this as I haven't seen users asking for this in the past. It's much easier to add it later if we need. It's much harder to remove something once it's been added.

I kinda agree. On the other hand... #3821 wants to add them. So apparently users are asking for them. What do you think? I'm okay with both options. I'll let you guys - as the actual code owners - decide what direction to choose here. It would certainly reduce the huge amount of validation / truncation I had to add 😄 On the other hand - customizability is always a good thing I guess. @gotjosh what do you think?

I'd side with George here unless we're explicitly using them I'd be in favour of removing them - I get when you're coming from on the customization approach but we're very short on hands in the Alertmanager, the less code we have to maintain the better.

My approach for this tends to be: Create an issue with the specific functionality you want to introduce, if we get enough support (number of reactions or comments on the issue) for it we can always add it.

grobinson-grafana · 2024-05-29T16:11:08Z

notify/discord/discord.go

+	// 2000 characters per message is the limit (not counting embeds).
+	maxMessageContentLength = 2000
+	// 10 embeds per Message is the maximum.
+	maxEmbedsPerMessage = 10


A concern I have here is that it's not uncommon for users to have large groups (more than 10 alerts). If Discord is limited to 10 embeds then this might make notifications less useful for those users as truncation will become a regular occurrence for them. What do you think @gotjosh?

🤔 good point, I think this is OK for now - I really like the look of this notification as a whole. If users find 10 is too minimal for them to pay attention (in single group) we can always improve it later on.

All I care is that when we have more than 10, we provide a clear message that states we couldn't fit them all.

grobinson-grafana · 2024-05-29T16:15:39Z

notify/discord/discord.go

 	data := notify.GetTemplateData(ctx, n.tmpl, as, n.logger)
 	tmpl := notify.TmplText(n.tmpl, data, &err)
 	if err != nil {
 		return false, err
 	}

-	title, truncated := notify.TruncateInRunes(tmpl(n.conf.Title), maxTitleLenRunes)
+	author, truncated := notify.TruncateInRunes(tmpl(n.conf.BotUsername), maxEmbedAuthorNameLenRunes)


Suggested change

author, truncated := notify.TruncateInRunes(tmpl(n.conf.BotUsername), maxEmbedAuthorNameLenRunes)

username, truncated := notify.TruncateInRunes(tmpl(n.conf.BotUsername), maxEmbedAuthorNameLenRunes)

grobinson-grafana · 2024-05-29T16:15:48Z

notify/discord/discord.go

-	if alerts.Status() == model.AlertFiring {
-		color = colorRed
+	w := webhook{
+		Username:  author,


Suggested change

Username: author,

Username: username,

grobinson-grafana · 2024-05-29T16:17:28Z

notify/discord/discord.go

+
+	alerts := types.Alerts(as...)
+
+	for alertIndex, alert := range alerts {


Can use idiomatic variable name i here:

Suggested change

for alertIndex, alert := range alerts {

for i, alert := range alerts {

grobinson-grafana · 2024-05-29T16:18:18Z

notify/discord/discord.go

+			break
+		}
+
+		title, truncated := notify.TruncateInRunes(tmpl(n.conf.Title), maxTitleLenRunes)


The title gets repeated for each embed which is kind of weird. For example: [FIRING:2] but each embed contains just one alert.

You're right. I'm gonna refactor this - using the embed title for displaying the alert summary (CommonAnnotations.summary) and adding the total alert count to the message above the embeds.

grobinson-grafana · 2024-05-29T16:23:08Z

I noticed that when the maximum number of embeds is exceeded, just the first embed is sent? Is that expected? Looking at the code I would expect 10 embeds to be sent with the truncation message rather than just 1 embed?

LucaDev · 2024-06-10T08:52:27Z

I'd love to finalize this and get this into 0.27.1. @gotjosh could you please have a look at the discussions above?

LucaDev · 2024-07-04T07:26:56Z

I've been waiting for over two months now, and my PR still hasn't been fully reviewed, nor have my questions been answered. I know everyone is busy, but it's getting a bit frustrating. I'd love to help improve the software we all use day to day, but I'm not sure if I can keep contributing if things don't change.

For those who might not know what I'm talking about, here is my current focus: Improving Discord Integration in Alertmanager.

Implemented features aren't tested at all (see: feat: implement webhook_url_file for discord and msteams #3555)
No problem. I'll write a fix for that. Only That the fixes aren't released (see: fix: discord & msteams webhook_url_file configuration #3728 (comment) "I'm going to release v0.27.1 relatively soon [...]")
PRs aren't getting reviewed in time. (see: this one)

Please don't get me wrong; I love your work and want to keep contributing. I do honor your tight schedules. It's just getting a little... frustrating for me and, looking at the comments on some of the PRs, for many others as well.

grobinson-grafana · 2024-07-04T07:59:41Z

Hi Luca! 👋

I think we are still waiting on some changes since the last time, no?

You're right. I'm gonna refactor this - using the embed title for displaying the alert summary (CommonAnnotations.summary) and adding the total alert count to the message above the embeds.

I don't see any commits since this comment, is this still something you want to change?

I noticed that when the maximum number of embeds is exceeded, just the first embed is sent? Is that expected? Looking at the code I would expect 10 embeds to be sent with the truncation message rather than just 1 embed?

I believe this isn't working as it should, did you get time to take a look?

LucaDev · 2024-07-04T08:11:03Z

I solved some locally without pushing because I'm waiting for the discussions above to resolve. As long as I don't know what way you want me to implement things, I don't know how to continue working on this.

See:
My last comment:

I'd love to finalize this and get this into 0.27.1. @gotjosh could you please have a look at the discussions above?

Waiting for someone else to comment on it:

Waiting on anyone to comment on it:

grobinson-grafana · 2024-07-04T08:30:24Z

I solved some locally without pushing because I'm waiting for the discussions above to resolve. As long as I don't know what way you want me to implement things, I don't know how to continue working on this.

Oh! Please push those changes so we can take a look! I think those two (repeated title and missing embeds) are unrelated to the other discussions so 100% recommend we push those fixes to be reviewed 👍

Waiting for someone else to comment on it:

I'll check in with @gotjosh, let's keep making progress on the remaining issues and we can come back to this! 👍

Waiting on anyone to comment on it:

I'm not sure I see where #3821 is adding a skip_fields field? I think my opinion is unchanged on this topic – let's remove it and add it back if it turns out users need it.

grobinson-grafana · 2024-07-08T07:25:54Z

Hi @LucaDev! Do you want to push the changes you made so we can take a look?

grobinson-grafana · 2024-10-23T18:58:22Z

Hi @LucaDev! We are preparing an Alertmanager 0.28 release, are you still interested in this?

gotjosh · 2024-10-24T20:20:29Z

I apologise @LucaDev - as you can appreciate, my GitHub notifications are just unmanageable and other priorities have kept me away from Alertmanager duties.

I don't think I'll have the time herd this PR into the cut for 0.28 which is sad - but can we try to attempt for the next release?

LucaDev · 2024-11-03T11:02:33Z

Hey all. Apologies for letting you wait for so long. I had quite a few busy months. I’ll rebase the code and push the latest changes soon. Would love to see it in 0.28 (especially due to the long release cycles of alertmanager) but 0.29 would also be fine I guess. Better late then never after all 😄

LucaDev force-pushed the main branch from 227592e to 064d99d Compare March 12, 2024 09:04

LucaDev marked this pull request as ready for review April 26, 2024 19:03

LucaDev force-pushed the main branch from a82bcd0 to 6277afd Compare May 23, 2024 15:38

LucaDev added 10 commits May 23, 2024 18:05

enhance discord integration

096ea99

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

fix UI build on apple silicon

e6cfc4f

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

extend discord notifier config options

00cc5d3

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

add new discord docs, fix skip_fields

29d6e56

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

add URL & templating support, improve validation

b16aa08

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

fix discord webhook sending

a41c6d6

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

allow alert grouping

4c72f1e

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

Enforce the limit of 10 embeds per message

c5f7187

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

minor code cleanup

f2bd09e

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

Fixed embed limit enforcement

20c9aea

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

LucaDev force-pushed the main branch from 656d2cf to 20c9aea Compare May 23, 2024 16:15

LucaDev added 3 commits May 23, 2024 18:23

fix formatting

48ba07a

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

Make sure all templating errors are handled correctly

5eab8d9

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

fixed some more formatting errors

4a6a4be

Signed-off-by: Luca Kröger <l.kroeger01@gmail.com>

grobinson-grafana reviewed May 29, 2024

View reviewed changes

grobinson-grafana mentioned this pull request May 30, 2024

Add discord message fields #3821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Discord Notifier #3761

Extend Discord Notifier #3761

LucaDev commented Mar 12, 2024 •

edited

Loading

gotjosh commented Mar 12, 2024

LucaDev commented Mar 12, 2024

LucaDev commented Mar 12, 2024

LucaDev commented Mar 14, 2024

LucaDev commented Apr 26, 2024

LucaDev commented Apr 26, 2024

grobinson-grafana commented May 12, 2024

LucaDev commented May 23, 2024

grobinson-grafana May 29, 2024

LucaDev May 30, 2024 •

edited

Loading

gotjosh Oct 24, 2024

grobinson-grafana May 29, 2024

gotjosh Oct 24, 2024

grobinson-grafana May 29, 2024

grobinson-grafana May 29, 2024

grobinson-grafana May 29, 2024

grobinson-grafana May 29, 2024

LucaDev May 30, 2024

grobinson-grafana commented May 29, 2024

LucaDev commented Jun 10, 2024

LucaDev commented Jul 4, 2024 •

edited

Loading

grobinson-grafana commented Jul 4, 2024

LucaDev commented Jul 4, 2024

grobinson-grafana commented Jul 4, 2024

grobinson-grafana commented Jul 8, 2024

grobinson-grafana commented Oct 23, 2024 •

edited

Loading

gotjosh commented Oct 24, 2024

LucaDev commented Nov 3, 2024

	author, truncated := notify.TruncateInRunes(tmpl(n.conf.BotUsername), maxEmbedAuthorNameLenRunes)
	username, truncated := notify.TruncateInRunes(tmpl(n.conf.BotUsername), maxEmbedAuthorNameLenRunes)


		alerts := types.Alerts(as...)

		for alertIndex, alert := range alerts {

	for alertIndex, alert := range alerts {
	for i, alert := range alerts {

Extend Discord Notifier #3761

Are you sure you want to change the base?

Extend Discord Notifier #3761

Conversation

LucaDev commented Mar 12, 2024 • edited Loading

gotjosh commented Mar 12, 2024

LucaDev commented Mar 12, 2024

LucaDev commented Mar 12, 2024

LucaDev commented Mar 14, 2024

LucaDev commented Apr 26, 2024

LucaDev commented Apr 26, 2024

grobinson-grafana commented May 12, 2024

LucaDev commented May 23, 2024

Choose a reason for hiding this comment

LucaDev May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobinson-grafana commented May 29, 2024

LucaDev commented Jun 10, 2024

LucaDev commented Jul 4, 2024 • edited Loading

grobinson-grafana commented Jul 4, 2024

LucaDev commented Jul 4, 2024

grobinson-grafana commented Jul 4, 2024

grobinson-grafana commented Jul 8, 2024

grobinson-grafana commented Oct 23, 2024 • edited Loading

gotjosh commented Oct 24, 2024

LucaDev commented Nov 3, 2024

LucaDev commented Mar 12, 2024 •

edited

Loading

LucaDev May 30, 2024 •

edited

Loading

LucaDev commented Jul 4, 2024 •

edited

Loading

grobinson-grafana commented Oct 23, 2024 •

edited

Loading