bf: S3C-1427 use expired interval to average out throughput #268
Conversation
|
Also, this should not be added to master. Instead, changes should be added to Arsenal master branch with support for multiple sites |
There was a problem hiding this comment.
Architecturally I think the backbeat API server is not the right place to do computation to return averages, etc. because it has only minimal info available to do this computation. We can think about how to do the integration with Prometheus later and how to just expose the needed raw stats, so that all the derived values are mashed by Prometheus itself.
This being said, in the short term since it improves the currently exposed stats I think we should keep this change anyway, left some comments though.
| const lastInterval = | ||
| this._statsClient._normalizeTimestamp(new Date(now)); | ||
| // in seconds | ||
| const diff = Math.floor((now - lastInterval) / 1000); |
There was a problem hiding this comment.
Is the Math.floor needed? Can't we just keep the original sub-second precision we have?
lib/api/BackbeatAPI.js
Outdated
| // Get average for last interval depending on time surpassed | ||
| // so far for newest interval | ||
| total += | ||
| Math.floor(((INTERVAL - diff) / INTERVAL) * r.requests[3]); |
There was a problem hiding this comment.
same about Math.floor
lib/api/BackbeatAPI.js
Outdated
|
|
||
| // Get average for last interval depending on time surpassed | ||
| // so far for newest interval | ||
| total += |
There was a problem hiding this comment.
You could condition this line to timeSinceStart >= EXPIRY and always return (total / EXPIRY).toFixed(2)
When an interval of data in Redis expires, throughput will abruptly reduce. Given the data we collect, we can only calculate the average throughput. To ease the erratic decrease on expiration of an interval, instead, get the average of elapsed time for the newest interval and remaining time of the interval multiplied against the average of the just-expired interval. In Redis, we need to save an extra interval to reference the just-expired data.
philipyoo
force-pushed
the
bf/S3C-1427-avgThroughputBetweenExpiredIntervals
branch
from
05906a2
to
18fb8b8
Compare
ironman-machine
dismissed
dora-korpar’s stale review
Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)
|
PR has been updated. Reviewers, please be cautious. |
|
Yup, I think prometheus has a lot to offer in terms of functionality as well, so should be a good refactor for crr metrics, and possibly metrics for other extensions in backbeat 👍 |
|
@ironman-machine |
|
Hello @jonathan-gramain "RUN_BACKBEAT_CRR_TESTS=1": Success |
|
⌛ PR is now pending. CI build url: http://ci.ironmann.io/gh/scality/Integration/21575 |
|
@ironman-machine |
|
Hello @philipyoo "RUN_BACKBEAT_CRR_TESTS=1": Success {
"RUN_BACKBEAT_CRR_TESTS": "1",
"DEFAULT_BRANCH": "rel/7.4",
"SCALITY_INTEGRATION_BRANCH": "ultron/rel/7.4",
"REPO_NAME": "backbeat",
"SCALITY_BACKBEAT_BRANCH": "bf/S3C-1427-avgThroughputBetweenExpiredIntervals"
} |
|
💔 ☔ circleCI test failed. |
|
@ironman-machine |
|
Hello @philipyoo "RUN_BACKBEAT_CRR_TESTS=1": Success {
"RUN_BACKBEAT_CRR_TESTS": "1",
"DEFAULT_BRANCH": "rel/7.4",
"SCALITY_INTEGRATION_BRANCH": "ultron/rel/7.4",
"REPO_NAME": "backbeat",
"SCALITY_BACKBEAT_BRANCH": "bf/S3C-1427-avgThroughputBetweenExpiredIntervals"
} |
|
Integration succeeded in http://ci.ironmann.io/gh/scality/Integration/21601 |
|
Hello @jonathan-gramain "r-": Success |
jonathan-gramain
deleted the
bf/S3C-1427-avgThroughputBetweenExpiredIntervals
branch
When an interval of data in Redis expires, throughput will
abruptly reduce. Given the data we collect, we can only
calculate the average throughput. To ease the erratic
decrease on expiration of an interval, instead, get the
average of elapsed time for the newest interval and
remaining time of the interval multiplied against the
average of the just-expired interval.
In Redis, we need to save an extra interval to reference
the just-expired data.