✨ Add ability to set/get logger in/from a context.Context #1093
Conversation
Signed-off-by: Vince Prignano <[email protected]>
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| "github.com/go-logr/logr" | ||
| ) | ||
|
|
||
| var ( | ||
| contextKey = &struct{}{} |
There was a problem hiding this comment.
err...Is this common, can we just claim a key like this?
There was a problem hiding this comment.
Yes, this is the approach pointed out in #1054 (comment)
It's actually a pretty nice approach for this use case, it won't use a string which might cause issues and the pointer is used to determine equality, which users can't overwrite
There was a problem hiding this comment.
Do we need the equality? I have to admit I am not an expert on how a context is supposed to be used but since it ends up being shared among code owned by different parties I intuitively thought it would make sense to somehow scope context keys to the project (e.G. via a string prefix) to avoid collisions that will result in unexpected behavior
There was a problem hiding this comment.
We can always change it later, unless we don't want to merge this for some reason, I do like that for now this is private and not allowed to be overwritten, I'd rather have this go in given that it has been proven in another project to work well and iterate on it as we go
There was a problem hiding this comment.
From that comment thread, there are at least 2 different active projects linked that use the same approach (cert-manager and go-grpc-middleware), so +1 to this approach
There was a problem hiding this comment.
I actually didn't realize the context key is the pointer address and was concerned this might conflict with someone else also using an empty struct as identifier.
/lgtm
There was a problem hiding this comment.
I think this breaks when you have multiple of such context keys. They are likely to be identical as they can have the same address: https://play.golang.org/p/4uqlsUv5Xuf
The Go ref (https://golang.org/ref/spec#Size_and_alignment_guarantees) says:
Two distinct zero-size variables may have the same address in memory.
|
lgtm, but will defer to @alvaroaleman for tagging based on the discussion thread. |
k8s-ci-robot
added
the
lgtm
|
/retest |
Signed-off-by: Vince Prignano [email protected]
/assign @alvaroaleman @detiber
This we might be able to merge now, given that's not a breaking change