alertmanager icon indicating copy to clipboard operation
alertmanager copied to clipboard

Enable setting ThreadId for Telegram notifications

Open th0th opened this issue 1 year ago • 8 comments

Enable setting message_thread_id for Telegram notifications.

Also see https://github.com/grafana/grafana/pull/79546.

th0th avatar Dec 15 '23 11:12 th0th

Please add

Also, please update the description of this PR with more information. Link to another PR is not a really good description. Thanks!

yuri-tceretian avatar Dec 15 '23 14:12 yuri-tceretian

Please add

Also, please update the description of this PR with more information. Link to another PR is not a really good description. Thanks!

Done 👍

The tests looked OK to me, but let me know if you think you need more cases.

th0th avatar Dec 15 '23 15:12 th0th

Any updates?

jsDotx3 avatar Dec 29 '23 15:12 jsDotx3

@gotjosh @simonpasquier can you please review the PR?

yuri-tceretian avatar Jan 02 '24 15:01 yuri-tceretian

@yuri-tceretian This is a duplicate of an existing pull request, https://github.com/prometheus/alertmanager/pull/3560, which has been pending review since October 16, 2023, nearing four months. Several maintainers have been tagged for attention. It is disappointing to see this duplicate receiving more attention.

asonnleitner avatar Jan 31 '24 19:01 asonnleitner

@yuri-tceretian This is a duplicate of an existing pull request, #3638, which has been pending review since October 16, 2023, nearing four months. Several maintainers have been tagged for attention. It is disappointing to see this duplicate receiving more attention.

If you mean https://github.com/prometheus/alertmanager/pull/3560, this is not duplicate, message_thread_id inclusion is different compared to reply_to_message_id, even though the idea is the same (sending message to a certain topic), message_thread is the intended way to sending a message to a certain topic, reply_to_message, as the fields name says, are more targeted to replying to a certain message ID rather than sending messages.

raphielscape avatar Jan 31 '24 20:01 raphielscape

@yuri-tceretian This is a duplicate of an existing pull request, #3638, which has been pending review since October 16, 2023, nearing four months. Several maintainers have been tagged for attention. It is disappointing to see this duplicate receiving more attention.

If you mean #3560, this is not duplicate, message_thread_id inclusion is different compared to reply_to_message_id, even though the idea is the same (sending message to a certain topic), message_thread is the intended way to sending a message to a certain topic, reply_to_message, as the fields name says, are more targeted to replying to a certain message ID rather than sending messages.

Thank you, @raphielscape, for explaining the differences between PR #3560 and the current request. Your insight into how message_thread_id differs from reply_to_message_id is very helpful. Considering both PRs are focused on enhancing Telegram-related functionalities, it might be beneficial if the maintainers could review and possibly merge them both 🚀

asonnleitner avatar Jan 31 '24 20:01 asonnleitner

hi guys, just a quick question. any plan for merging this PR and releasing the new feature?

cahyahikmawan avatar Feb 20 '24 03:02 cahyahikmawan

Please rebase and sign. See https://github.com/prometheus/alertmanager/pull/3638/checks?check_run_id=21162278935

TheMeier avatar Mar 20 '24 05:03 TheMeier

Please rebase and sign. See https://github.com/prometheus/alertmanager/pull/3638/checks?check_run_id=21162278935

I followed the instructions on the link you shared, please let me know if there is anything else I need to do 💐

th0th avatar Mar 23 '24 18:03 th0th

@th0th I don't think you rebased correctly as there are now 73 commits in this PR. Did you use git rebase? You also still need to sign off all of your commits with --signoff.

grobinson-grafana avatar Mar 23 '24 19:03 grobinson-grafana

@th0th I don't think you rebased correctly as there are now 73 commits in this PR. Did you use git rebase? You also still need to sign off all of your commits with --signoff.

I followed the exact instructions on the link, but I think the commit history got messed up since I previously merged the upstream branch to my own branch. And now I don't know how to fix 😅

th0th avatar Mar 23 '24 19:03 th0th

You can squash all the commits into a single commit using git rebase and then sign the commit using git commit --amend --no-edit --signoff.

Another option is to delete the branch, create a new branch with the same name, add the modifications, commit them, and then force push the branch to GitHub and it will replace the old branch.

grobinson-grafana avatar Mar 23 '24 20:03 grobinson-grafana

You can squash all the commits into a single commit using git rebase and then sign the commit using git commit --amend --no-edit --signoff.

Another option is to delete the branch, create a new branch with the same name, add the modifications, commit them, and then force push the branch to GitHub and it will replace the old branch.

I rebased, and it reduced the number of commits but there are still some commits that are shown in this PR, but not mine. No idea why.

The compile fail was due to a typo I guess, I fixed that, too.

th0th avatar Mar 25 '24 17:03 th0th

I rebased, and it reduced the number of commits but there are still some commits that are shown in this PR, but not mine. No idea why.

Can you share the commands you ran to rebase?

grobinson-grafana avatar Mar 25 '24 19:03 grobinson-grafana

Can you also run go mod tidy as otherwise it doesn't seem to compile:

../../../../../../pkg/mod/github.com/go-openapi/[email protected]/header.go:21:2: missing go.sum entry for module providing package github.com/go-openapi/jsonpointer (imported by github.com/go-openapi/analysis); to add:
	go get github.com/go-openapi/[email protected]
../../../../../../pkg/mod/github.com/go-openapi/[email protected]/ref.go:25:2: missing go.sum entry for module providing package github.com/go-openapi/jsonreference (imported by github.com/go-openapi/spec); to add:
	go get github.com/go-openapi/[email protected]

grobinson-grafana avatar Mar 25 '24 19:03 grobinson-grafana

I rebased, and it reduced the number of commits but there are still some commits that are shown in this PR, but not mine. No idea why.

Can you share the commands you ran to rebase?

I did this:

git remote add upstream https://github.com/prometheus/alertmanager.git
git rebase upstream/main

th0th avatar Mar 25 '24 21:03 th0th

I rebased, and it reduced the number of commits but there are still some commits that are shown in this PR, but not mine. No idea why.

Can you share the commands you ran to rebase?

I did this:

git remote add upstream https://github.com/prometheus/alertmanager.git
git rebase upstream/main

Did you merge main before the rebase? If so, that might have caused the issue. I would just squash all 16 commits in this branch to a single commit using git rebase and then we can go from there. There is a nice tutorial on how to do that here.

grobinson-grafana avatar Mar 26 '24 10:03 grobinson-grafana

come on :))) merge this

hatamiarash7 avatar Apr 08 '24 13:04 hatamiarash7

Hi @th0th! 👋 Do you need help with the rebase?

grobinson-grafana avatar Apr 16 '24 14:04 grobinson-grafana

Hi @th0th! 👋 Do you need help with the rebase?

I do indeed 🙃

th0th avatar Apr 16 '24 22:04 th0th

OK. I ran the following:

git fetch
git rebase origin/main // here origin is [email protected]:prometheus/alertmanager.git
git rebase -i HEAD~16

I then did the following:

