FT: Add CRR statistics for Orbit #1162
Conversation
lib/utilities/reportHandler.js
Outdated
| method: 'getCRRStats', | ||
| error: err, | ||
| }); | ||
| return cb(err); |
There was a problem hiding this comment.
Backbeat being down should not cause the whole report to fail so it would be wiser to return (null, {}).
There was a problem hiding this comment.
Good point—I've updated to return (null, {}) in the case of any error in this method. I've left the error logs though.
lib/utilities/reportHandler.js
Outdated
| } | ||
| let body; | ||
| try { | ||
| body = JSON.parse(res.body); |
There was a problem hiding this comment.
request(...).json() would have the same effect
There was a problem hiding this comment.
Nice! I did not know about this.
lib/utilities/reportHandler.js
Outdated
| } | ||
| const stats = { | ||
| completions: { | ||
| count: parseFloat(completions.results.count), |
There was a problem hiding this comment.
The swagger definition expects a int64 instead of a float
There was a problem hiding this comment.
I was wondering about this: I think we may have to address this in the backbeat code because it reduces the size to a dividend of 1000, and is limited to the second decimal place (for example: here).
We could parse the value as a float and then multiply by 1000, but that limits us to an approximation of the byte count. For example, if replicating a 1024 byte object, we get a size of '1.02' so we can only deduce the value as 1020. Or, in the case of replicating a 1 byte object we get a size of '0.00' which seems undesirable.
There was a problem hiding this comment.
I think the approximation is fine, the volume of data that zenko's meant to handle makes the rounding errors not matter much :)
lib/utilities/reportHandler.js
Outdated
| log.debug('getting CRR stats', { method: 'getCRRStats' }); | ||
| // TODO: Reuse metrics code from Backbeat by moving it to Arsenal instead of | ||
| // making an HTTP request to the Backbeat metrics route. | ||
| const params = { url: 'http://localhost:8900/_/metrics/crr/all' }; |
There was a problem hiding this comment.
even if it's temporary, having a configuration would be easier because we currently need to support 4 different stacks (swarm, kube, federation, sandboxes) and not all of them are able to reach backbeat at localhost:8900.
There was a problem hiding this comment.
Updated to add backbeat as a configuration property.
|
PR has been updated. Reviewers, please be cautious. |
| "site": "us-east-2", | ||
| "type": "aws_s3" | ||
| }], | ||
| "backbeat": { |
There was a problem hiding this comment.
To make it usable out of the box and ease testing, could we handle CRR_METRICS_HOST and CRR_METRICS_PORT environment variables to update this directly in docker-endpoint.sh?
There was a problem hiding this comment.
You got it! Updated so that you can run the image with a custom endpoint as such:
docker run -e CRR_METRICS_HOST=10.100.3.139 -e CRR_METRICS_PORT=1337 a7ae6fa66070
|
Almost there for the temporary version :) |
|
PR has been updated. Reviewers, please be cautious. |
bennettbuchanan
force-pushed
the
ft/add-crr-all-metrics
branch
from
4a23bd6 to
6ede43b
Compare
|
PR has been updated. Reviewers, please be cautious. |
1 similar comment
|
PR has been updated. Reviewers, please be cautious. |
|
bennettbuchanan
force-pushed
the
ft/add-crr-all-metrics
branch
from
a56046e to
487e65f
Compare
|
PR has been updated. Reviewers, please be cautious. |
|
Ready to squash :) |
This is the first effort to use forthcoming Backbeat routes for retrieving metrics (to be added by scality/backbeat#200). The next step is to extract metrics accumulation code and move it to Arsenal so that we can avoid making an HTTP request to Backbeat.