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: Enable sending notifications to a specific topic on Telegram #79546

Merged
merged 12 commits into from
Feb 6, 2024
Merged

Alerting: Enable sending notifications to a specific topic on Telegram #79546

merged 12 commits into from
Feb 6, 2024

Conversation

th0th
Copy link
Contributor

@th0th th0th commented Dec 14, 2023

I need help, please check the Special notes for your reviewer section.

What is this feature?

Telegram has a feature called group topics, which is like a channel in Slack or Discord. Current Telegram alerting mechanism doesn't allow sending notifications to a specific topic. This PR aims to add (optionally) setting the topic for Telegram type contact point.

Why do we need this feature?

It is very common for Telegram bot integrations to set a custom topic for the messages sent. For example, a group can have multiple topics like #general #notifications, and the user might want to send alerts to the #notifications topic.

Who is this feature for?

For the folks who use Grafana with Telegram alerts.

Special notes for your reviewer:

I added the required changes for the feature, however, I couldn't get it working, so I need a fresh pair of eyes :) It is like some of the changes I made on my development environment are not taken into consideration. For example, I tried replacing all the api.telegram.orgs in the code with a random, invalid URL, but the "Test" button on the "Contact points" still successfully sent the message to Telegram.

Once we get the thing working, I am planning to update relevant part of the docs as well 👍

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@th0th th0th requested a review from a team as a code owner December 14, 2023 22:07
@CLAassistant
Copy link

CLAassistant commented Dec 14, 2023

CLA assistant check
All committers have signed the CLA.

@grafana-pr-automation grafana-pr-automation bot added area/backend area/frontend pr/external This PR is from external contributor labels Dec 14, 2023
@th0th th0th changed the title Alerting: Enable sending notifications to specific topics on telegram super groups Alerting: Enable sending notifications to a specific topic on Telegram Dec 14, 2023
@yuri-tceretian
Copy link
Contributor

yuri-tceretian commented Dec 15, 2023

