-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Build custom alert message #3137
Conversation
Nice feature @k-tomoyasu — regarding the color of the preview button: we don't use that color so using btn-default would make sense. I'll think about having a better layout for this in the meantime. |
@kocsmy Thanks for your advice! |
When we get this in ? Could rest of integrations be done in follow-up PR? I would like to get this first for emails and for slack.. |
@jupe Thanks for your comment!
That's a big help if it is OK. |
For our case email&slack would be enough. Most probably we doesn’t need anything else, at least in near future. Currently slack alert is almost useless because it gives just title, not even link.. so this would be really welcome feature :) |
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.
Thank you for this pull request, @k-tomoyasu ! This is a great feature and apologies that I couldn't review it sooner.
I made a few comments inline in code, but I have two other things to discuss in general --
How about we use Mustache as the template language? It has two important benefits: ability to render preview in client side without having to call the server and more importantly: it's far more secure. While Jinja2 supposed to be secure, it still has quite a big attack surface.
For Email type notification it will be beneficial to be able to edit the subject too. How about we provide the user the option to customize "Alert short description" (will be used as subject for email, text for Slack and just first line for others) and "Alert description" (what you already have)?
There will be needed some UI work as well, but we can do this in a follow up once we work out the other details. And this is something we can help you with.
Thanks!
|
||
def upgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.add_column('alerts', sa.Column('template', sa.Text(), nullable=True)) |
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 put template
in the options
JSON object we already have in alerts
.
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.
okay, fix logic and remove migration file.
redash/models/__init__.py
Outdated
@@ -772,6 +773,10 @@ def evaluate(self): | |||
def subscribers(self): | |||
return User.query.join(AlertSubscription).filter(AlertSubscription.alert == self) | |||
|
|||
def render_template(self, showError=None): |
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.
showError
seems to be unused? Also should be show_error
.
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.
thanks! I've removed.
@arikfr
Thank you for comments! |
Apologies for letting this linger. I will try to get to it this week 🤞 |
Hello, I am also looking forward to this functionality. How soon does this appear in redash? |
"this week"... 2 months later here I am 😅 To avoid further review cycles, I did some changes myself:
The reason for the feature flag is that the UI still needs more work. I will work on the product and UX definition with @ranbena and we will do these changes ourselves (along with converting this page to React). But you can already use this in your setup 👍 Thank you for your effort here, this great feature and patience during the process. Once the build passes I will merge this. |
@k-tomoyasu it's not a big thing, but the email address you signed this commits with (k.tomoyasu@geniee.co.jp) is not associated with your GitHub account. This means that you won't show up in the contributors page on GitHub :-( You can fix this by verifying this email address in your account: https://help.github.com/en/articles/setting-your-commit-email-address. |
Merged 👍 |
@arikfr
Thank you for your kindness. But this email is invalid now... |
* build custom alert message * fit button color tone * pass existing test * fix typos * follow code style * add webhook alert description and avoid key error * refactor: create alert template module * follow code style * use es6 class, fix template display * use alerts.options, use mustache * fix email description * alert custom subject * add alert state to template context, sanitized preview * remove console.log 🙇 * chatwork custom_subject * add alert custom message. pagerduty, mattermost, hangoutschat * Pass custom subject in webhook destination * Add log message when checking alert. * Add feature flag for extra alert options.
Hi, how to enable this feature in version 8.0.0 |
Thanks |
@ranbena the link you referenced is for v9... @Vaidas737 in v8, you can enable it with an env var, but note that it won't be 100% compatible with the final implementation in v9. |
@arikfr thanks a lot |
* build custom alert message * fit button color tone * pass existing test * fix typos * follow code style * add webhook alert description and avoid key error * refactor: create alert template module * follow code style * use es6 class, fix template display * use alerts.options, use mustache * fix email description * alert custom subject * add alert state to template context, sanitized preview * remove console.log 🙇 * chatwork custom_subject * add alert custom message. pagerduty, mattermost, hangoutschat * Pass custom subject in webhook destination * Add log message when checking alert. * Add feature flag for extra alert options.
Hi @arikfr I know it is a year old feature and v9 is already there, but I'm still on v8.0.0. I didn't find a clear explanation how to enable it in v8. |
try |
This feature was replaced in V9 and is now always enabled (the environment variable has no effect). You can read about how to use it here. |
Build custom alert messages with query results.
Alert feature is useful but we cannot see detail about query result.
This feature enable us write custom alert messages with query results (using template engine
Jinja2
).WIP yet. I would appreciate if give me feedback.
alert destination
hipchatsetting
slack alert