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

[marathon] service checks and improvements #1266

Closed
wants to merge 21 commits into from
Closed

[marathon] service checks and improvements #1266

wants to merge 21 commits into from

Conversation

gphat
Copy link
Contributor

@gphat gphat commented Dec 26, 2014

What's This Do?

After working on the just-merged etcd plugin for #1235 I wanted to take a look at the Marathon plugin. Looks like the key problem I saw was already fixed in #1240. While I was here I:

  • Refactored some of the magic numbers and arrays up into class variables
  • Removed unnecessary includes
  • Switched to the service check API
  • Added the "disk" attribute from the metrics

Thanks!

I appreciate you taking the time to look at this. If I can do anything to help a merge, let me know!

@LeoCavaille LeoCavaille changed the title Improve marathon plugin [marathon] service checks and improvements Dec 29, 2014
@@ -53,38 +52,27 @@ def get_v2_apps(self, url, timeout):
return r.json
Copy link
Member

Choose a reason for hiding this comment

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

This compat can be dropped, also as get_v2_apps and get_v2_app_versions could be refactored in one single function with the endpoint as argument because they are the same.

Also that would be the occasion to drop timeout_event and status_code_event and inline the service checks directly in the function above. Wdyt?

@LeoCavaille
Copy link
Member

Hi @gphat , me again 😉
Thanks for the improvements, could you look at the few comments I made?

@LeoCavaille LeoCavaille added this to the 5.2.0 milestone Dec 29, 2014
@LeoCavaille LeoCavaille self-assigned this Dec 29, 2014
@gphat
Copy link
Contributor Author

gphat commented Dec 30, 2014

All great feedback as usual @LeoCavaille. Thanks for the suggestions. Pretty soon you'll have to give fewer of them.

@gphat
Copy link
Contributor Author

gphat commented Dec 30, 2014

This looks good locally

   marathon
    --------
      - instance #0 [OK]
      - Collected 137 metrics, 0 events & 1 service check

@LeoCavaille
Copy link
Member

Hey @gphat, looking again at your PR, there are just a few things missing:

  • you report a service check only for critical state but never an OK state
  • the service check is tagged by url in the get_json functino which means that we will report different service checks for the different urls, do we want that? Maybe we could just tag by the url base and not the full URL ?
  • to be more readable APP_METRICS could be wrapped and sorted like
APP_METRICS = [
    'a',
    'b',
]
  • also can you rebase and squash your commits against the current master, it looks like you have a merge conflict of some sort

Thanks

@gphat
Copy link
Contributor Author

gphat commented Jan 14, 2015

Somehow I've botched my merge and screwed things up. Lemme resend this. :)

@gphat
Copy link
Contributor Author

gphat commented Jan 14, 2015

Sorry for moving this, I suck. This is replaced by #1296.

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

Successfully merging this pull request may close these issues.

4 participants