pick 9f7f92cd Enable setting ThreadId for telegram notifications
s 7dceb6fa Fix the typo
s 0274e497 Update tests
s 5ec55ae7 Update documentation
s 15682c4a Update notifiers_test.go
s a75dd888 Update notifiers_test.go
s b6017dd0 Use int32 instead of int64 for MessageThreadID
s 3cecaae6 Bump github.com/go-openapi/swag from 0.22.4 to 0.22.7 (#3655)
d b29d9af6 Support UTF-8 label matchers: Add metrics to matchers compat package (#3658)
d c8eca8d9 Change compat metrics to counters (#3686)
s 3c958067 Bump github.com/go-openapi/errors from 0.20.4 to 0.21.0
s aeb3cdbd Bump github.com/go-openapi/swag from 0.22.7 to 0.22.9
s 37426b93 Bump eslint from 8.35.0 to 8.56.0 in /ui/react-app (#3692)
d 084e0d0c Do not register compat metrics in amtool (#3713)
d 70540688 Remove metrics from compat package (#3714)
s ef146bce Fix the typo

This removes the unrelated commits and squashes the others into a single commit. I also removed the commit messages of all other commits except the first commit. You can see here that this leaves just one commit ahead of main:

git log master..th0th/main --oneline
fa78ab4b (HEAD -> th0th/main) Enable setting ThreadId for telegram notifications
cb9724db (origin/main, origin/HEAD, main) notify/webhook: Fix crash on errors when url_file is used (#3798) (#3800)

grobinson-grafana avatar Apr 17 '24 08:04 grobinson-grafana

OK. I ran the following:

git fetch
git rebase origin/main // here origin is [email protected]:prometheus/alertmanager.git
git rebase -i HEAD~16

I then did the following:

pick 9f7f92cd Enable setting ThreadId for telegram notifications
s 7dceb6fa Fix the typo
s 0274e497 Update tests
s 5ec55ae7 Update documentation
s 15682c4a Update notifiers_test.go
s a75dd888 Update notifiers_test.go
s b6017dd0 Use int32 instead of int64 for MessageThreadID
s 3cecaae6 Bump github.com/go-openapi/swag from 0.22.4 to 0.22.7 (#3655)
d b29d9af6 Support UTF-8 label matchers: Add metrics to matchers compat package (#3658)
d c8eca8d9 Change compat metrics to counters (#3686)
s 3c958067 Bump github.com/go-openapi/errors from 0.20.4 to 0.21.0
s aeb3cdbd Bump github.com/go-openapi/swag from 0.22.7 to 0.22.9
s 37426b93 Bump eslint from 8.35.0 to 8.56.0 in /ui/react-app (#3692)
d 084e0d0c Do not register compat metrics in amtool (#3713)
d 70540688 Remove metrics from compat package (#3714)
s ef146bce Fix the typo

This removes the unrelated commits and squashes the others into a single commit. I also removed the commit messages of all other commits except the first commit. You can see here that this leaves just one commit ahead of main:

git log master..th0th/main --oneline
fa78ab4b (HEAD -> th0th/main) Enable setting ThreadId for telegram notifications
cb9724db (origin/main, origin/HEAD, main) notify/webhook: Fix crash on errors when url_file is used (#3798) (#3800)

Thanks a lot @grobinson-grafana. But when I run git rebase -i HEAD~16, and put in the input, it says:

➜  alertmanager git:(main) git rebase -i HEAD~16

error: could not parse '9f7f92cd'
error: invalid line 1: pick 9f7f92cd Enable setting ThreadId for telegram notifications
error: could not parse '7dceb6fa'
error: invalid line 2: s 7dceb6fa Fix the typo
error: could not parse '0274e497'
error: invalid line 3: s 0274e497 Update tests
error: could not parse '5ec55ae7'
error: invalid line 4: s 5ec55ae7 Update documentation
error: could not parse '15682c4a'
error: invalid line 5: s 15682c4a Update notifiers_test.go
error: could not parse 'a75dd888'
error: invalid line 6: s a75dd888 Update notifiers_test.go
error: could not parse 'b6017dd0'
error: invalid line 7: s b6017dd0 Use int32 instead of int64 for MessageThreadID
error: could not parse '3cecaae6'
error: invalid line 8: s 3cecaae6 Bump github.com/go-openapi/swag from 0.22.4 to 0.22.7 (#3655)
error: could not parse 'b29d9af6'
error: invalid line 9: d b29d9af6 Support UTF-8 label matchers: Add metrics to matchers compat package (#3658)
error: could not parse 'c8eca8d9'
error: invalid line 10: d c8eca8d9 Change compat metrics to counters (#3686)
error: could not parse '3c958067'
error: invalid line 11: s 3c958067 Bump github.com/go-openapi/errors from 0.20.4 to 0.21.0
error: could not parse 'aeb3cdbd'
error: invalid line 12: s aeb3cdbd Bump github.com/go-openapi/swag from 0.22.7 to 0.22.9
error: could not parse '37426b93'
error: invalid line 13: s 37426b93 Bump eslint from 8.35.0 to 8.56.0 in /ui/react-app (#3692)
error: could not parse '084e0d0c'
error: invalid line 14: d 084e0d0c Do not register compat metrics in amtool (#3713)
error: could not parse '70540688'
error: invalid line 15: d 70540688 Remove metrics from compat package (#3714)
error: could not parse 'ef146bce'
error: invalid line 16: s ef146bce Fix the typo
You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
Or you can abort the rebase with 'git rebase --abort'.

What am I doing wrong? :(

th0th avatar Apr 17 '24 11:04 th0th

What am I doing wrong? :(

You should be able to edit what is there when you run git rebase -i HEAD~16 – you don't need to copy/paste what I wrote. Basically you should change all pick to s (for squash) with the following exceptions:

  • 9f7f92cd should be kept as pick because this is the first commit
  • b29d9af6 should be d (for drop)
  • c8eca8d9 should be d (for drop)
  • 084e0d0c should be d (for drop)
  • 70540688 should be d (for drop)

grobinson-grafana avatar Apr 17 '24 14:04 grobinson-grafana

What am I doing wrong? :(

You should be able to edit what is there when you run git rebase -i HEAD~16 – you don't need to copy/paste what I wrote. Basically you should change all pick to s (for squash) with the following exceptions:

  • 9f7f92cd should be kept as pick because this is the first commit
  • b29d9af6 should be d (for drop)
  • c8eca8d9 should be d (for drop)
  • 084e0d0c should be d (for drop)
  • 70540688 should be d (for drop)

I think I managed to rebase, can you please confirm @grobinson-grafana? 💐

th0th avatar Apr 18 '24 00:04 th0th

I think I managed to rebase, can you please confirm @grobinson-grafana? 💐

Looks good to me! @gotjosh can you help with CircleCI?

grobinson-grafana avatar Apr 23 '24 19:04 grobinson-grafana

Thanks Josh! @th0th there are some failing tests (please see CI / Test https://github.com/prometheus/alertmanager/actions/runs/8892559543/job/24416875284?pr=3638). You can also run make test before committing the fix to make sure there are no other failing tests.

grobinson-grafana avatar Apr 30 '24 09:04 grobinson-grafana

Thank you very much for your contribution!

gotjosh avatar Apr 30 '24 10:04 gotjosh

@th0th Thanks also for the contribution! I would appreciate it if you could test it and make sure it works as I cannot seem to create a Telegram supergroup or topics (perhaps I just don't know how to do it?).

grobinson-grafana avatar Apr 30 '24 11:04 grobinson-grafana

Thanks for the help getting this merged folks :) @grobinson-grafana I would love to test, but I am using grafana right now, and I don't have alertmanager running separately :(

th0th avatar Apr 30 '24 13:04 th0th