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

Include screenshot URL to Telegram message #240

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nutmos
Copy link
Contributor

@nutmos nutmos commented Aug 31, 2024

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

@nutmos nutmos marked this pull request as ready for review August 31, 2024 13:36
@nutmos nutmos requested a review from a team as a code owner August 31, 2024 13:36
@nutmos nutmos marked this pull request as draft August 31, 2024 13:37
@nutmos nutmos marked this pull request as ready for review September 3, 2024 14:05
@nutmos
Copy link
Contributor Author

nutmos commented Oct 19, 2024

@yuri-tceretian Could you please help me review the PR? Thank you.

@yuri-tceretian
Copy link
Contributor

yuri-tceretian commented Oct 21, 2024

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?

@nutmos
Copy link
Contributor Author

nutmos commented Oct 22, 2024

@yuri-tceretian I updated the description already. Thanks for your feedback.

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 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

Screenshot

image

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

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.

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 {
Copy link
Contributor

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.

@nutmos
Copy link
Contributor Author

nutmos commented Oct 23, 2024

@yuri-tceretian Thank you for your suggestions. That makes sense. I will do it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Include screenshot URL to Telegram message
2 participants