Add EC / BN client logging and better request timeouts #13
Conversation
…factored the fallback recheck system
beacon/client/http-provider.go
Outdated
| fastGetMethod string = "Fast GET" | ||
| slowGetMethod string = "Slow GET" | ||
| postMethod string = "POST" |
There was a problem hiding this comment.
instead of postMethod there's http.MethodPost.
Instead of multiple clients, use https://pkg.go.dev/net/http#NewRequestWithContext which gives you control over the timeout via a context with a timeout
Pass the resulting Request to client.Do and pick a large timeout for client.Timeout (at least the max of the timeouts specified in context.WithTimeout)
Better yet, use the provided context.Context in each of the getters unless nil, and default to an appropriate-lengthed timeout. It would be an inflexible abstraction if it prescribed a two-tier timeout system instead of just using context.Context
beacon/client/http-provider.go
Outdated
|
|
||
| logger.Debug("Calling BN request", | ||
| slog.String(log.MethodKey, methodName), | ||
| slog.String(log.PathKey, path), |
There was a problem hiding this comment.
would be nice to log the host, too...
There was a problem hiding this comment.
The host is included in the path; it's the whole URL minus the query because queries can get unwieldy for certain calls.
There was a problem hiding this comment.
well i leave it up to you but i would probably parse it using net/url so that we can leverage structured logging fully
beacon/client/http-provider.go
Outdated
| return nil, 0, fmt.Errorf("error running GET request to [%s]: %w", trimmedPath, err) | ||
| } | ||
| return response.Body, response.StatusCode, nil | ||
| } | ||
|
|
||
| // Log a request and add HTTP tracing to the context if the logger has it enabled | ||
| func logRequest(ctx context.Context, methodName string, path string) context.Context { |
There was a problem hiding this comment.
seems like this should be a receiver on BeaconHttpProvider so it can log info specific to the instance
beacon/client/std-http-client.go
Outdated
| // Create a new client instance. | ||
| // Most calls will use the fast timeout, but queries to validator status will use the slow timeout since they can be very large. | ||
| // Set a timeout of 0 to disable it. | ||
| func NewStandardHttpClient(providerAddress string, fastTimeout time.Duration, slowTimeout time.Duration) *StandardHttpClient { |
There was a problem hiding this comment.
I'd add a type StandardHttpClientOpts struct with two time.Duration fields. if non-0 they can be used as the defaults to context.WithTImeout. If 0, default to something sensible.
NewStandardHttpClient("http://blah", nil) would just use all defaults.
config/external-beacon-config.go
Outdated
| FastTimeout Parameter[uint64] | ||
|
|
||
| // Number of seconds to wait for a slow request to complete | ||
| SlowTimeout Parameter[uint64] |
There was a problem hiding this comment.
Can we add units to these field names? ie FastTimeoutMS
Seconds seems too rough of a granularity
| @@ -174,6 +177,18 @@ func NewLoggerConfig() *LoggerConfig { | |||
| Network_All: true, | |||
| }, | |||
| }, | |||
|
|
|||
| EnableHttpTracing: Parameter[bool]{ | |||
eth/std-rpc-client.go
Outdated
There was a problem hiding this comment.
Going to wave my hand and say "see above commends on using context.Context for this file too"
log/logger.go
Outdated
| @@ -71,6 +78,11 @@ func (l *Logger) GetFilePath() string { | |||
| return l.path | |||
| } | |||
|
|
|||
| // Get the HTTP client tracer for this logger if HTTP tracing was enabled | |||
| func (l *Logger) GetHttpTracer() *httptrace.ClientTrace { | |||
| return l.tracer | |||
There was a problem hiding this comment.
might as well just expose this field (i'd rename to HttpTracer) and conditionally populate it in NewLogger when options.EnableHttpTracing is true
| @@ -73,10 +79,49 @@ func (m *BeaconClientManager) GetClientTypeName() string { | |||
|
|
|||
| func (m *BeaconClientManager) SetPrimaryReady(ready bool) { | |||
| m.primaryReady = ready | |||
| if ready { | |||
| m.primaryFailTime = time.Time{} | |||
There was a problem hiding this comment.
instead of primaryFailTime and fallbackFailTime as fields on BeaconClientManager you could use a unexported struct which has a boolean field to indicate failure (rather than relying on 0 value to mean success)
| } | ||
| } | ||
|
|
||
| func (m *BeaconClientManager) RecheckFailTimes(logger *log.Logger) { |
There was a problem hiding this comment.
Might be better to name this function something that contains the word 'throttle', since it throttles reconnection attempts to recheckTime intervals. Might even be sensible to return a boolean to indicate whether or not a reconnection was attempted.
| } | ||
|
|
||
| logger.Debug("Calling BN request", | ||
| slog.String(log.MethodKey, methodName), |
There was a problem hiding this comment.
seems way more practical to log ctx.Deadline() btw
|
Alright I addressed most of the stuff in here, lower priority stuff like refactoring / renaming the reconnect delay is on hold because it's the weekend. |
This PR adds supplemental logging support to the standard EC and BN API bindings, including HTTP tracing if requested. It also refactors the primary / fallback timeout system used by the client managers to make it easier to understand what's going on with the reconnect checks. Finally it adds config parameters for all of the above so users can tweak the new behavior if desired.
This comes from feedback provided by some HD operators while running on Mainnet. They'd like more logging granularity when specifically running requests against the standard EC and BN bindings, so they can diagnose why the daemon(s) can't contact the clients when tools like
cURLcan.