adding warning logs for rebuiding in memory chain #157
Conversation
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
| @@ -84,6 +86,10 @@ func (ee *eventEnricher) filterEnrichEthLog(ctx context.Context, f *eventFilter, | |||
| } | |||
| } | |||
|
|
|||
| if blockNumber < 0 || transactionIndex < 0 || logIndex < 0 { | |||
There was a problem hiding this comment.
overflow check as spotted by the linter
|
will add more debug logs |
Signed-off-by: Chengxuan Xing <[email protected]>
internal/ethereum/blocklistener.go
Outdated
| @@ -426,7 +425,7 @@ func (bl *blockListener) rebuildCanonicalChain() *list.Element { | |||
| // It's possible the chain will change while we're doing this, and we fall back to the next block notification | |||
| // to sort that out. | |||
| if expectedParentHash != "" && mbi.parentHash != expectedParentHash { | |||
| log.L(bl.ctx).Debugf("Block listener canonical chain view rebuilt up to new re-org at block %d", nextBlockNumber) | |||
| log.L(bl.ctx).Warnf("rebuildCanonicalChain rebuild stopped at block: %d due to mismatch hash for parent block (%d): %s (expected: %s)", nextBlockNumber, nextBlockNumber-1, mbi.parentHash, expectedParentHash) | |||
There was a problem hiding this comment.
Is this the change @Chengxuan ?
I see a move from regular logs otherwise, to including some function call semantics in the log names.
That's not a general approach this repo uses. There's pros and cons to it... but the main thing I'm concerned about is consistency.
Would you mind just making the change to the information that the logs contain?
There was a problem hiding this comment.
I switched them back for consistency. I personally found it's easier to have a unique ID to filter out logs.
From ops point of view, I need a string that have a good summary of what a function does when searching for logs, and I assume that's an important function of method names. Therefore, I prefer including function name in the collected logs. (no matter it's in log context or the message itself) other than inventing another way of summarize what the logs line is for.
There was a problem hiding this comment.
We discussed offline that this is a good thing to work out separately, and needs to consider that the logging framework does provide this information (if you switch to JSON logging you'll see it)
Signed-off-by: Chengxuan Xing <[email protected]>
|
Thanks @peterbroadhurst both comments addressed |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
=======================================
Coverage 99.81% 99.81%
=======================================
Files 25 27 +2
Lines 1635 1650 +15
=======================================
+ Hits 1632 1647 +15
Misses 3 3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Chengxuan Xing <[email protected]>
adding more logs to highlight the situation in which the in memory canonical chain is getting rebuilt