-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg/policy: Consistently use logrus fields #1834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, 1 suggestion inline.
pkg/policy/consumer.go
Outdated
@@ -259,25 +280,29 @@ func (c *Consumable) AllowConsumerLocked(cache *ConsumableCache, id NumericIdent | |||
// consumers map and the given consumable to the given consumer's consumers map. | |||
// Must be called with Consumable mutex Locked. | |||
func (c *Consumable) AllowConsumerAndReverseLocked(cache *ConsumableCache, id NumericIdentity) { | |||
log.Debugf("Allowing direction %d -> %d", id, c.ID) | |||
scopedLog := log.WithFields(log.Fields{ | |||
logfields.Identity + ".from": c.ID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original message specified opposite direction: id -> c.ID
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! so it did. Fixed.
pkg/policy/consumer.go
Outdated
c.AllowConsumerLocked(cache, id) | ||
|
||
if reverse := cache.Lookup(id); reverse != nil { | ||
log.Debugf("Allowing reverse direction %d -> %d", c.ID, id) | ||
scopedLog.Debug("Allowing reverse direction") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message will have the same from
and to
fields, but the direction is opposite, so I think they should be swapped here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I remember thinking "well, it says reverse, so I should show the un-reverse" but the old message had them in the other order. I'll fix it.
f57cc86
to
55c252c
Compare
@nebril A little uglier than before, but it's explicit. :) Please take another look. |
55c252c
to
6d92d41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a rebase
Signed-off-by: Ray Bejjani <ray@covalent.io>
f81d891
to
6cd05cc
Compare
@tgraf rebased |
…3-09-14 main-ce: Sync with upstream main branch 2023-09-14
This switches a bunch of log callsites to use logrus.WIthFields. These changes are not supposed to break anything and follow the same reasoning from #1801 The goal of these changes is to make debugging via logs easier by making our usage more consistent. Ideally, field names should be used the same way throughout the code and mean the same thing.
The most important things to review are whether the field names make sense in the context they're used, and whether we should introduce new ones to pkg/logfields/logfields.go. I'm also happy to incorporate better messages into this PR :)