-
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?
Changes from all commits
83338a4
59fab7e
b23f7e6
9dad346
3f14446
257d8cc
8a9f80e
c822d29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import ( | |
"github.com/OffchainLabs/bold/challenge-manager/types" | ||
"github.com/OffchainLabs/bold/containers/option" | ||
retry "github.com/OffchainLabs/bold/runtime" | ||
utillog "github.com/OffchainLabs/bold/util/log" | ||
"github.com/ethereum/go-ethereum/accounts/abi/bind" | ||
"github.com/ethereum/go-ethereum/log" | ||
) | ||
|
@@ -55,6 +56,10 @@ func (m *Manager) keepTryingAssertionConfirmation(ctx context.Context, assertion | |
log.Error("Could not get prev assertion creation info", "err", err) | ||
return | ||
} | ||
|
||
backoffLogLevel := time.Second | ||
exceedsMaxMempoolSizeEphemeralErrorHandler := utillog.NewEphemeralErrorHandler(10*time.Minute, "posting this transaction will exceed max mempool size", 0) | ||
|
||
ticker := time.NewTicker(m.confirmationAttemptInterval) | ||
defer ticker.Stop() | ||
for { | ||
|
@@ -83,11 +88,24 @@ func (m *Manager) keepTryingAssertionConfirmation(ctx context.Context, assertion | |
confirmed, err := solimpl.TryConfirmingAssertion(ctx, creationInfo.AssertionHash, prevCreationInfo.ConfirmPeriodBlocks+creationInfo.CreationBlock, m.chain, m.averageTimeForBlockCreation, option.None[protocol.EdgeId]()) | ||
if err != nil { | ||
if !strings.Contains(err.Error(), "PREV_NOT_LATEST_CONFIRMED") { | ||
log.Error("Could not confirm assertion", "err", err, "assertionHash", assertionHash.Hash) | ||
backoffLogLevel *= 2 | ||
logLevel := log.Error | ||
if backoffLogLevel > time.Minute { | ||
backoffLogLevel = time.Minute | ||
} else { | ||
logLevel = log.Warn | ||
} | ||
Comment on lines
+91
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
logLevel = exceedsMaxMempoolSizeEphemeralErrorHandler.LogLevel(err, logLevel) | ||
|
||
logLevel("Could not confirm assertion", "err", err, "assertionHash", assertionHash.Hash) | ||
errorConfirmingAssertionByTimeCounter.Inc(1) | ||
} | ||
continue | ||
} | ||
|
||
exceedsMaxMempoolSizeEphemeralErrorHandler.Reset() | ||
backoffLogLevel = time.Second | ||
|
||
if confirmed { | ||
assertionConfirmedCounter.Inc(1) | ||
log.Info("Confirmed assertion by time", "assertionHash", creationInfo.AssertionHash) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
load("@io_bazel_rules_go//go:def.bzl", "go_library") | ||
|
||
go_library( | ||
name = "log", | ||
srcs = ["log.go"], | ||
importpath = "github.com/OffchainLabs/bold/util/log", | ||
visibility = ["//visibility:public"], | ||
deps = ["@com_github_ethereum_go_ethereum//log"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
package log | ||
|
||
import ( | ||
"strings" | ||
"time" | ||
|
||
"github.com/ethereum/go-ethereum/log" | ||
) | ||
|
||
// EphemeralErrorHandler handles errors that are ephemeral in nature i.h these are errors | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean "i.e." instead of "i.h"? |
||
// that we would like to log as a warning unless they repeat for more than a certain duration of time. | ||
type EphemeralErrorHandler struct { | ||
Duration time.Duration | ||
ErrorString string | ||
FirstOccurrence *time.Time | ||
|
||
IgnoreDuration time.Duration | ||
IgnoredErrLogLevel func(string, ...interface{}) // Default IgnoredErrLogLevel is log.Debug | ||
} | ||
|
||
func NewEphemeralErrorHandler(duration time.Duration, errorString string, ignoreDuration time.Duration) *EphemeralErrorHandler { | ||
return &EphemeralErrorHandler{ | ||
Duration: duration, | ||
ErrorString: errorString, | ||
FirstOccurrence: &time.Time{}, | ||
IgnoreDuration: ignoreDuration, | ||
IgnoredErrLogLevel: log.Debug, | ||
} | ||
} | ||
|
||
// LogLevel method defaults to returning the input currentLogLevel if the given error doesnt contain the errorSubstring, | ||
// but if it does, then returns one of the corresponding loglevels as follows | ||
// - IgnoredErrLogLevel - if the error has been repeating for less than the IgnoreDuration of time. Defaults to log.Debug | ||
// - log.Warn - if the error has been repeating for less than the given duration of time | ||
// - log.Error - Otherwise | ||
// | ||
// # Usage Examples | ||
// | ||
// ephemeralErrorHandler.Loglevel(err, log.Error)("msg") | ||
// ephemeralErrorHandler.Loglevel(err, log.Error)("msg", "key1", val1, "key2", val2) | ||
// ephemeralErrorHandler.Loglevel(err, log.Error)("msg", "key1", val1) | ||
func (h *EphemeralErrorHandler) LogLevel(err error, currentLogLevel func(msg string, ctx ...interface{})) func(string, ...interface{}) { | ||
if h.ErrorString != "" && !strings.Contains(err.Error(), h.ErrorString) { | ||
h.Reset() | ||
return currentLogLevel | ||
} | ||
|
||
if *h.FirstOccurrence == (time.Time{}) { | ||
*h.FirstOccurrence = time.Now() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
if h.IgnoreDuration != 0 && time.Since(*h.FirstOccurrence) < h.IgnoreDuration { | ||
if h.IgnoredErrLogLevel != nil { | ||
return h.IgnoredErrLogLevel | ||
} | ||
return log.Debug | ||
} | ||
|
||
if time.Since(*h.FirstOccurrence) < h.Duration { | ||
return log.Warn | ||
} | ||
return log.Error | ||
} | ||
|
||
func (h *EphemeralErrorHandler) Reset() { | ||
*h.FirstOccurrence = time.Time{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
package log | ||
|
||
import ( | ||
"errors" | ||
"reflect" | ||
"testing" | ||
"time" | ||
|
||
"github.com/ethereum/go-ethereum/log" | ||
) | ||
|
||
func compareFunctions(f1, f2 func(msg string, ctx ...interface{})) bool { | ||
return reflect.ValueOf(f1).Pointer() == reflect.ValueOf(f2).Pointer() | ||
} | ||
func TestSimple(t *testing.T) { | ||
allErrHandler := NewEphemeralErrorHandler(2500*time.Millisecond, "", time.Second) | ||
err := errors.New("sample error") | ||
logLevel := allErrHandler.LogLevel(err, log.Error) | ||
if !compareFunctions(log.Debug, logLevel) { | ||
t.Fatalf("incorrect loglevel output. Want: Debug") | ||
} | ||
|
||
time.Sleep(1 * time.Second) | ||
logLevel = allErrHandler.LogLevel(err, log.Error) | ||
if !compareFunctions(log.Warn, logLevel) { | ||
t.Fatalf("incorrect loglevel output. Want: Warn") | ||
} | ||
|
||
time.Sleep(2 * time.Second) | ||
logLevel = allErrHandler.LogLevel(err, log.Error) | ||
if !compareFunctions(log.Error, logLevel) { | ||
t.Fatalf("incorrect loglevel output. Want: Error") | ||
} | ||
} | ||
|
||
func TestComplex(t *testing.T) { | ||
// Simulation: errorA happens continuously for 2 seconds and then errorB happens | ||
errorAHandler := NewEphemeralErrorHandler(time.Second, "errorA", 0) | ||
errorBHandler := NewEphemeralErrorHandler(1500*time.Millisecond, "errorB", 0) | ||
|
||
// Computes result of chaining two ephemeral error handlers for a given recurring error | ||
chainingErrHandlers := func(err error) func(string, ...interface{}) { | ||
logLevel := log.Error | ||
logLevel = errorAHandler.LogLevel(err, logLevel) | ||
logLevel = errorBHandler.LogLevel(err, logLevel) | ||
return logLevel | ||
} | ||
|
||
errA := errors.New("this is a sample errorA") | ||
if !compareFunctions(log.Warn, chainingErrHandlers(errA)) { | ||
t.Fatalf("incorrect loglevel output. Want: Warn") | ||
} | ||
time.Sleep(2 * time.Second) | ||
if !compareFunctions(log.Error, chainingErrHandlers(errA)) { | ||
t.Fatalf("incorrect loglevel output. Want: Error") | ||
} | ||
|
||
errB := errors.New("this is a sample errorB") | ||
if !compareFunctions(log.Warn, chainingErrHandlers(errB)) { | ||
t.Fatalf("incorrect loglevel output. Want: Warn") | ||
} | ||
if !compareFunctions(log.Warn, chainingErrHandlers(errA)) { | ||
t.Fatalf("incorrect loglevel output. Want: Warn") | ||
} | ||
|
||
errC := errors.New("random error") | ||
if !compareFunctions(log.Error, chainingErrHandlers(errC)) { | ||
t.Fatalf("incorrect loglevel output. Want: Error") | ||
} | ||
} |
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'm actively allergic to directories or packages called
util
https://www.jeromethibaud.com/en/blog/utils-considered-harmful/
Also, I very strongly try to avoid having to override golang package imports. Maybe the package could live in
logs/ephemeral
and the main type could beLogger
orHandler
then client code calls it likeephemeral.NewHandler()
?