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 #1296

Merged
merged 2 commits into from
Jan 14, 2015

Conversation

gphat
Copy link
Contributor

@gphat gphat commented Jan 14, 2015

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!

@gphat
Copy link
Contributor Author

gphat commented Jan 14, 2015

Note: This is a replacement for #1266. I used git before being caffeinated and seem to've ruined my branch. :)

@gphat gphat changed the title Modernize marathon plugin [marathon] service checks and improvements Jan 14, 2015

if r.status_code != 200:
self.status_code_event(url, r, aggregation_key)
self.warning("Got %s when hitting %s" % (r.status_code, url))
self.status_code_event(url, r)
Copy link
Member

Choose a reason for hiding this comment

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

This function does not exist anymore?
You may want to use r.raise_for_status() in the

try: 
     r = requests.get()
     r.raise_for_status()
except timeout:
    <service_check_criticial>
except httperror:
    <service_check_criticial>
else:
    <service_check_ok>`

above (don't know for marathon, but other 2XX status may be OK) and you can get rid of this extra if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know about that. Thanks!

@gphat
Copy link
Contributor Author

gphat commented Jan 14, 2015

Updated with your feedback. Thanks for dealing with my dumb re-send of this. :P

@LeoCavaille
Copy link
Member

👍

LeoCavaille added a commit that referenced this pull request Jan 14, 2015
[marathon] service checks and improvements
@LeoCavaille LeoCavaille merged commit f910715 into DataDog:master Jan 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants