Skip to content
This repository has been archived by the owner on Mar 14, 2019. It is now read-only.

Redesign monitoring internals #30

Merged
merged 4 commits into from Jan 31, 2016
Merged

Conversation

davetron5000
Copy link
Contributor

Treating the rake task as the public interface here, but this would break anyone using the stuff in lib/monitoring

  • Created a real data type for the results of checks
  • Removed the "per queue" notifier, which was tightly coupled to its checker. Now librato notifier can handle all of them
  • This should make it easier to add more advanced checks/notifications.

To get confidence this won't break anything, I basically made the internal changes and kept running the monitoring integration test. Once I'd made all the changes without that breaking, I then updated the unit tests.

(The change in the monitoring integration test was just to add failure messages to the asserts. No actual changes were made)

This is in advance of another change I'm going to make to add stats around the class of failed jobs, so we can treat certain failures as more urgent than others without having to create a custom resque error handler. Wanted to get feedback on this first, which will make the overall change easier to understand (hopefully :)

Treating the rake task as the public interface here, but this would
break anyone using the stuff in lib/monitoring

* Created a real data type for the results of checks
* Removed the "per queue" notifier, which was tightly coupled to its
  checker.  Now librato notifier can handle all of them
* This should make it easier to add more advanced checks/notifications.

To get confidence this won't break anything, I basically made the
internal changes and kept running the monitoring integration test.  Once
I'd made all the changes without that breaking, I then updated the unit
tests.

(The change in the monitoring integration test was just to add failure
messages to the asserts. No actual changes were made)
@@ -0,0 +1,30 @@
module Monitoring
class CheckResult
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now captures the contract between checker and notifier. Before it was basically whatever and so it resulted in tight coupling between some checkers and notifiers.

davetron5000 added a commit that referenced this pull request Jan 31, 2016
@davetron5000 davetron5000 merged commit e9c4231 into master Jan 31, 2016
@davetron5000 davetron5000 deleted the redesign-monitoring-internals branch January 31, 2016 00:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants