-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Get request logs from past n blocks #11052
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
I see that you haven't updated any README files. Would it make sense to do so? |
logPollerCacheDurationSec := int64(pluginConfig.LogPollerCacheDurationSec) | ||
if logPollerCacheDurationSec <= 0 { | ||
lggr.Warnw("invalid logPollerCacheDuration, using 300 instead", "logPollerCacheDurationSec", logPollerCacheDurationSec) | ||
logPollerCacheDurationSec = 300 |
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.
const
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.
Can you explain what you mean by const
here? I'm not understanding why this should be a const variable.
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.
Bolek probably means because this is a magic number we should instead set it to a more descriptive named const such as const PastBlocksToPoll = 50
. My personal preferenece would also be to pull this either the top of the file or to a constants
area for easy reference and changing.
pastBlocksToPoll := int64(pluginConfig.PastBlocksToPoll) | ||
if pastBlocksToPoll <= 0 { | ||
lggr.Warnw("invalid pastBlocksToPoll, using 50 instead", "pastBlocksToPoll", pastBlocksToPoll) | ||
pastBlocksToPoll = 50 |
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.
const
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.
Here as well.
0dc1baf
to
8b98b57
Compare
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.
lgtm
return resultsReq, resultsResp, nil | ||
} | ||
|
||
func (l *logPollerWrapper) FilterPreviouslyDetectedEvents(logs []logpoller.Log, detectedEvents *detectedEvents, filterType string) []logpoller.Log { | ||
if len(logs) > maxLogsToProcess { | ||
l.lggr.Warnw("FilterPreviouslyDetectedEvents: too many logs to process, only processing latest maxLogsToProcess logs", "filterType", filterType, "nLogs", len(logs), "maxLogsToProcess", maxLogsToProcess) |
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.
Change to error level
SonarQube Quality Gate |
* Get request logs from past n blocks * Separate confirmations setting for reqs & resps * Filter already detected reqs & resps * validate pastBlocksToPoll * Fixed bugs which appeared during tests * Addressed feedback * Added test * Added comment to config * Fixed log & removed const * Used const values for defaults * Address feedback * Fixed types for logPoller * Added tests for FilterPreviouslyDetectedEvents * Added additional test assertions * Fixed lint errors
No description provided.