Skip to content
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

Bpf logrus.WithFields fixups #1827

Merged
merged 6 commits into from
Oct 26, 2017
Merged

Bpf logrus.WithFields fixups #1827

merged 6 commits into from
Oct 26, 2017

Conversation

raybejjani
Copy link
Contributor

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 :)

@raybejjani raybejjani requested a review from a team October 24, 2017 11:16
@raybejjani raybejjani added the kind/enhancement This would improve or streamline existing functionality. label Oct 24, 2017
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, but LGTM. My guess is that the CI failure is a flake and unrelated, so you likely just need to rebase this.

@@ -216,7 +223,9 @@ func UpdateRevNat(key RevNatKey, value RevNatValue) error {
}

func DeleteRevNat(key RevNatKey) error {
log.Debugf("deleting RevNatKey: %s", key.String())
log.WithFields(log.Fields{
"revNATK": key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateRevNat seems to use logfields.BPFMapKey, any reason not to use it here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! I missed it when I decided have a global BPFMapKey. Fixed

Signed-off-by: Ray Bejjani <ray@covalent.io>
Signed-off-by: Ray Bejjani <ray@covalent.io>
Signed-off-by: Ray Bejjani <ray@covalent.io>
Signed-off-by: Ray Bejjani <ray@covalent.io>
Signed-off-by: Ray Bejjani <ray@covalent.io>
@tgraf tgraf merged commit a235458 into master Oct 26, 2017
@tgraf tgraf deleted the bpf-log-with-fields branch October 26, 2017 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement This would improve or streamline existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants