-
Notifications
You must be signed in to change notification settings - Fork 102
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
Multiplot/fetch_multi changes #460
Conversation
|
||
if r.status_code == 200: | ||
return [m['metric'] for m in r.json()] | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding retries over here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People have been using it without a problem I think, and I've had to work on so many other issues that I'm not going to do this one yet.
a0d8e81
to
c21dd40
Compare
|
||
def complete(self, metric, query_depth): | ||
metric_parts = metric.split('.') | ||
if query_depth < len(metric_parts): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally up to you, but what do you think about consolidating these if statements into just one block like, if (query_depth < len(metric_parts)) or (self.enable_submetrics and (query_depth == len(metric_parts)))
These files have been updated per the review from chinmay-gupte.
Note that if you only want to see the diffs since the initial |
# names, so return leaf nodes for each submetric alias that is | ||
# enabled | ||
if query_parts[-1] == '*': | ||
for metric in self.find_metrics('.'.join(query_parts[0:-1])): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the query ends with a *
but its not for a leaf node, it will make a call to find_metrics
here and because its not complete, it will make a call again to find_metrics
to return a branch node. Are these actual API calls or the data returned is cached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its doing the former, can we make just one API call in this function to find_metrics
and cache the data in a local variable?
The lastest diffs: |
yield BranchNode('.'.join(parts[:query_depth])) | ||
# First modify the pattern to get a superset that includes already complete | ||
# submetrics | ||
new_pattern = complete_pattern + '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the variable new_pattern
same as original query.pattern
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it isn't. It is a superset which is what I was trying to explain in the comment. For example, if query.pattern is "a.b.", it won't return "a.b", so new_pattern is "a.b", with no second ".". Does that make sense? I'm trying to eliminate the second api call by combining the two.
+1 after my last comment has been addressed. |
Multiplot/fetch_multi changes
Made a number of fixes/enhancements to the finder:
equidistant in time, (interleaved with Null datapoints if necessary.)
We weren't doing this so the timescale was off on graphs with sparse data.
(Note this required updating the graphite-api code from version 1.0.1 to
Head of master. Since we also had to patch that code I created a new repo
for it here: https://github.com/rackerlabs/graphite-api/tree/george/fetch_multi_with_patches
account. Multplot allows only 100 metrics per call, and has implicit limit of around
7000 json characters. So my changes batch up each of graphite-apis fetch_multi() calls
into to the minimum number of multiplot calls that still abide by those limits.
than just assuming "average", "num_points" or "latest" as we were before.
This required changes on both the search side, and the fetch_multi side.
This work was based on the changes suggested and created by Alex Weeks:
https://github.com/aweeks
https://23282e1499f9e238db58e26751fc6d96be63cc17-www.googledrive.com/host/0B28IWTWKORCkfnlLYmljbW1NQ295NWhWbmtfbDJ1UXYwQm9xdlVuMjN0NFV1QTNmcDltY2M/x/blueflood.html