feat: expose underlying nim-ethers errors to logs #985
Conversation
There was a problem hiding this comment.
Nice work. My main question is more around should we do more than just log these errors? Are they bad enough to raise a Defect (and crash the node)? Or should we add metrics and monitor how often they happen?
I'll approve now since the suggestions are not really blocked.
| proc onBlock(_: Block) = | ||
| proc onBlock(blckResult: ?!Block) = | ||
| if eventError =? blckResult.errorOption: | ||
| error "There was an error in block subscription", msg=eventError.msg |
There was a problem hiding this comment.
Part of me thinks if the clock fails to update due to fetching a block error, then we should raise a Defect, because a correct clock is critical for proper node function. However, we don't know if it's a temporary error or not.
We could add an error count and trigger a Defect after a few errors, but that sounds potentially like an over-optimisation without data yet. Maybe for now we should add a metric and monitor it? Wdyt?
| proc onSlotFilled(eventRes: ?!SlotFilled) = | ||
| if event =? eventRes: | ||
| slotIdxFilled = some event.slotIndex |
There was a problem hiding this comment.
Same here...
| proc onSlotFilled(eventRes: ?!SlotFilled) = | |
| if event =? eventRes: | |
| slotIdxFilled = some event.slotIndex | |
| proc onSlotFilled(event: ?!SlotFilled) = | |
| slotIdxFilled = some event.get.slotIndex |
| without event =? eventResult, eventErr: | ||
| error "There was an error in Request subscription", msg = eventErr.msg | ||
| return |
There was a problem hiding this comment.
Same comment for most of these event subscriptions in the Market abstraction -- they are critical to node functionality, so should we log and continue or are they severe enough that we cannot assume normal node operation afterwards (thus warranting a Defect)?
| proc onStorageRequested(eventRes: ?!StorageRequested)= | ||
| if event =? eventRes: | ||
| requestId = event.requestId.some |
There was a problem hiding this comment.
Wdyt about raising a Defect (and fail the test) if there's an error in the callback?
| proc onStorageRequested(eventRes: ?!StorageRequested)= | |
| if event =? eventRes: | |
| requestId = event.requestId.some | |
| proc onStorageRequested(event: ?!StorageRequested)= | |
| requestId = some event.get.requestId |
AuHau
force-pushed
the
feat/nim-ethers-subscription-result
branch
from
63fee49
to
8a0e3ac
Compare
Brings in a version of nim-ethers where subscription callbacks are returned Result, which will give an error when there is some underlying problem in the subscription mechanisms. This PR exposes these errors through logging.