-
Notifications
You must be signed in to change notification settings - Fork 23
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
Adjusts log levels for ErrExceedsMaxMempoolSize #692
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #692 +/- ##
==========================================
- Coverage 59.68% 59.67% -0.02%
==========================================
Files 51 53 +2
Lines 9057 9264 +207
==========================================
+ Hits 5406 5528 +122
- Misses 3645 3730 +85
Partials 6 6 |
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
(with a suggestion)
assertions/confirmation.go
Outdated
backoffLogLevel *= 2 | ||
logLevel := log.Error | ||
if backoffLogLevel > time.Minute { | ||
backoffLogLevel = time.Minute | ||
} else { | ||
logLevel = log.Warn | ||
} |
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.
seeing the number of times this code block is being repeated, do you think its worth it to incorporate this logic in one place?
(thinking out loud here) maybe something simpler like having a backoff duration set to ephemeral handler itself and define a new method on it LogLevelAtCurrentBackOff( time.Duration )
that would return what initial loglevel we want to begin with?
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.
I agree with this comment.
Furthermore, I really feel like this whole feature should be encapsulated in a customer Logger https://pkg.go.dev/log/slog#Logger or handler https://pkg.go.dev/log/slog#Handler and we should be able to just plug it into the go-ethereum/log library directly. It seems wrong that the callers have to do this level of configuration each time they want to say, "treat this message I'm about to log specially."
Sorry to drop a comment which basically reads like, "go back to the drawing board" so long after you requested the review and after you've already gotten OffchainLabs/nitro#2719 merged.
I'm willing to approve this one and merge it in for now, but we should open a Linear ticket for improving our logging code to be more decoupled from our logic. Let me know what you think.
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.
I decided to keep it simple and removed the backoff pattern for now.
We can revisit this decision under the logging improvement ticket umbrella later.
Similarly to what was done with StopWaiter, EphemeralErrorHandler was copied from https://github.com/OffchainLabs/nitro.
Since the goal of the ticket related to this PR is simple, I decided to move forward with a straightforward approach, and "took advantage" of the precedence created by the StopWaiter case, and just copied the EphemeralErrorHandler.
In this kind of scenario I usually like to create another git repo, that could be used as a dependency on nitro and bold.
But in previous experiences this type of change created lots of discussions around if it is worth to maintain one more repo, if we should go with a monorepo approach, or if we should just copy and paste, etc.
I wanted to avoid going into this rabbit hole backed up by a change as simple as the one that this PR is trying to achieve, ROI would not be great :(
I think it is a good idea to create a linear ticket related to improving code around logging, not only on bold but also on nitro, and then adding it to triage.
At least nitro relies a lot on a centralized logger, so a change like you are proposing will need a significant effort.
This was discussed once in a team's meeting by the way, and it was decided to postpone changes on the logging pattern.
I raised an issue that, when inspecting log lines of system tests runs that have more than one node, it is not straightforward to identify which node produced a specific log line, and improving that will require a significant effort, mainly due the fact that nitro uses a centralized logging pattern.
assertions/confirmation.go
Outdated
backoffLogLevel *= 2 | ||
logLevel := log.Error | ||
if backoffLogLevel > time.Minute { | ||
backoffLogLevel = time.Minute | ||
} else { | ||
logLevel = log.Warn | ||
} |
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.
I agree with this comment.
Furthermore, I really feel like this whole feature should be encapsulated in a customer Logger https://pkg.go.dev/log/slog#Logger or handler https://pkg.go.dev/log/slog#Handler and we should be able to just plug it into the go-ethereum/log library directly. It seems wrong that the callers have to do this level of configuration each time they want to say, "treat this message I'm about to log specially."
Sorry to drop a comment which basically reads like, "go back to the drawing board" so long after you requested the review and after you've already gotten OffchainLabs/nitro#2719 merged.
I'm willing to approve this one and merge it in for now, but we should open a Linear ticket for improving our logging code to be more decoupled from our logic. Let me know what you think.
util/log/log.go
Outdated
} | ||
|
||
if *h.FirstOccurrence == (time.Time{}) { | ||
*h.FirstOccurrence = time.Now() |
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.
One trick I've learned, and that I always try to use when I'm writing library code that uses time, is that if you'll make the time library stubbable or mockable, you can avoid having to call sleep in your test code which really speeds up the test execution time and doesn't really compromise the integrity of your qualification. Plus, if you really want to make sure it works with the "real" implementation of time, you can usually write a simple "happy-path" test that uses the real time library and do all your complicated edge-case checking logic using the stubbed implementation.
I should probably write a blog post about this, it comes up frequently enough.
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.
Yes I agree, I usually like to rely on dependency injection to provide a stubbed "Clock" implementation in tests, or "Clock" implementation with a real clock in production.
But as mentioned in other comment, this file was copied from Nitro.
I will keep the way it is so it is in sync with what we have on nitro.
…ot submit latest assertion to L1' errors
c822d29
to
d078088
Compare
cb8cec3
to
ad8a745
Compare
Adjusts log levels for ErrExceedsMaxMempoolSize.
With this PR logs related to ErrExceedsMaxMempoolSize are initially logged as warnings, and only if they persist for a while that will be logged as errors.
Similarly to what was done with StopWaiter, EphemeralErrorHandler was copied from https://github.com/OffchainLabs/nitro.
Resolves NIT-2785