-
Notifications
You must be signed in to change notification settings - Fork 366
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
INDY-1475: Preparation for statistics processing script #843
Conversation
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@@ -703,6 +703,7 @@ def trackBatches(self, pp: PrePrepare, prevStateRootHash): | |||
# tracked to revert this PRE-PREPARE | |||
self.logger.trace('{} tracking batch for {} with state root {}'.format( | |||
self, pp, prevStateRootHash)) | |||
self.metrics.add_event(MetricsType.THREE_PC_BATCH_SIZE, len(pp.reqIdr)) |
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.
Should we consider 3PC Batch size for different Replicas separately? maybe not now, but for future.
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.
@ashcherbakov yes, this is an interesting thing to add in future, as well as some others, like message sizes per type
|
||
assert acc.count == len(values) | ||
assert acc.sum == sum(values) | ||
assert acc.avg == statistics.mean(values) |
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.
Why do we use statistics
module only in tests and not in ValueAccumulator
for calculation?
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.
@ashcherbakov because ValueAccumulator doesn't store values (there can be billions of them when analyzing metrics from a long run), and one of purposes of this test is to be sure that it's implementation is accurate enough
Signed-off-by: Sergey Khoroshavin sergey.khoroshavin@dsr-corporation.com