CBG-3379 add missing context logging #6408
Conversation
- add contexts through functions - does not include any places needed to make modifications to sg-bucket (will come in a separate PR) Performed a search on context.Background() and context.TODO() and followed them through. There are a few cases I didn't change: - generally things should come from context.Background() in the start of main, but there were some cases we use logging in init - there are a few cases we intentionally reset context during replication and ISGR, but these will need to be tagged with a database
| ctx := context.TODO() // fix in sg-bucket | ||
| WarnfCtx(ctx, "Unable to obtain gocbcore.Agent while retrieving expiry:%v", err) |
There was a problem hiding this comment.
Arguably this should be a wrapped error rather than a log but agree with TODO for now to coordinate other sg-bucket API changes.
| func (c *changeCache) Remove(ctx context.Context, collectionID uint32, docIDs []string, startTime time.Time) (count int) { | ||
| return c.channelCache.Remove(ctx, collectionID, docIDs, startTime) |
There was a problem hiding this comment.
Is there a pattern/rule to follow for usage of parameter context vs. c.logCtx? Usage-dependent?
Having context defined only in some methods makes me wonder how we're going to maintain this with any consistency going forwards.
There was a problem hiding this comment.
I largely didn't change the existing code - but I think writing new code we should try to pass a context in. There are some places this is impractical:
- DCP related code (especially but not limited to cbgt) with specific callbacks we have to match interfaces with
I was thinking in main branch we have the opportunity to clean this up but not for log streaming.
| // Clear reinitializes the cache to an empty state | ||
| Clear() | ||
|
|
||
| // Size of the the largest individual channel cache, invoked for stats reporting | ||
| // // TODO: let the cache manage its own stats internally (maybe take an updateStats call) | ||
| MaxCacheSize() int | ||
| MaxCacheSize(context.Context) int |
There was a problem hiding this comment.
Shouldn't we make context consistent across the full interface? Even if the current implementation doesn't actually use context in all methods?
| @@ -242,7 +242,7 @@ func (bh *blipHandler) handleSetCheckpoint(rq *blip.Message) error { | |||
| func (bh *blipHandler) handleSubChanges(rq *blip.Message) error { | |||
| defaultSince := CreateZeroSinceValue() | |||
| latestSeq := func() (SequenceID, error) { | |||
| seq, err := bh.collection.LastSequence() | |||
| seq, err := bh.collection.LastSequence(bh.loggingCtx) | |||
There was a problem hiding this comment.
As mentioned via Slack - What looks like a 3.1.0 regression - bh.loggingCtx isn't being set properly for default collection replications, resulting in us losing context ID for a lot of logging called via BLIP.
use context passed in where possible in change log
Co-authored-by: Ben Brooks <[email protected]>
Co-authored-by: Ben Brooks <[email protected]>
Co-authored-by: Ben Brooks <[email protected]>
Co-authored-by: Ben Brooks <[email protected]>
Performed a search on context.Background() and context.TODO() and followed them through.
There are a few cases I didn't change:
I think the way to review this is to see where I've changed
context.Background()to something that is passed in. Sometimes this could be a problemPre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2002