Alerting integration #542
Alerting integration #542
Conversation
Created notification content class
Feature/issue notification
Feature/issue by group
# Conflicts: # flask_monitoringdashboard/core/exceptions/exception_collector.py # flask_monitoringdashboard/database/exception_occurrence.py
…-issues Feature/config for GitHub issues
# Conflicts: # flask_monitoringdashboard/core/exceptions/exception_collector.py
Email notification, modifiable notifcation type
Added chat integration
Master pull
…eature/chat_integration
Feature/chat integration
mircealungu
left a comment
mircealungu
left a comment
There was a problem hiding this comment.
amazing work folks.
a few small points and I look forward to deploying this.
|
|
||
| [alerting] | ||
| ENABLED=False | ||
| TYPE=email |
There was a problem hiding this comment.
Do we really need the TYPE? We can simply test in the code for the existence of the ENVVARS? Or is there a case where we want to have the TYPE defined independently from the enviers?
There was a problem hiding this comment.
We kept the type property, since we think the config/code is more readable this way. But we can remove it if you want to, it shouldn't really be an issue
There was a problem hiding this comment.
Ok. Let's keep it then. But then I have another question: what happens if someone sets TYPE=email but forgets SMTP_HOST? Do we handle this situation?
There was a problem hiding this comment.
Yes, we log that the config is invalid and would turn off the email alerting until SMTP_HOST provided
|
|
||
| At the moment the secrets and variables for the issue are saved in a .env file | ||
|
|
||
| There is a template with file name `.env_template` that can be used to fill in the info needed for the issue creation alert to work. |
There was a problem hiding this comment.
I don't see such a file. And also, I see that the file above -- config.cfg -- already definies secrets and variables. So is this still needed?
There was a problem hiding this comment.
No it is not needed and should be deleted. Was a part of previous setup. Now the config.cfg is used
There was a problem hiding this comment.
Yeah, we will delete this README and provide some info in a separate rst doc file
| template = template_env.get_template('report.html') | ||
|
|
||
|
|
||
| class AlertContent: |
There was a problem hiding this comment.
A small comment explaining the use of this class would be good
There was a problem hiding this comment.
I don't know why but alert. Seems like a strange name for this module. Either alerting or alerts would somehow be more understandable for me.
|
|
||
| def create_teams_payload(alert_content: AlertContent): | ||
| return { | ||
| "type": "message", |
There was a problem hiding this comment.
haha. Why am I not surprised that the Microsoft alternative is the most complicated one?
| @@ -0,0 +1,16 @@ | |||
| class GitHubRequestInfo: | |||
There was a problem hiding this comment.
Do you really need this class?
There was a problem hiding this comment.
I guess we could do it without. Is that preferred?
| e_copy = _get_copy_of_exception(e) | ||
|
|
||
| if config.alert_enabled: | ||
| _notify(e_copy, session, config) |
There was a problem hiding this comment.
Why do I need a copy? Maybe at the comment too? It's not evident
There was a problem hiding this comment.
I'm not sure, we will look into it.
| return new_exc | ||
|
|
||
|
|
||
| def _notify( |
There was a problem hiding this comment.
Maybe move to the alerts package?
| templateUrl: 'static/pages/database_management.html', | ||
| controller: ['$scope', '$http', 'menuService', 'endpointService', 'modalService', DatabaseManagementController] | ||
| }) | ||
| .when('/alerting_settings', { |
There was a problem hiding this comment.
oh. we have also a UI for the alerting settings :) COOL!
| :return: A JSON-object with alert configuration details | ||
| """ | ||
| # TODO | ||
| # post_to_back_if_telemetry_enabled(**{'name': 'deploy_alert_config'}) |
There was a problem hiding this comment.
what is this for? not for sending telemetry info about the installation is it? because then email is too private to collect it.
There was a problem hiding this comment.
It should be posting back telemetry data about the endpoint being called. It doesn't send any sensitive info, just the name of the endpoint. We think it should be enabled, as it is enabled for every other endpoint. Do you agree with this?
There was a problem hiding this comment.
what's this method used for then? Why does it return email info? Is that not going to the telemetry?
There was a problem hiding this comment.
This method provides the information for the alerting settings UI
4 participants
No description provided.