-
Notifications
You must be signed in to change notification settings - Fork 108
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
Send alerts to prometheus when a workflow fails to be processed. #10959
Conversation
Jenkins results:
|
Jenkins results:
|
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.
Kenyi, these changes are looking good. However, I think these alerts will be better organized if we actually have a wrapper function/method for each of them. We might even want to create a new module called something like MSTransferorAlerts
and encapsulate all this information in there.
If you feel like refactoring the notifyLargeData
method as well, that would be even better :-D
What do you think?
@amaltaro : Okay, I encapsulated the alerts in different methods. Could you please check the second commit? |
Jenkins results:
|
Jenkins results:
|
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.
These changes are looking much better Kenyi! I left a few comments along the code for your consideration though (I think I made single comments instead of a review, sorry!).
Alright! Just made the changes. |
@khurtado this is looking good now. Can you please squash those 3 commits into a single one? Thanks |
Jenkins results:
|
Rename notifyLargeData method. Fixes dmwm#10215
@amaltaro Done! |
Jenkins results:
|
Thanks Kenyi! |
Fixes #10215
Status
In development
Description
Send alerts to prometheus when a workflow fails to be processed.
Is it backward compatible (if not, which system it affects?)
YES