fix: eth1 provider error logging #6863
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #6863 +/- ##
============================================
- Coverage 62.20% 62.19% -0.01%
============================================
Files 571 571
Lines 60021 60103 +82
Branches 1976 1976
============================================
+ Hits 37334 37380 +46
- Misses 22644 22680 +36
Partials 43 43 |
nflaig
force-pushed
the
nflaig/eth1-error-logging2
branch
from
d0f0428
to
2a2aeee
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
force-pushed
the
nflaig/eth1-error-logging2
branch
from
2a2aeee
to
09819d8
Compare
nflaig
force-pushed
the
nflaig/eth1-error-logging2
branch
from
09819d8
to
c339c6c
Compare
nazarhussain
previously requested changes
Jun 10, 2024
|
|
||
| return msSinceLastCall > minElapsedTime; | ||
| if (msSinceLastCall > minElapsedTime) { |
There was a problem hiding this comment.
I am not in favor of this change in createElapsedTimeTracker as it's just to track the total elapsed time without any frequency.
If there is no other usage then we can rename this to createTimeFrequencyTracker or similar to make it clear.
Or I would better suggest to use the rateLimiter here. We have few implementation that we can move to utils and reuse here.
There was a problem hiding this comment.
It's really unintuitive based on current usage of createElapsedTimeTracker that is would reset that timer if there are intermediate calls.
In the test cases, the function is even called callIfTimePassed which suggests call code if time passed, e.g. like this
if (callIfTimePassed()) {
// run this code after time passed
}The usage in eth1Provider is the same, the name of the function is isOneMinutePassed which suggest log the error if one minute is passed, irrespective of how many times it called isOneMinutePassed in-between.
My suggestion is to either fix createElapsedTimeTracker, or to remove it altogether has the only use case right now is in eth1Provider.ts and it does not what it is supposed to do there.
wemeetagain
approved these changes
Jun 24, 2024
|
🎉 This PR is included in v1.20.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, we will only log the error once if the eth1 provider throws continuous errors with a frequency of < 1min while the intended behavior should be to log at most an error every minute to not be silent about the issue but also not flood the logs with errors.
Description
Update
elapsedTimeTrackerto not reset timer if there are intermediate calls before time has passedPart of #6469