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

Add card feature to msteamsv2_config #4243

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jverger
Copy link

@jverger jverger commented Feb 3, 2025

The goal of this PR is to add an option card to the msteamsv2_config configuration.

If this card option is not used, the default behaviour is keept using title and text values.

Here is an example screenshot of the default result in msteams channel:
image

When using card option, the user is able to customize the msteams card rendering output by using a custom template file and configuring card option to use this template.

Here is an example screenshot of the output result in msteams channel when using card option:
image

To be honest, it's the first time i'm developing in go and create a PR.
Hope it will be not so bad :)

@jverger
Copy link
Author

jverger commented Feb 3, 2025

All checks have passed 🎉 , waiting for someone to merge this PR ⌚

@grobinson-grafana
Copy link
Collaborator

I would really like to avoid pushing users towards templating JSON blobs. It has a lot of problems that I've explained in a couple other PRs such as #3799 (comment).

I think supporting cards would be excellent, because visually its a massive improvement, but just not with templated JSON.

@jverger
Copy link
Author

jverger commented Feb 3, 2025

Nevermind, I would have try.
Thank you for your answer.

@grobinson-grafana
Copy link
Collaborator

What if you made cards the default instead of a separate field, using the template you have already written? Visually it looks so much better in my opinion!

@jverger
Copy link
Author

jverger commented Feb 7, 2025

Hello @grobinson-grafana !

I'm trying to add a card template in the default.tmpl file but it does not work (and i don't know why 😢 )
The error message looks like this:
level=ERROR source=dispatch.go:360 msg="Notify for alerts failed" component=dispatcher num_alerts=1 err="teams/msteamsv2[0]: notify retry canceled due to unrecoverable error after 1 attempts: template: :1:12: executing \"\" at <{{template \"msteamsv2.default.card\" .}}>: template \"msteamsv2.default.card\" not defined"

I think that i've correctly defined the card template but it does not seem to detect it...

Some help is needed

@ewencodes
Copy link

ewencodes commented Feb 24, 2025

Hi @jverger, did you rename your template function to msteamsv2.default.card in default.tmpl ?

Your card is more readable then the existing one, we need your template to be used as default one :)

@deejiw deejiw force-pushed the feature/msteams_card branch 2 times, most recently from 2978d90 to df4fd79 Compare February 26, 2025 16:03
jverger and others added 9 commits February 26, 2025 23:06
Signed-off-by: jverger <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
Signed-off-by: jverger <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
Signed-off-by: Anastasios_Dados <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
…on (prometheus#4258)

The update addresses failing GitHub Actions caused by the deprecation of
v3 actions/upload-artifact and actions/download-artifact APIs.

This change:
- Updates promci from previous version to v0.4.6
- I hope resolves CI failures in artifact upload/download steps

Signed-off-by: Raúl Naveiras <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
…on (prometheus#4259)

Same as prometheus#4258

The update addresses failing GitHub Actions caused by the deprecation of
v3 actions/upload-artifact and actions/download-artifact APIs.

This change:
- Updates promci from previous version to v0.4.6
- I hope resolves CI failures in artifact upload/download steps

https://github.com/prometheus/promci/releases/tag/v0.4.6
https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

Signed-off-by: Raúl Naveiras <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
Signed-off-by: Tossaporn Jiw <[email protected]>
@deejiw deejiw force-pushed the feature/msteams_card branch from 98cbaa5 to f69c86a Compare February 26, 2025 16:06
@deejiw
Copy link

deejiw commented Feb 26, 2025

@ewencodes @grobinson-grafana I have updated the default card template as per request. Is it what you are expected?

@TheMeier
Copy link
Contributor

Can we expect any progress here?

@grobinson-grafana
Copy link
Collaborator

Hi! 👋 I think there is some misunderstanding here, let's see if we can get back on the same page here.

I'm not looking for a card field, and I'm not looking for a Go template that outputs JSON, whether user customizable or in the default template.

The card.tmpl template you have written sums up the problem quite well. In my opinion, it's basically impossible to read this template and know that it produces valid JSON for every possible combination of inputs. You seem to have figured out the trick that you need to put the , at the start of the block rather than at the end:

"facts": [
  { "title": "Status", "value": "{{ .Status }} {{ if eq .Status "firing" }}🔥{{ else if eq .Status "resolved" }}✅{{ else }}⚠️{{ end }}" }
  {{ if .CommonLabels.alertname }}, { "title": "Alert", "value": "{{ .CommonLabels.alertname }}" }{{ end }}
  {{ if .CommonLabels.instance }}, { "title": "In host", "value": "{{ .CommonLabels.instance }}" }{{ end }}
  {{ if .CommonLabels.severity }}, { "title": "Severity", "value": "{{ .CommonLabels.severity }} {{ if eq .CommonLabels.severity "critical" }}❌{{ else if eq .CommonLabels.severity "error" }}❗️{{ else if eq .CommonLabels.severity "warning" }}⚠️{{ else if eq .CommonLabels.severity "info" }}ℹ️{{ else }}❓{{ end }}" }{{ end }}
  {{ if .CommonAnnotations.description }}, { "title": "Description", "value": "{{ .CommonAnnotations.description }}" }{{ end }}
  {{- range $key, $value := .CommonLabels }}
    {{- if and (ne $key "alertname") (ne $key "instance") (ne $key "severity") }}
    , { "title": "{{ $key }}", "value": "{{ $value }}" }
    {{- end }}
  {{- end }}
  {{- range $key, $value := .CommonAnnotations }}
    {{- if and (ne $key "summary") (ne $key "description") }}
    , { "title": "{{ $key }}", "value": "{{ $value }}" }
    {{- end }}
  {{- end }}
]

but as soon as someone wants to do this, it no longer works:

"facts": [
  {{ if .CommonLabels.alertname }}, { "title": "Alert", "value": "{{ .CommonLabels.alertname }}" }{{ end }}
  {{ if .CommonLabels.instance }}, { "title": "In host", "value": "{{ .CommonLabels.instance }}" }{{ end }}
]

This will output invalid JSON whenever {{ if .CommonLabels.alertname }} is true.

What I would like to see is what you have done in msteamsv2.go where you are building the AdaptiveCards as an intermediate data structure and then calling json.Marshal on it.

R.E user customization of adaptive cards, we have the exact same problem to solve in Slack with BlockKit. I'm not sure how best to solve it right now, but I'm 99% sure templating JSON in Go templates won't be the right solution and will create us more problems then it solves in the long term.

@TheMeier
Copy link
Contributor

I would really love to see a possibility to provide a go template which would be used to generate the complete card JSON. What problems do you see apart from user errors leading to support request? These could be mitigated with some stern words in the documentation telling the user that this feature does not have support for the user generated content and one should be very careful with it.

@grobinson-grafana
Copy link
Collaborator

grobinson-grafana commented Mar 12, 2025

I would really love to see a possibility to provide a go template which would be used to generate the complete card JSON. What problems do you see apart from user errors leading to support request? These could be mitigated with some stern words in the documentation telling the user that this feature does not have support for the user generated content and one should be very careful with it.

The main issue for me is that writing templates that output valid JSON is difficult if not impossible for real-world templates, and becomes even harder when using if statements and for loops. For example, this template outputs invalid JSON because it has a trailing comma:

[
  {{ range .Alerts.Firing }}
  {
      "alertname": "{{ index .Labels "alertname" }}"
  },
  {{ end }}
]
[
  {
    "name": "foo"
  },
]

Instead, the template must be written like this:

[
  {{ range $i, $v := .Alerts.Firing }}
  {{ if ne $i 0 }},{{end }}{
    "alertname": "{{ index .Labels "alertname" }}"
  }
  {{ end }}
]

I suppose simple enough to fix? But now consider doing this over a much larger template that is hundreds of lines of code, with nested if statements, inner loops, etc.

Just one missing , will break the entire notification, which is far from ideal for what might be mission critical alerts.

I do appreciate that we could tell users "you are responsible for making sure your template is correct". But I feel the tools we would be giving them are so difficult to use correctly, it's neither helpful nor useful advice. I also fear it will give the project a bad reputation of being unreliable when people do complain that Alertmanager didn't send a notification for one of their alert because of a bad template.

@TheMeier
Copy link
Contributor

TheMeier commented Mar 12, 2025

I think you underestimate the customer here. I for example have templates with many non-printing define sections which in the end will be included in the json printing part of the template. This makes it quite manageable and maintainable. The defines can even reside in separate template files.
Also the default would still be the same as it is now, after all this would only add an option. So unless a user want's to go down that path he wouldn't even know.
The alternative is to send the payload to a different service which then formats it for you and sends it on to the final recipient, which makes everything much more complex (and potentially less reliable).

@grobinson-grafana
Copy link
Collaborator

grobinson-grafana commented Mar 12, 2025

I do appreciate that perspective. Just for completeness, something like this is kind of what I had in mind for both Slack and Microsoft Teams, although I haven't had time to iterate on it and build a PoC.

Basically the schema would be defined in YAML:

elements:
  - TextBlock:
    text: {{ len .Alerts.Firing }} :fire: alerts, {{ len .Alerts.Resolved }} :white_check_mark: alerts
    wrap: true
  - Range:
    expr: {{ range .Alerts.Firing }}
    elements:
      - ActionSet:
        actions:
          - Action.OpenUrl
              title: Silence this alert
              url: {{ .SilenceURL }}

This would mean we can do enough static checks to catch most errors. Behind the scenes, we could generate a Go template that produces JSON, but it wouldn't require the user to deal with all the challenges of writing it themselves:

[
  {
    "type": "TextBlock",
    "text": "{{ len .Alerts.Firing }} :fire: alerts, {{ len .Alerts.Resolved }} :white_check_mark: alerts",
    "wrap": true
  },
  {{ range $i, $_ := .Alerts }}
  {{ if ne $i 0 }},{{ end }}
  {
    "type": "ActionSet",
    "actions": [
      {
       "type": "Action.OpenUrl",
       "title": "Silence",
        "url": "{{ .SilenceURL }}"
      }
    ]
  }
  {{ end }}
]

We would have something similar for other control blocks like if conditions.

What are you thoughts on this?

@TheMeier
Copy link
Contributor

@grobinson-grafana I am not a big fan of that. This would increase the complexity of the notifiers but still not unlock the full potential of cards. The AdaptiveCard has many more features then TextBlock and URLs, same goes for slack or rocketchat or telegram..

I had a different thought this morning though which goes in a completely different direction. First I want to re-iterate what I would like to achieve:

I want users to have the option to completely define and shape the body content of notifications send via POST to a http(s) endpoint. I would also like to have that option for slack, telegram, teams or whatever notifer.

Well it turns out at that point all you need is a generic web-hook notifier with templating. And... we already have an existing web-hook notifier but... it does not support templating.

So I would like to re-open the templating for the web-hook notifiers discussion once again ;)

As I said, such a feature could be hidden behind large warning texts. Also to make it more approachable a repository (maybe in https://github.com/prometheus-community) could be provided with templates and template includes (think
go template defines) , not only for this but also to gather awesome templates for all notfiiers. Much like https://github.com/samber/awesome-prometheus-alerts provides some inspiration for alert-rules.

@grobinson-grafana
Copy link
Collaborator

@grobinson-grafana I am not a big fan of that. This would increase the complexity of the notifiers but still not unlock the full potential of cards. The AdaptiveCard has many more features then TextBlock and URLs, same goes for slack or rocketchat or telegram..

We can support all the elements we want, this was just an example 😄

I had a different thought this morning though which goes in a completely different direction. First I want to re-iterate what I would like to achieve:

I want users to have the option to completely define and shape the body content of notifications send via POST to a http(s) endpoint. I would also like to have that option for slack, telegram, teams or whatever notifer.

Well it turns out at that point all you need is a generic web-hook notifier with templating. And... we already have an existing web-hook notifier but... it does not support templating.

So I would like to re-open the templating for the web-hook notifiers discussion once again ;)

I know this is feature is top of a lot of people's wish list. I'm still not convinced it's the right move. If we add support for templating JSON in Go templates, I'm certain people will also want to be able to do other things like template HTTP headers, implement multiple HTTP request/responses, use their own authentication flows, etc.

My opinion on this would be to allow people to attach sidecars (plugins) where they can use their own integrations without having to upstream it.

As I said, such a feature could be hidden behind large warning texts. Also to make it more approachable a repository (maybe in https://github.com/prometheus-community) could be provided with templates and template includes (think go template defines) , not only for this but also to gather awesome templates for all notfiiers. Much like https://github.com/samber/awesome-prometheus-alerts provides some inspiration for alert-rules.

@TheMeier
Copy link
Contributor

TheMeier commented Mar 13, 2025

We can support all the elements we want, this was just an example 😄

The thing is, the AdaptiveCard has a versioned schema, so any feature added at a later point might trigger a feature request by someone in the future.

I know this is feature is top of a lot of people's wish list. I'm still not convinced it's the right move. If we add support for templating JSON in Go templates, I'm certain people will also want to be able to do other things like template HTTP headers, implement multiple HTTP request/responses, use their own authentication flows, etc.

My opinion on this would be to allow people to attach sidecars (plugins) where they can use their own integrations without having to upstream it.

Yes the sidecar thingy been there before. But for a very large part of the use-cases this simply would become unnecessary if the web-hook notifier would allow optional templating. This could improve usability and reduce complexity for many users.

There is probably a reason it is: top of a lot of people's wish list ;)

@grobinson-grafana
Copy link
Collaborator

We can support all the elements we want, this was just an example 😄

The thing is, the AdaptiveCard has a versioned schema, so any feature added at a later point might trigger a feature request by someone in the future.

I know this is feature is top of a lot of people's wish list. I'm still not convinced it's the right move. If we add support for templating JSON in Go templates, I'm certain people will also want to be able to do other things like template HTTP headers, implement multiple HTTP request/responses, use their own authentication flows, etc.
My opinion on this would be to allow people to attach sidecars (plugins) where they can use their own integrations without having to upstream it.

Yes the sidecar thingy been there before. But for a very large part of the use-cases this simply would become unnecessary if the web-hook notifier would allow optional templating. This could improve usability and reduce complexity for many users.

There is probably a reason it is: top of a lot of people's wish list ;)

Btw, do you know of any examples of other projects that support templating JSON? It would be good to evaluate those to see what worked well and what didn't so we don't make the same mistakes here.

@TheMeier
Copy link
Contributor

Btw, do you know of any examples of other projects that support templating JSON? It would be good to evaluate those to see what worked well and what didn't so we don't make the same mistakes here.

I can't think of one now. But there is quite the precedent for using go templates for yaml, a small project called helm

@grobinson-grafana
Copy link
Collaborator

Btw, do you know of any examples of other projects that support templating JSON? It would be good to evaluate those to see what worked well and what didn't so we don't make the same mistakes here.

I can't think of one now. But there is quite the precedent for using go templates for yaml, a small project called helm

I understand that templating Helm charts is quite a large pain point for lots of Helm users as whitespace preservation is essential when templating YAML and Go templates make this hard. I think the challenges of templating configuration files is one of the reasons for jsonnet.

@TheMeier
Copy link
Contributor

TheMeier commented Mar 24, 2025

@grobinson-grafana I see your point, but I do disagree. There are lots of people who are quite well versed in working with go-templating nowadays. Also tooling for go-templates is much better then it was a couple of years ago (online-playground, ide-integrations etc)

Wondering if anyone else has an opinion here @simonpasquier @gotjosh @w0rm.

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

Successfully merging this pull request may close these issues.

7 participants