Propagate loggers via contexts

[scrye]

This change is 

[lukedirtwalker]

Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @scrye)

---

go/lib/log/context.go, line 28 at r1 (raw file):

// logger. The logger can be recovered using GetLogger. Attaching a logger to a
// context which already contains one will overwrite the existing value.
func AttachLogger(ctx context.Context, logger Logger) context.Context {

Not sure about the naming. In infra we have infra.NewContextWithMessenger however this feels also a bit long.
Context API uses With for attaching.

I thought about (from a clients perspective):

		log.CtxWithLogger(ctx,logger)
		log.NewCtx(ctx,logger)
		log.CtxWith(ctx,logger)

and I think I prefer the last version.

---

go/lib/log/context.go, line 34 at r1 (raw file):

// GetLogger returns the logger embedded in ctx if one exists, or the root
// logger otherwise. GetLogger is guaranteed to never return nil.
func GetLogger(ctx context.Context) Logger {

Similarly to above I'm also not sure about the naming here. I don't like the Get prefix, saw enough of those in my Java days ;)
I thought about (from a clients perspective):

log.FromCtx(ctx)

---

go/lib/log/context_test.go, line 43 at r1 (raw file):

		ctx := log.AttachLogger(context.Background(), mockLogger)
		Convey("Writing to the logger from the context writes on the correct log", func() {
			logger := log.GetLogger(ctx)

I think we don't need the mocking etc. Wouldn't it be enough to create a new logger above and attach it to the context and here check if the logger is the same (points to the same object).?

---

go/path_srv/internal/handlers/common.go, line 152 at r1 (raw file):

		revInfos, verifiedSeg, verifiedRev, segErr, revErr)
	if len(insertedSegmentIDs) > 0 {
		logger.Debug("Segments inserted in DB", "count", len(insertedSegmentIDs),

Good Catch!

---

go/path_srv/internal/handlers/segreqnoncore_test.go, line 311 at r1 (raw file):

				msger := mock_infra.NewMockMessenger(ctrl)
				req := infra.NewRequest(
					log.AttachLogger(

This is probably not needed because GetLogger on the ctx will never return nil

[scrye]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)

---

go/lib/log/context.go, line 28 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Not sure about the naming. In infra we have infra.NewContextWithMessenger however this feels also a bit long.
Context API uses With for attaching.

I thought about (from a clients perspective):

		log.CtxWithLogger(ctx,logger)
		log.NewCtx(ctx,logger)
		log.CtxWith(ctx,logger)

and I think I prefer the last version.

Done.

---

go/lib/log/context.go, line 34 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Similarly to above I'm also not sure about the naming here. I don't like the Get prefix, saw enough of those in my Java days ;)
I thought about (from a clients perspective):

log.FromCtx(ctx)

Done.

---

go/lib/log/context_test.go, line 43 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think we don't need the mocking etc. Wouldn't it be enough to create a new logger above and attach it to the context and here check if the logger is the same (points to the same object).?

Done.

---

go/path_srv/internal/handlers/segreqnoncore_test.go, line 311 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This is probably not needed because GetLogger on the ctx will never return nil

Done.

[lukedirtwalker]

Reviewed 15 of 15 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[scrye]

Fixes #2096

[lukedirtwalker]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved