alertmanager icon indicating copy to clipboard operation
alertmanager copied to clipboard

Extend discord messages with content

Open mogoll92 opened this issue 1 year ago • 6 comments

These changes are aimed to support discord content parameter besides with embedded message and allow to overcome limitation when you need to ping someone or group of people. (Ping over embedded messages doesn't trigger discord notifications)

Alertmanager already has struct that allows to send content.

type webhook struct {
	Content string         `json:"content"`
	Embeds  []webhookEmbed `json:"embeds"`
}

These changes make content optional and allow to send any text that doesn't break discord limits with embedded message. To enable this user should apply the following changes:

receivers:
  - name: nothing
  - name: discord-success
    discord_configs:
      - webhook_url: ""
        content: '{{ template "discord.content" . }}'
        title: '{{ template "discord.title" . }}'
        message: '{{ template "discord.message" . }}'

and override text in custom template like the following.

{{ define "discord.content" }}
Hey you! <@6532081833221312329>
{{ end }}

Here is the result. Screenshot 2024-08-30 at 15 34 41

mogoll92 avatar Aug 30 '24 12:08 mogoll92

Hello! 👋 I think this might be a duplicate of https://github.com/prometheus/alertmanager/pull/3821?

grobinson-grafana avatar Aug 30 '24 13:08 grobinson-grafana

Hello! 👋 I think this might be a duplicate of #3821?

Yeah, I saw that PR but looks like author is not interested in pushing it forward and I really don't see value in making that additional changes with avatar and username. The only feature that makes sense there is content that is really needed in cases there you need to ping ppl outside.

mogoll92 avatar Aug 30 '24 13:08 mogoll92

Sounds good to me! Before I review, it looks like you have somehow brought in a number of additional commits that aren't relevant to your PR. For example, commits fbcb49b and c07eec8. Can you fix your PR to remove those additional commits? I think just we need commits ae07f25 and 5875042.

grobinson-grafana avatar Sep 02 '24 12:09 grobinson-grafana

Hi, looks like you added even more commits?

grobinson-grafana avatar Sep 02 '24 12:09 grobinson-grafana

Hi, looks like you added even more commits?

yeah, a bit struggled on replacing with needed set of commits. I will ping you here once I'm done.

mogoll92 avatar Sep 02 '24 13:09 mogoll92

@grobinson-grafana check if nothing needs to add, please.

mogoll92 avatar Sep 02 '24 13:09 mogoll92

Is this still being looked at? I am interested in the avatar/username fields, and it'd be nice for this work to be in place before chipping away at adding other fields.

featheredtoast avatar Oct 21 '24 14:10 featheredtoast

I've tested it and looks good to me! I've left a comment of something small that's missing. Please also add some documentation for this in docs/configuration.md.

Something that I'm still not 100% satisfied with is the intuitiveness of title, message and content. It seems like users will be confused when to use title and message and when to use content.

Since the use case here is mentioning users, and nothing else, it might make more sense to do the following instead:

receivers:
  - name: discord
    discord_configs:
      - webhook_url: https://discord.com/api/webhooks/abc
        title: '{{ template "discord.title" . }}'
        message: '{{ template "discord.message" . }}'
        mentions:
          - 6532081833221312329

This will avoid the confusion and be more intuitive to users while still solving the intended use case.

grobinson-grafana avatar Oct 22 '24 19:10 grobinson-grafana

I've tested it and looks good to me! I've left a comment of something small that's missing. Please also add some documentation for this in docs/configuration.md.

Something that I'm still not 100% satisfied with is the intuitiveness of title, message and content. It seems like users will be confused when to use title and message and when to use content.

Since the use case here is mentioning users, and nothing else, it might make more sense to do the following instead:

receivers:
  - name: discord
    discord_configs:
      - webhook_url: https://discord.com/api/webhooks/abc
        title: '{{ template "discord.title" . }}'
        message: '{{ template "discord.message" . }}'
        mentions:
          - 6532081833221312329

This will avoid the confusion and be more intuitive to users while still solving the intended use case.

It will confuse even more, as there is no mentions entity in discord, mentions is some kind of feature that you can use in context, but it isn't the same. Overall in discord the embedded message it's a part of the message and can't exist separately. If you would like to have mirror between alertmanager discord configuration and discord's entities I would recommend the following structure.

receivers:
  - name: discord
    discord_configs:
      - webhook_url: https://discord.com/api/webhooks/abc
        context: '{{ template "discord.content" . }}'
        embedded:
            title: '{{ template "discord.title" . }}'
            message: '{{ template "discord.message" . }}'

In this case it represent exactly what it is. Also this structure could be extended with other fields that discord supports.

mogoll92 avatar Oct 23 '24 08:10 mogoll92

If you would like to have mirror between alertmanager discord configuration and discord's entities I would recommend the following structure.

The problem we have is that renaming/moving/deleting existing fields creates a lot of complex migration issues for other software that builds on top of Alertmanager, like Cortex and Mimir. We can add new fields, but making breaking changes to existing ones is hard.

Could you please update the documentation in docs/configuration.md for this new field? We can then merge it and add it to the 0.28 release.

grobinson-grafana avatar Oct 23 '24 10:10 grobinson-grafana

Hey @mogoll92 I'll give you about ~30 minutes or so to see if you can get it sorted and update the documentation. Otherwise, I'll proceed with #4080 unless you feel oppose to it.

gotjosh avatar Oct 23 '24 15:10 gotjosh

Hey @mogoll92 I'll give you about ~30 minutes or so to see if you can get it sorted and update the documentation. Otherwise, I'll proceed with #4080 unless you feel oppose to it.

if it's okay to you feel free to proceed with it, no objections at all.

mogoll92 avatar Oct 23 '24 16:10 mogoll92

Can you rebase and update the vfs_asserts?

gotjosh avatar Oct 23 '24 17:10 gotjosh

Thank you very much for your contribution!

gotjosh avatar Oct 23 '24 18:10 gotjosh

Thank you very much for your contribution!

you are welcome, first of all I need those changes so I'm interested in it. Thought it never will be happened since so much time passed with no response :)

mogoll92 avatar Oct 23 '24 18:10 mogoll92