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

Add basic mesos cllector #106

Merged
merged 19 commits into from
Mar 3, 2017
Merged

Add basic mesos cllector #106

merged 19 commits into from
Mar 3, 2017

Conversation

janisz
Copy link
Contributor

@janisz janisz commented Feb 19, 2015

Collector retrive all data from /metrics/snapshot for one host provided in configuration.

nhlfr
nhlfr reviewed Feb 19, 2015
View reviewed changes
src/collectors/mesos/mesos.py Outdated
stats = set(x) | set(y)
summed_stats = {}
for k in stats:
summed_stats.update({k: x.get(k, 0) + y.get(k, 0)})
Copy link

Choose a reason for hiding this comment

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

I'd suggest dict comprehension here

summed_stats = {
    key: x.get(key, 0) + y.get(key. 0)
    for key in stats
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@josegonzalez
Copy link
Member

@deejay1 this seems reasonable, thoughts on merging?

@janisz janisz force-pushed the mesos_collector branch 2 times, most recently from f182e6f to ff37a0b Compare March 11, 2015 08:09
@deejay1
Copy link
Contributor

deejay1 commented Mar 11, 2015

@josegonzalez After the fixes now 👍

@nhlfr
Copy link

nhlfr commented Mar 11, 2015

LGTM 👍

@janisz janisz force-pushed the mesos_collector branch 3 times, most recently from 13e3910 to f487aeb Compare September 16, 2015 09:00
@janisz
Copy link
Contributor Author

janisz commented Sep 16, 2015

I was sure that this was merged. I rebased on top of the master. Unfortunately this version is not compatible with previous one. @MichaelDoyle what do you think about using / instead of dots? In graphite it will be spited anyway.

@MichaelDoyle
Copy link
Contributor

@janisz first pass looks pretty useful. going to try and run this locally and see what the diff looks like

bmhatfield
bmhatfield reviewed Sep 16, 2015
View reviewed changes
src/collectors/mesos/mesos.py
return len(value) - decimal - 1

def _sanitize_metric_name(self, name):
return name.replace('.', '_').replace('/', '_')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of this change. I prefer the nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used only on framework_id and executor_id. So for this input

...
  "frameworks": [
    {
      "checkpoint": true,
      "completed_executors": [],
      "executors": [],
      "failover_timeout": 604800,
      "hostname": "master.mesos.local",
      "id": "105146",
      "name": "marathon-0.7.6",
      "role": "*",
      "user": "root"
    }
  ]
...
{
    "executor_id": "com.domain.group_anotherApp.08002432371b",
    "executor_name": "Command Executor (Task: com.domain.group_anotherApp.08002432371b) (Command: sh -c 'start...')",
    "framework_id": "105146",
    "source": "com.domain.group_anotherApp.08002432371b",
    "statistics": {
      "cpus_limit": 1.6,
      "cpus_system_time_secs": 2.19,
      "cpus_user_time_secs": 7.74,
      "mem_anon_bytes": 103370752,
      "mem_file_bytes": 86016,
      "mem_limit_bytes": 557842432,
      "mem_mapped_file_bytes": 45056,
      "mem_rss_bytes": 103456768,
      "timestamp": 1422535725.82344
    }
 }

you will get following metric:

"frameworks.marathon-0_7_6.executors.com_domain_group_anotherApp.mem_mapped_file_bytes": 45056

If there will be nesting you will end up with nested version of marathon in framework branch. But I think executor_id could be spited so there will be tree com/domain/group/. Good catch I will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. It its only used for framework_name

@MichaelDoyle
Copy link
Contributor

Are these extra URLs documented anywhere? Specifically:

monitor/statistics.json
slave(1)/state.json

Only metrics/snapshot seems to be mentioned by the Mesos monitoring documentation. It worries me that these could be brittle and subject to change. Also, there seems be be overlap. Many of the stats being collected from these endpoints are in fact available from metrics/snapshot.

@janisz
Copy link
Contributor Author

janisz commented Sep 17, 2015

@deejay1
Copy link
Contributor

deejay1 commented Oct 7, 2015

Hey, any updates on this?

@janisz
Copy link
Contributor Author

janisz commented Oct 7, 2015

Next Mesos release will remove/rename some endpoints. I need to handle it in this PR.

@kamilchm
Copy link

kamilchm commented Oct 7, 2015

slave(1) prefix in endpoint comes from:
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L125
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/process.cpp#L154

@janisz
Copy link
Contributor Author

janisz commented Oct 13, 2015

I changed slave(1)/state.json to slave/state.json becouse they are returning same values and slave/state.json is used in Mesos tools and UI.

@MichaelDoyle How I should handle API changes in Mesos 0.25, should I add config option version or use new endpoint as a fall back?

@janisz
Copy link
Contributor Author

janisz commented Feb 28, 2017

@shortdudey123 @josegonzalez Are you interested in merging this PR after I rebase it?

@shortdudey123
Copy link
Member

@janisz yes please :)
I will review it more after that

@janisz
Copy link
Contributor Author

janisz commented Mar 1, 2017

@shortdudey123 Rebased. I included commits that we added since this PR was started. There are some bugfixes and features. I think preserving history of this commits will be good to see who and why made specific change.

@janisz janisz changed the title Add basic messos cllector Add basic mesos cllector Mar 1, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e784b43 on janisz:mesos_collector into ** on python-diamond:master**.

shortdudey123
shortdudey123 requested changes Mar 1, 2017
View reviewed changes
Copy link
Member

@shortdudey123 shortdudey123 left a comment

Choose a reason for hiding this comment

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

docs/collectors/MesosCollector.md also needs to be regenerated to pick up the updated help section

src/collectors/mesos/mesos.py Outdated


class MesosCollector(diamond.collector.Collector):
def __init__(self, config=None, handlers=[], name=None, configfile=None):
self.master = True
Copy link
Member

Choose a reason for hiding this comment

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

probably don't need this since this is the default that is set under get_default_config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

src/collectors/mesos/mesos.py Outdated
"""
Execute a Mesos API call.
"""
url = 'http://%s:%s/%s' % (host, port, path)
Copy link
Member

Choose a reason for hiding this comment

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

lets keep the scheme option in here and not remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/collectors/mesos/test/testmesos.py Outdated
"registrar.state_store_ms.p9999": (17.8412544, 6)
}

self.setDocExample(collector=self.collector.__class__.__name__,
Copy link
Member

Choose a reason for hiding this comment

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

self.setDocExample should only be called once (already called in test_should_work_for_slave_with_real_data). Remove this one or the other one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

@janisz
Copy link
Contributor Author

janisz commented Mar 2, 2017

Fixed issues. How can I generate docs? make docs is not working.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3e0e413 on janisz:mesos_collector into ** on python-diamond:master**.

@shortdudey123
Copy link
Member 

./build_doc.py --configfile=conf/diamond.conf -C mesos
./test.py -c mesos

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 05e9131 on janisz:mesos_collector into ** on python-diamond:master**.

@janisz
Copy link
Contributor Author

janisz commented Mar 3, 2017

Thanks, regenerated the docs.

@shortdudey123 shortdudey123 merged commit 5b12717 into python-diamond:master Mar 3, 2017
@janisz janisz deleted the mesos_collector branch March 3, 2017 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shortdudey123 shortdudey123 shortdudey123 approved these changes

Assignees
No one assigned
Labels
category: collector collector: mesos type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet