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.

Sign up for GitHub

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

Jump to bottom

[mesos] Improve mesos integration #1535

Merged
merged 3 commits into from
Jun 1, 2015
Merged

[mesos] Improve mesos integration #1535

merged 3 commits into from
Jun 1, 2015

Conversation

DorianZaccaria
Copy link
Contributor

  • Add a check for mesos masters, only the leader will report metrics
  • Add a check for mesos slave, slaves will report metrics from the selected
    tasks only if the task is running on the node
  • Add a mocked tests for mesos integration

@alq666
Copy link
Member

alq666 commented Apr 13, 2015

@DorianZaccaria

  1. what's the different between cpu_percent and master.cpu_percent. Should we use tags instead? e.g. slave, master (This question applies to more metrics).
  2. mesos.state and mesos.stats are very similar and thus likely to trigger mistakes. What is in general, the difference between role and state?

@DorianZaccaria
Copy link
Contributor Author

I kept the metrics names from the old check to avoid breaking things during an upgrade. But we could also tag the old check as deprecated and keep it until the 6.0 release. For the later, we can reduce the number of metrics sent by the new check.
For state metrics, I think cpu_percent and master.cpu_percent are the same and are named differently for historical reasons. I have only one node in my environment so I can't check yet but I am working on it.
Stats metrics are the same across the cluster, as well as roles and frameworks. State metrics are more specific to the node itself (except for frameworks and slaves).
I noticed that some endpoints return the same metrics, for example /metrics/snapshot is included in /stats.json.

Let me know if you have any feedback .

@alq666
Copy link
Member

alq666 commented Apr 14, 2015

@DorianZaccaria let me know once you have a multi-node setup running; we can take a look at the metrics together.

@DorianZaccaria
Copy link
Contributor Author

Sure, I'm still in touch with mesosphere support to figure out why the cluster setup is failing.

@remh remh added this to the 5.4.0 milestone Apr 17, 2015
@degemer degemer mentioned this pull request Apr 23, 2015
Merged
@remh
Copy link
Contributor

remh commented May 11, 2015

@DorianZaccaria could you rebase your PR please ? it can't be merged right now.

remh
remh reviewed May 11, 2015
View reviewed changes
checks.d/mesos_master.py
# project
from checks import AgentCheck

# 3rd party
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny tiny nitpick but as per pep8 the import order should be:

#stdlib
#3p
#project

@remh
Copy link
Contributor

remh commented May 11, 2015

Great work @DorianZaccaria .
How do you think this should replace the old mesos check and how do you picture the transition from the old to the new checks ?

@DorianZaccaria DorianZaccaria force-pushed the dorian/mesos branch 3 times, most recently from 6c285f9 to bb4c654 Compare May 13, 2015 18:47
@DorianZaccaria
Copy link
Contributor Author

@remh I think this one can be merged before the 5.4 freeze.

@remh
Copy link
Contributor

remh commented May 26, 2015

@DorianZaccaria what's the status of the metric type updates ?

@DorianZaccaria
[mesos] Improve mesos integration
8d15b4a 
* Add a check for mesos masters, only the leader will report metrics
* Add a check for mesos slave, slaves will report metrics from the selected
tasks only if the task is running on the node
* Add a mocked tests for mesos integration
@DorianZaccaria
[mesos] Support multiple versions and Updates for the review
e033b23 
* Change mesos stats endpoint for versions above 0.22.0
* Simplify syntax
* Update tests for @degemer refactor
@DorianZaccaria DorianZaccaria force-pushed the dorian/mesos branch 2 times, most recently from a82e041 to 2476aa9 Compare May 28, 2015 22:05
@DorianZaccaria
Copy link
Contributor Author

@remh Done and updated.

remh
remh reviewed May 29, 2015
View reviewed changes
checks.d/mesos_slave.py
for key_name, (metric_name, metric_func) in m.iteritems():
metric_func(self, metric_name, stats_metrics[key_name], tags=tags)

self.SERVICE_CHECK_NEEDED = True
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, but usually upper case variables are for constants which is not the case here.

@remh
Copy link
Contributor

remh commented May 29, 2015

Looks good!
a few tiny nitpicks to fix and then it's ready to merge.

remh added a commit that referenced this pull request Jun 1, 2015
@remh
Merge pull request #1535 from DataDog/dorian/mesos
3a56270 
[mesos] Improve mesos integration
@remh remh merged commit 3a56270 into master Jun 1, 2015
@remh
Copy link
Contributor

remh commented Jun 1, 2015

💥 👍

@LeoCavaille LeoCavaille deleted the dorian/mesos branch June 1, 2015 22:25
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d036028 on dorian/mesos into ** on master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@remh remh

Labels
None yet
Projects
None yet
Milestone
5.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@DorianZaccaria @alq666 @remh @coveralls