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

Percent by type #122

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

Percent by type #122

wants to merge 13 commits into from

Conversation

bnkr
Copy link

@bnkr bnkr commented Aug 8, 2014

This is a "first thing that works" implementation for a query percent by <tag> as discussed in #121 which will result in graph targets roughly meaning (for percent by tag2):

asPercent(metric.tag1=x.tag2=y, sumSeries(metric.tag1=x.tag2=*))

This also works when you do something like percent by type sum by core, you get a metric like:

asPercent(sumSeries(metric.core=*.metric=idle), sumSeries(metric.core=*.metric=*))

Additionally I added a utility to introspect the process of generating a query and give a very simple graphite url for testing purposes. Could come in handy for other people.

I expect this pull request needs some refactoring but I wanted to create it early so you can get an idea of where I went with it.

A couple of things I intend to fix:

  • not constructing proper objects in the debugging utility -- it would be much better and probably pretty easy to load the Config and Preferences object.
  • I did I string.replace() hack to insert the asPercent context which is probably unnecessary but I didn't want to get too far into understanding how the function application works yet.
  • computing the context for the percent is pretty messy.

I think in general build_from_targets would be easier to change if it had a bit more of an object language, rather than just dicts everywhere. In particular I found it quite hard to work out if I was computing something which had already been done elsewhere.

@Dieterbe
Copy link
Contributor

Dieterbe commented Aug 8, 2014

I like query_explorer.py (and agree with your todo's for it)

@bnkr
Copy link
Author

bnkr commented Aug 18, 2014

Query explorer now loads the proper configuration and I tidied up build_from_targets a little.

I think that's enough to merge, but there's a few things which could be nice to consider in other patches:

  • graph is not limited to 100
  • graph unit is not changed to percent
  • if there was a standard location for the config we could default to /etc somewhere and no need for the user to write the file every time. I'm not sure if you need to do python packaging Stuff to make that happen though.
  • some tests fail on master, perhaps travis-ci could be used to avoid this
  • if you provide wrong network values for the es server you don't get errors, it just sits there uninteruptable (and maybe times out eventually?)
  • the targets are so long it makes the tooltips pretty unusable
  • build_from_targets could still be cleaner

@Dieterbe
Copy link
Contributor

i'm sorry i've been dropping the ball on this. i've been so busy.
hopefully i can find some time for this soon

@bnkr
Copy link
Author

bnkr commented Sep 19, 2014

No problem. If you have some things that you think I might be able to do to make it easier then let me know.

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