Hi @th0th! I appreciate your contribution! However, currently, we do not accept any changes to the integrations that do not exist in the Prometheus Alertmanager (see https://prometheus.io/docs/alerting/latest/configuration/#telegram_config) because we are trying to unify the Grafana and Prometheus Alertmanagers.
I would recommend the following:

  1. Open a PR in https://github.com/prometheus/alertmanager to add this field to TelegramConfig

  2. Open a PR in https://github.com/grafana/alerting/issues to add this field to telegram.Config. This is needed anyway to make this PR working because here it's only the API models.

  3. Ping me about the PRs, and I will make sure they are reviewed promptly.

  4. Once those 2 PRs are merged, we will be able to merge this one.

  5. After all 4 PRs are merged, we will need to update Terraform provider . Open a PR to add this field to the Telegram resource in the Grafana Terraform Provider.

Also, there is another PR that adds a similar field #79534. I left the same message there.

@thoth
Copy link

thoth commented Dec 15, 2023

I think you sent this to the wrong account...

@th0th
Copy link
Contributor Author

th0th commented Dec 15, 2023

Hi @thoth! I appreciate your contribution! However, currently, we do not accept any changes to the integrations that do not exist in the Prometheus Alertmanager (see https://prometheus.io/docs/alerting/latest/configuration/#telegram_config) because we are trying to unify the Grafana and Prometheus Alertmanagers. I would recommend the following:

  1. Open a PR in https://github.com/prometheus/alertmanager to add this field to TelegramConfig
  2. Open a PR in https://github.com/grafana/alerting/issues to add this field to telegram.Config. This is needed anyway to make this PR working because here it's only the API models.
  3. Ping me about the PRs, and I will make sure they are reviewed promptly.
  4. Once those 2 PRs are merged, we will be able to merge this one.
  5. After all 4 PRs are merged, we will need to update Terraform provider . Open a PR to add this field to the Telegram resource in the Grafana Terraform Provider.

Also, there is another PR that adds a similar field #79534. I left the same message there.

Hey @yuri-tceretian, thanks for the explanation.

I created these 3 PRs:

I didn't quite understand how all these are connected to each other, is there is a way I can run this implementation on my local machine, please tell me and I will test :) Also, let me know if there are additional changes needed 💐

@yuri-tceretian
Copy link
Contributor

I think you sent this to the wrong account...

Oh, sorry :)

@yuri-tceretian
Copy link
Contributor

I didn't quite understand how all these are connected to each other, is there is a way I can run this implementation on my local machine, please tell me and I will test :)

To test it locally, you need to update Grafana to use your branch in the alerting repository. Replace the import to the local copy. Add this line to the go.mod of Grafana repo.

replace github.com/grafana/alerting => ../alerting

change path to point to the directory where the alerting repository is cloned.

Make sure you do not commit it.

@th0th
Copy link
Contributor Author

th0th commented Dec 15, 2023

I didn't quite understand how all these are connected to each other, is there is a way I can run this implementation on my local machine, please tell me and I will test :)

To test it locally, you need to update Grafana to use your branch in the alerting repository. Replace the import to the local copy. Add this line to the go.mod of Grafana repo.

replace github.com/grafana/alerting => ../alerting

change path to point to the directory where the alerting repository is cloned.

Make sure you do not commit it.

Thank you, do I need to do the same thing about alertmanager, too? There is already a replace line for it: https://github.com/grafana/grafana/blob/main/go.mod#L498

@yuri-tceretian
Copy link
Contributor

you can replace that "replace" with your version.

@th0th
Copy link
Contributor Author

th0th commented Dec 15, 2023

you can replace that "replace" with your version.

I replaced the alertmanager with my local clone, but it didn't work. Looks like there is a difference between grafana's fork and the upstream one. Should we implement this Telegram change thing on https://github.com/grafana/prometheus-alertmanager as well?

go build -ldflags -w -X main.version=10.3.0-pre -X main.commit=88d47e3257 -X main.buildstamp=1702592018 -X main.buildBranch=telegram-message-thread-id -o ./bin/grafana ./pkg/cmd/grafana
# github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions
pkg/services/ngalert/api/tooling/definitions/alertmanager.go:480:25: undefined: amv2.Integration
# github.com/grafana/alerting/notify
../alerting/notify/grafana_alertmanager.go:48:10: undefined: silence.ValidateMatcher
../alerting/notify/grafana_alertmanager.go:108:28: undefined: notify.Receiver
../alerting/notify/grafana_alertmanager.go:149:30: undefined: notify.Receiver
../alerting/notify/grafana_alertmanager.go:412:26: undefined: notify.Receiver
../alerting/notify/grafana_alertmanager.go:419:40: undefined: notify.NewReceiver
../alerting/notify/grafana_alertmanager.go:445:71: undefined: notify.Receiver
../alerting/notify/grafana_alertmanager.go:621:38: cannot use integrations[i] (variable of type *"github.com/prometheus/alertmanager/notify".Integration) as "github.com/prometheus/alertmanager/notify".Integration value in argument to notify.NewRetryStage
../alerting/notify/factory.go:59:40: cannot use i (variable of type "github.com/prometheus/alertmanager/notify".Integration) as *"github.com/prometheus/alertmanager/notify".Integration value in argument to append
../alerting/notify/grafana_alertmanager_metrics.go:25:44: too many arguments in call to metrics.NewAlerts
	have (string, prometheus.Registerer)
	want (prometheus.Registerer)
exit status 1
exit status 1
make[1]: *** [build-go] Error 1

@th0th
Copy link
Contributor Author

th0th commented Feb 1, 2024

@yuri-tceretian I fixed stuff you mentioned. Can you please have another look?

Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

I merged PR in alerting repository. So, now you can execute

go get github.com/grafana/alerting@ec2c02267fa591e7252ddb44143555bc941ce6fa

and commit those changes into your PR. Unfortunately, your PR seems to be blocked by PR #81512 because alerting module contains some changes that have not updated in Grafana. Let's wait for that PR merged first.

@@ -200,7 +200,7 @@ func (tn *TelegramNotifier) generateTelegramCmd(message string, messageField str
return nil, err
}

tn.log.Info("Sending telegram notification", "chat_id", tn.ChatID, "bot_token", tn.BotToken, "apiAction", apiAction)
tn.log.Info("Sending telegram notification", "chat_id", tn.ChatID, "bot_token", tn.BotToken, "message_thread_id", tn.MessageThreadId, "apiAction", apiAction)
Copy link
Contributor

Choose a reason for hiding this comment

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

this one can be removed too.

@yuri-tceretian yuri-tceretian dismissed their stale review February 2, 2024 18:53

changes addressed

@th0th th0th requested a review from a team as a code owner February 6, 2024 14:46
@th0th th0th requested review from zserge, diegommm and mildwonkey and removed request for VikaCep and a team February 6, 2024 14:46
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM

@yuri-tceretian yuri-tceretian enabled auto-merge (squash) February 6, 2024 15:05
@yuri-tceretian yuri-tceretian merged commit cf601fa into grafana:main Feb 6, 2024
10 checks passed
@th0th
Copy link
Contributor Author

th0th commented Feb 6, 2024

Thank you @yuri-tceretian 💐

Ukochka pushed a commit that referenced this pull request Feb 14, 2024
#79546)

Co-authored-by: Yuri Tseretyan <yuriy.tseretyan@grafana.com>
@aangelisc aangelisc modified the milestones: 10.3.x, 10.3.4, 10.4.0 Mar 6, 2024
@th0th th0th deleted the telegram-message-thread-id branch April 11, 2024 01:45
julienduchesne added a commit to th0th/terraform-provider-grafana that referenced this pull request Jun 13, 2024
The attribute is `message_thread_id`, according to grafana/grafana#79546
julienduchesne added a commit to grafana/terraform-provider-grafana that referenced this pull request Jun 13, 2024
* Add message_thread_id for Telegram contact points

* Update resource_alerting_contact_point_notifiers.go

* Fix casing and tests
The attribute is `message_thread_id`, according to grafana/grafana#79546

* Try with an integer

---------

Co-authored-by: Julien Duchesne <julien.duchesne@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend area/frontend no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants