-
Notifications
You must be signed in to change notification settings - Fork 43
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
Include screenshot URL to Telegram message #240
base: main
Are you sure you want to change the base?
Conversation
@yuri-tceretian Could you please help me review the PR? Thank you. |
Hi @nutmos, thank you for contributing to the project! I really appreciate it. While I am reviewing the changes, can you please add more information about what the pull request does, format of the notification before and after the change? |
@yuri-tceretian I updated the description already. Thanks for your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a closer look and unfortunately, I cannot accept the proposed solution.
As you may know, there is a limit to the maximum length of the message - 4096 of UTF-16 characters. The URL, especially when it's a link provided by a cloud provider, can be very long (200-300 characters). Also, in the case of multiple alerts in a group, there could be multiple images for the message. Appending them to the message as text will likely make them truncated, which is no better than dropping them altogether. Also, from a perception standpoint, long URLs are not useful and clutter the message, especially when they go one after another.
I can think of an alternative solution - the request model of sendMessage
API offers field entities
(see https://core.telegram.org/bots/api#sendmessage). It allows clients to implement some WYSIWYG capabilities and format the plain text message. For example
curl --location 'https://api.telegram.org/bot$BOT_TOKEN/sendMessage' \
--form 'chat_id="$CHAT_ID"' \
--form 'entities="[{\"type\":\"text_link\", \"offset\": 10, \"length\": 7, \"url\":\"http://grafana.com\"}]"' \
--form 'text="Hello, 🎉 Grafana\!"'
produces the following message
This approach will let us save a lot of space in the message, and for example append screenshots like
.... Screenshots: [1],[2],[3]
where [1] is a clickable link.
However, there is a downside too - this approach works only when parse_mode
is not specified. However, the integration supports different formats such as HTML or Markdown. So, can't use entities in those cases. I think this is ok if we provide some alternatives (does not have to be in this PR).
Ideally, in such a configuration, we can populate template data with URLs. There are already fields in the template data model
alerting/templates/template_data.go
Lines 50 to 51 in e81931a
ImageURL string `json:"imageURL,omitempty"` | |
EmbeddedImage string `json:"embeddedImage,omitempty"` |
Also, there are examples in webhook and email integrations.
Actually, I think I can accept PR that just sets the ImageURL field in template data, and leaves it up to the custom template to include the URL in the message.
receivers/telegram/telegram.go
Outdated
if err := f.Close(); err != nil { | ||
tn.log.Warn("failed to close image", "error", err) | ||
// Works only if IncludeScreenshotURL is set to false. | ||
if !tn.settings.IncludeScreenshotURL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's invert this expression. Go's best practices recommend reducing nesting, and this is a good candidate.
@yuri-tceretian Thank you for your suggestions. That makes sense. I will do it later. |
This PR is for adding screenshot URL to Telegram messages.
This PR introduces
IncludeScreenshotURL
for a Telegram channel. When switch it on, all images will be sent to Telegram into 1 message as URLs. When switch it off, an image will be sent with the separate message.Fixes #123