notify/telegram: Set API URL and ParseMode defaults #2879

metalmatze · 2022-04-05T10:46:42Z

For Telegram, there is really just one API URL, and other than for testing I never saw anything changing it. Therefore, as asked in #2876 we can set the default.
The default Telegram template that's shipped with Alertmanager uses HTML and if send to Telegram without the parseMode set to HTML Telegram's API will return an error:

Bad Request: can't parse entities: Character '-' is reserved and must be escaped with the preceding '\\' (400)"

Closes #2866
Closes #2876

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

gotjosh

LGTM

Can you please fix the title of the PR so that it specifies that what we're fixing is the URL?

From notify/telegram: Set API and ParseMode defaults to Telegram: Set API URL and ParseMode defaults

gotjosh · 2022-04-08T15:26:54Z

notify/telegram/telegram.go

@@ -44,6 +45,19 @@ func New(conf *config.TelegramConfig, t *template.Template, l log.Logger, httpOp
 		return nil, err
 	}

+	if conf.APIUrl == nil {
+		apiURL, err := url.Parse("https://api.telegram.org")


nit: Can this be a const?

This should be done during unmarhalling of the configuration, not when calling New.

Good shout Julien.

On a second look at

alertmanager/config/notifiers.go

Lines 140 to 147 in 71d61c9

DefaultTelegramConfig = TelegramConfig{

NotifierConfig: NotifierConfig{

VSendResolved: true,

},

DisableNotifications: false,

Message: `{{ template "telegram.default.message" . }}`,

ParseMode: "MarkdownV2",

}

It seems like both of these belong within DefaultTelegramConfig

nit really because the API should come from DefaultGlobalConfig

gotjosh · 2022-04-08T15:31:10Z

notify/telegram/telegram.go

+	if conf.ParseMode == "" {
+		// The default Telegram template is in HTML,
+		// so we need to set the parse mode to HTML for Telegram not to return a parse error.
+		conf.ParseMode = "HTML"


nit: Can this also be a const? Perhaps a suggestion is defaultParseMode = "HTML"

This should be done during unmarhalling of the configuration, not when calling New.

Our current default is actually MarkdownV2 this would effectively be a breaking change and I don't think it was the intent of #2827.

Looking at the current template - I don't see how it is HTML, am I missing something? https://github.com/prometheus/alertmanager/blob/main/template/default.tmpl#L105-L114

gotjosh

On a second look, it seems like some parts of these are not correct.

I suggest we split this into two PRs.

The default URL is correct and seems straightforward - we need to add it to the docs though.
The default parse mode is currently Markdown, and setting the default to HTML would be a breaking change based on what's currently here.

gotjosh · 2022-04-11T12:47:08Z

notify/telegram/telegram.go

@@ -44,6 +45,19 @@ func New(conf *config.TelegramConfig, t *template.Template, l log.Logger, httpOp
 		return nil, err
 	}

+	if conf.APIUrl == nil {
+		apiURL, err := url.Parse("https://api.telegram.org")


Good shout Julien.

On a second look at

alertmanager/config/notifiers.go

Lines 140 to 147 in 71d61c9

DefaultTelegramConfig = TelegramConfig{

NotifierConfig: NotifierConfig{

VSendResolved: true,

},

DisableNotifications: false,

Message: `{{ template "telegram.default.message" . }}`,

ParseMode: "MarkdownV2",

}

It seems like both of these belong within DefaultTelegramConfig

gotjosh · 2022-04-11T12:50:54Z

notify/telegram/telegram.go

+	if conf.ParseMode == "" {
+		// The default Telegram template is in HTML,
+		// so we need to set the parse mode to HTML for Telegram not to return a parse error.
+		conf.ParseMode = "HTML"


Our current default is actually MarkdownV2 this would effectively be a breaking change and I don't think it was the intent of #2827.

Looking at the current template - I don't see how it is HTML, am I missing something? https://github.com/prometheus/alertmanager/blob/main/template/default.tmpl#L105-L114

roidelapluie · 2022-04-11T13:03:27Z

Actually the API URl should come from DefaultGlobalConfig. i do not know why it is not the case.

metalmatze · 2022-06-03T10:00:25Z

I've looked into the parseMode issue again. It seems that Telegram doesn't allow the - in Markdown messages.
This character is used many times in messages.

Most notably the - is used to list all labels of an alert:

alertmanager/template/default.tmpl

Lines 8 to 10 in f958b8b

    
           {{ range .Labels.SortedPairs }} - {{ .Name }} = {{ .Value }} 
        
           {{ end }}Annotations: 
        
           {{ range .Annotations.SortedPairs }} - {{ .Name }} = {{ .Value }}

Unless we want to change the default template for all receivers, we should probably make HTML the default parseMode...

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

metalmatze · 2022-06-03T11:28:58Z

Not sure why the tests fail. I can try rebasing later.

gotjosh · 2022-06-27T13:53:45Z

Once you rebase this should be fixed - it relates to #2899

prometheus/alertmanager#2879

notify/telegram: Set API and ParseMode defaults

45c9966

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

gotjosh approved these changes Apr 8, 2022

View reviewed changes

gotjosh requested changes Apr 11, 2022

View reviewed changes

metalmatze changed the title ~~notify/telegram: Set API and ParseMode defaults~~ notify/telegram: Set API URL and ParseMode defaults Jun 3, 2022

metalmatze added 2 commits June 3, 2022 12:04

notify/telegram: Update to use default telegram config

d4dda44

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

notify/telegram: Fix test

4821358

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

metalmatze mentioned this pull request Jun 27, 2022

telegram: Bad Request: can't parse entities: Unsupported start tag \"\" at byte offset #2960

Closed

roidelapluie approved these changes Jun 27, 2022

View reviewed changes

metalmatze closed this Jul 4, 2022

metalmatze deleted the telegram-defaults branch July 4, 2022 20:21

metalmatze mentioned this pull request Jul 4, 2022

notify/telegram: Set API URL and ParseMode defaults #2981

Merged

Sh4kE added a commit to Sh4kE/k8s-projects that referenced this pull request Nov 15, 2022

set default parse mode to html

617fe29

prometheus/alertmanager#2879

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notify/telegram: Set API URL and ParseMode defaults #2879

notify/telegram: Set API URL and ParseMode defaults #2879

metalmatze commented Apr 5, 2022

gotjosh left a comment

gotjosh Apr 8, 2022

roidelapluie Apr 8, 2022

gotjosh Apr 11, 2022

roidelapluie Apr 11, 2022

gotjosh Apr 8, 2022

roidelapluie Apr 8, 2022

gotjosh Apr 11, 2022

gotjosh left a comment

gotjosh Apr 11, 2022

gotjosh Apr 11, 2022

roidelapluie commented Apr 11, 2022

metalmatze commented Jun 3, 2022

metalmatze commented Jun 3, 2022

gotjosh commented Jun 27, 2022

	DefaultTelegramConfig = TelegramConfig{
	NotifierConfig: NotifierConfig{
	VSendResolved: true,
	},
	DisableNotifications: false,
	Message: `{{ template "telegram.default.message" . }}`,
	ParseMode: "MarkdownV2",
	}

notify/telegram: Set API URL and ParseMode defaults #2879

notify/telegram: Set API URL and ParseMode defaults #2879

Conversation

metalmatze commented Apr 5, 2022

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie commented Apr 11, 2022

metalmatze commented Jun 3, 2022

metalmatze commented Jun 3, 2022

gotjosh commented Jun 27, 2022