allow basic logging for http Get err with RegisterErrLogFn #81
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
groupcache.go
Outdated
| @@ -121,6 +122,20 @@ func RegisterNewGroupHook(fn func(*Group)) { | |||
| newGroupHook = fn | |||
| } | |||
|
|
|||
| // RegisterErrLogFn registers a log func that is used for http error logging | |||
There was a problem hiding this comment.
The "Fn" suffix isn't consistent with the naming elsewhere in Go in general or this file in particular. This file uses "Hook".
Also, end sentences with a period. And http should be HTTP.
groupcache.go
Outdated
| // log of the past few for /groupcachez? It's | ||
| // probably boring (normal task movement), so not | ||
| // worth logging I imagine. | ||
| logErr(err) |
There was a problem hiding this comment.
What about other context? Do we want to include the key?
There was a problem hiding this comment.
Good point! Just wondering how specific errLogFn func(error) should be ?
logErr(fmt.Errorf("g.getFromPeer(ctx, peer, key), ctx: %+v, peer: %+v, key: %+v, err: %+v", ctx, peer, key, err))
or
logErr(ctx, peer, key, err)
rename to follow hook function name convention, log more ctx info
|
updated:) |
|
sorry.. but was there a new review? #81 (review) didn't show anything |
|
@bradfitz any other parts I could improve on this diff ? |
…ontext info (ctx, peer, key, err) rename to follow hook function name convention, log more ctx info
attempt to improve this :
func (g *Group) load(ctx Context, key string, dest Sink) (value ByteView, destPopulated bool, err error) {
// TODO(bradfitz): log the peer's error? keep
// log of the past few for /groupcachez? It's
// probably boring (normal task movement), so not
// worth logging I imagine.