Truncate data logged when fetching commit reports. #1036
Conversation
winder
requested review from
asoliman92,
matYang,
makramkd,
AmrMohamedRezk,
dimkouv,
0xAustinWang and
0xnogo
| @@ -776,6 +777,11 @@ func (l *DefaultAccessor) processCommitReports( | |||
| if len(reports) < limit { | |||
| return reports | |||
| } | |||
| lggr.Errorw("too many commit reports received, commit report results are truncated", | |||
There was a problem hiding this comment.
Is it an error? This can easily happen when we have higher load and not considered an error. I think Infow is good here.
There was a problem hiding this comment.
Yes, this is an error. There was a war room last week because of a delayed execution, it turned out the message wasn't being executed because it was truncated from the results.
There was a problem hiding this comment.
Can't really agree with it being an error if it's smth that's expected to happen. What we do afterwards is the issue.
There was a problem hiding this comment.
I thought we had some logic somewhere where we filter out price-only reports and paginate? Or is that not the case?
pkg/reader/ccip.go
Outdated
| func printReports(lggr logger.Logger, reports []cciptypes.CommitPluginReportWithMeta) { | ||
| tokenPriceUpdates := 0 | ||
| gasPriceUpdates := 0 | ||
| var truncatedReports []cciptypes.CommitPluginReportWithMeta |
There was a problem hiding this comment.
don't think this function cares whether they're truncated or not.
| var truncatedReports []cciptypes.CommitPluginReportWithMeta | |
| var reportToPrint []cciptypes.CommitPluginReportWithMeta |
|
| lggr.Errorw("too many commit reports received, commit report results are truncated", | ||
| "numTruncatedReports", len(reports)-limit) | ||
| for l := limit; l < len(reports); l++ { | ||
| lggr.Warnw("dropping commit report which doesn't fit in limit", "report", reports[l]) |
There was a problem hiding this comment.
Should we only log it if it has a merkle root? Otherwise we might spam price reports?
There was a problem hiding this comment.
Also maybe consider grouping the relevant roots into one log instead?
During a recent incident it was difficult to tell which reports were being returned by the get commit reports query. The token and fee price reports were too spammy. This PR improves the logging situation with two improvements: