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

Add extended flag to CPU collector #577

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

scottcunningham
Copy link
Contributor

This allows us to produce both simple and complex metrics - this way, simple metrics can be aggregated, but we still have the option to drill-down into per-host metrics in situations that require them (eg. checking steal on an individual VM)

This allows us to produce both simple and complex metrics - this way, simple metrics can be aggregated, but we still
have the option to drill-down into per-host metrics in situations that require them (eg. checking steal on an
individual VM)
@coveralls
Copy link

Coverage Status

Coverage remained the same at 59.729% when pulling 2bede02 on Ensighten:extended_cpu_metrics_upstream into ab59e12 on python-diamond:master.

@shortdudey123
Copy link
Member

This should be documented and tests updated

@coveralls
Copy link

Coverage Status

Coverage remained the same at 59.729% when pulling 85e92f3 on Ensighten:extended_cpu_metrics_upstream into ab59e12 on python-diamond:master.

@scottcunningham
Copy link
Contributor Author

@shortdudey123 I've added docs (I ran make docs and then replaced the INSERT_EXAMPLES_HERE with the examples previously there), tried to start looking at tests but it seems like many tests are broken in master:

Ran 478 tests in 2.439s

FAILED (failures=2, errors=8, skipped=4)
make: *** [test] Error 1

They seem to be legitimate errors (eg. 'NoneType' object has no attribute 'CommandGenerator', False is not true etc), so it's probably not a problem with missing libraries or somesuch. Are there any docs on how the tests work or how to run a subset of the tests?

@shortdudey123
Copy link
Member

Travis shows all tests passing currently so that means you are missing a needed library i your dev environment
Take a look at https://github.com/python-diamond/Diamond/blob/master/.travis.requirements.txt

@scottcunningham
Copy link
Contributor Author

Thanks @shortdudey123, will give it a go tomorrow and hopefully return with some tests!

Are there any docs about this, or general dev setup, or a CONTRIBUTING.md with details about requirements for PRs? I couldn't find any when I looked but I might be missing something obvious. If not, I'm more than happy to contribute docs or instructions. Should I open a new issue for this?

@shortdudey123
Copy link
Member

There really only is 1 doc file for development and it isn't too large:
https://github.com/python-diamond/Diamond/tree/master/docs/Development

A while back i added a vagrant box that does most of the setup and testing:
https://github.com/python-diamond/Diamond/blob/master/Vagrantfile#L78-L120

@coveralls
Copy link

Coverage Status

Coverage decreased (-39.6%) to 20.084% when pulling c810a9a on Ensighten:extended_cpu_metrics_upstream into 8c7ea91 on python-diamond:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-39.6%) to 20.084% when pulling c810a9a on Ensighten:extended_cpu_metrics_upstream into 8c7ea91 on python-diamond:master.

@scottcunningham
Copy link
Contributor Author

Thanks for the info @shortdudey123. I've added tests for the extended flag, and also the simple flag (since I needed something to base the extended flag tests on and there were none).

Not sure why coveralls think the coverage has decreased so drastically, I'll take a look at that.

@kormoc
Copy link
Contributor

kormoc commented Jan 21, 2017

Would it make more sense to add two new flags:

  • aggregate defaulting to false
  • individual defaulting to true

and having simple be an alias for aggregate true and individual false?

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