Align log buffer to work with logs limits config #11781
Conversation
- Used FastExecLogsHigh - limit the number of logs we save in a block for an upkeep - Used FastExecLogsHigh * NumOfLogUpkeeps - limit the amount of logs we save in a block for all upkeeps - Ensured these values can be set dynamically for the buffer, as the configs might change over time
amirylm
force-pushed
the
AUTO-8229-align-log-buffer-to-work-with-logs-limits-config
branch
from
3bc6137 to
f6c05eb
Compare
|
I see that you haven't updated any README files. Would it make sense to do so? |
| // defaultFastExecLogsHigh is the default upper bound / maximum number of logs that Automation is committed to process for each upkeep, | ||
| // based on available capacity, i.e. if there are no logs from other upkeeps. | ||
| // Used by Log buffer to limit the number of logs we are saving in memory for each upkeep in a block | ||
| defaultFastExecLogsHigh = 10 |
There was a problem hiding this comment.
I'm not sure what the significance of fastExec is here; going by the comment block would it make sense to just call this maxLogsPerUpkeep? Or is that too broad?
There was a problem hiding this comment.
I wanted to keep our terminology stable cross components, for each component it might be represented with a better name but then we have multiple names for the same variable.
See AUTO-7207 for more information
There was a problem hiding this comment.
Do we want to keep the overall limit in the buffer (maxLogsPerBlock before), NumOfLogUpkeeps * FastExecLogsHigh in ticket?
I think we had some discussion remaining and can finalise the exact NumOfLogUpkeeps terminology
There was a problem hiding this comment.
I started with NumOfLogUpkeeps but reduced it in f422a54
We should keep it for the log provider but for the buffer it seems redundant, as we anyway cap the number of logs per upkeep and the number of upkeeps in another place
There was a problem hiding this comment.
Is the number of upkeeps a hard cap? What do we expect if upkeeps go bigger than that cap, would we want the log buffer to increase indefinitely in size?
There was a problem hiding this comment.
I've reverted this part, means we will have hard limit based on the number of log upkeeps specified in the config
| @@ -109,7 +109,7 @@ func NewLogProvider(lggr logger.Logger, poller logpoller.LogPoller, packer LogDa | |||
| threadCtrl: utils.NewThreadControl(), | |||
| lggr: lggr.Named("KeepersRegistry.LogEventProvider"), | |||
| packer: packer, | |||
| buffer: newLogEventBuffer(lggr, int(opts.LookbackBlocks), maxLogsPerBlock, maxLogsPerUpkeepInBlock), | |||
| buffer: newLogEventBuffer(lggr, int(opts.LookbackBlocks), defaultFastExecLogsHigh), | |||
There was a problem hiding this comment.
I always find vars being passed as params a little deceptive; if the business logic only passes defaultFastExecLogsHigh as the parameter here, does it make sense to reduce the complexity here and just drop the parameter from this function, and then in your tests, change the value of the defaultFastExecLogsHigh per test?
There was a problem hiding this comment.
This is the first PR introducing this variable, in future PRs we will get this value in the log provider constructor and pass it over to log buffer constructor, i.e. this default value will be used only from within the buffer constructor in case the passed value is nil/0
This reverts commit f422a54.
…ogs-limits-config
|
SonarQube Quality Gate
|
amirylm
deleted the
AUTO-8229-align-log-buffer-to-work-with-logs-limits-config
branch
AUTO-8229
Description
The log buffer needs to accept a config (
FastExecLogsHigh) from the registry to set the upper limit for the number of logs that can be processed for a given upkeep.Changes
FastExecLogsHighto limit the number of logs we save in a block for an upkeepFastExecLogsHigh = 10