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

Alertmanager: Add default Grafana template string #8465

Merged
merged 18 commits into from
Jun 26, 2024

Conversation

santihernandezc
Copy link
Contributor

@santihernandezc santihernandezc commented Jun 21, 2024

When using Grafana receivers in Mimir with the default settings some fields in notifications end up empty (e.g. title and message). This PR adds DefaultTemplateString to the *template.Template we use in notifications to fix this issue.

Copy link
Contributor

@stevesg stevesg left a comment

Choose a reason for hiding this comment

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

Whilst we not merging configurations, I'm a little worried about introducing these additional templates into users which just have a standard configuration uploaded. I think I'd prefer them only to be loaded if a Grafana configuration is active.

Why not just add the template into the configuration?

@santihernandezc
Copy link
Contributor Author

@stevesg I realized that trying to send notifications with upstream's receivers results in errors with the original approach. There are some differences in the structure of alerts between upstream and Grafana.

I created separate *template.Template structs for both sets of receivers, and in my last commit I'm creating the grafana template struct only if necessary.

Copy link
Contributor

@stevesg stevesg left a comment

Choose a reason for hiding this comment

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

Hm, this isn't as simple a problem as it seemed like.... We'll definitely have to re-think the ExternalURL situation when we come to merging the configurations, but this is good enough for now.

Perhaps when we do merge configurations, we'll have to pass two ExternalURLs down to the Alertmanager, one for the standard receivers and one for Grafana ones.

@santihernandezc santihernandezc merged commit d429788 into main Jun 26, 2024
29 checks passed
@santihernandezc santihernandezc deleted the santihernandezc/include_default_templates branch June 26, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants