Add metrics to deliverorderer#562
Conversation
|
coverage: 91.321% (+0.06%) from 91.264% — liran-funaro:bft-metrics into hyperledger:main |
cendhu
left a comment
cendhu
left a comment
There was a problem hiding this comment.
Please try to avoid missing many things in a single PR. Smaller and focused PR would help to review and merge PRs quicker.
| Namespace: params.Namespace, | ||
| Subsystem: params.Subsystem, |
There was a problem hiding this comment.
utils/deliverorderer is used only by the sidecar. Hence, I wonder whether we should keep it as utils or move them under sidecar so that we can have sidecar as the namespace. Is the orderer team planing to use this package?
There was a problem hiding this comment.
I don't think there is a plan to use this package in the Orderer.
But I still think it is best as a standalone package.
- It complements the
deliveranddelivercommitterpackages. - It can be used externally.
- It allows a flatter project structure.
Anyway, even if we include it under the sidecar package, it still makes sense to allow setting the namespace and subsystem by the user.
There was a problem hiding this comment.
it still makes sense to allow setting the namespace and subsystem by the user.
Agreed.
But I still think it is best as a standalone package.
Now, it becomes unrelated to this PR. As deliverorder holds some of the core logic, I do feel now that it should belong to the sidecar similar to dependency graph. Most other packages in utils really sound like utilities without complex logics.
There was a problem hiding this comment.
The "complex logic" is a strong point regarding not considering this as a utility.
We can address this in a different PR.
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
| d.targetArrivalTime = time.Now().Add(d.params.SuspicionGracePeriodPerBlock * gapDuration) | ||
| targetArrivalTimeUnix := float64(d.targetArrivalTime.Unix()) | ||
| d.metrics.TargetArrivalDeadline.WithLabelValues(dataSourceIDLabel).Set(targetArrivalTimeUnix) | ||
| d.metrics.SuspicionRaisedTotal.WithLabelValues(dataSourceIDLabel).Inc() |
There was a problem hiding this comment.
Why are we raising the suspicion before waiting for the arrival deadline to pass? Or should we also include SuspicionConfirmedTotal along with Raised and Cleared?
There was a problem hiding this comment.
We can add SuspicionConfirmedTotal. It can give valuable information.
There was a problem hiding this comment.
I added SuspicionConfirmedTotal and added a test case to verify this and the target deadline.
I also modified TargetArrivalDeadline to have millisecond granularity.
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
3 participants
Type of change
Description
Related issues