Skip to content
This repository has been archived by the owner on Oct 13, 2021. It is now read-only.

Improve debug logging #114

Merged
merged 14 commits into from
May 1, 2019
Merged

Improve debug logging #114

merged 14 commits into from
May 1, 2019

Conversation

JoelSpeed
Copy link
Contributor

@JoelSpeed JoelSpeed commented Mar 29, 2019

This PR intends to improve debug logging throughout the Faros codebase.

It introduces the logr framework and klog logging to allow more structured and levelled logging throughout the codebase.

I've also changed the logging so that it should log by default to stdout and only log to stderr for actual errors or if --logtostderr=true is passed CC @sebastianroesch

I've built an image which I'm going to test internally quay.io/pusher/faros:pull-114

This is pretty much exclusively conversion at the moment and doesn't actually add that much in the way of logging, but I think we need to work out what extra logs we want

Fixes #122 (?)

@JoelSpeed
Copy link
Contributor Author

/build docker

3 similar comments
@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

/build docker

@pusher-ci pusher-ci added size/L and removed size/M labels Apr 1, 2019
@JoelSpeed
Copy link
Contributor Author

/retest

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

This is kind of annoying that it is at the V(1) level as V(2) (IMO) should be a reasonable default, but this makes things rather noisy

https://github.com/kubernetes-sigs/controller-runtime/blob/856708d78b479900b9d17f98f7f59a2f074fbc35/pkg/internal/controller/controller.go#L231

Copy link
Contributor

@mthssdrbrg mthssdrbrg left a comment

Choose a reason for hiding this comment

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

Yay, logging! Looks reasonable at first glance. We should decide on a style for the fields, as they're now a mix of lowercase (maybe snake_case?) and CamelCase. Personally I prefer snake_case.

cmd/manager/main.go Show resolved Hide resolved
pkg/controller/gittrack/gittrack_controller.go Outdated Show resolved Hide resolved
pkg/controller/gittrackobject/watch.go Outdated Show resolved Hide resolved
@sebastianrosch
Copy link
Contributor

Awesome! I tested the build and it definitely fixes #122. I noticed that several logs appear multiple times, even though they should be logged only once, like:

I0424 09:41:24.325716       1 main.go:99] manager "level"=0 "msg"="Registering Components."
I0424 09:41:24.325716       1 main.go:99] manager "level"=0 "msg"="Registering Components."

I also noticed that --v=0 logs almost nothing while --v=1 is already spamming quite a bit. I would suggest we have one level that is similar to the current behavior and logs errors and normal operation, like checkout, without spamming. What do you think?

@JoelSpeed
Copy link
Contributor Author

I've also noticed the spamming, though it is mostly out of our control (being inside controller-runtime).

I've been following the community guidelines on levels where they just have a 0 and a 1 level, which is kind of annoying, short of forking or proposing they follow the same guidelines I haven't worked out how we can be less spammy right now

@pusher-ci pusher-ci added size/XL and removed size/L labels Apr 24, 2019
@JoelSpeed
Copy link
Contributor Author

@mthssdrbrg @sebastianroesch I've just added b047eac which offsets all controller-runtime logs by 3 levels, so the logs that were v=0 from CR become v=3 and the logs that were v=1 become v=4, this aligns their debug logging with our debug logging so should make reconciles quieter and gives us better control over what is printed at each of the lower levels

Not sure I 100% like this though so let me know what you think

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

/build docker

@wonderhoss
Copy link
Contributor

I really like the new structure in general, thanks @JoelSpeed for all the yak shaving involved.

One observation:
I feel that wherever we have a log line akin to Starting controllers..., we should also always have a corresponding like at the same severity that says All controllers started.
So, if we log the beginning of an activity, we also log when the activity is completed so that it becomes easier to reason about the execution flow just from the logs.

@JoelSpeed
Copy link
Contributor Author

/build docker

@JoelSpeed
Copy link
Contributor Author

@sebastianroesch FYI I've fixed the duplicate logging issue in my last push

I feel that wherever we have a log line akin to Starting controllers..., we should also always have a corresponding like at the same severity that says All controllers started.

@gargath in general I agree with you and for the most part in Faros we have that, however, that particular example that you have I can do very little about unfortunately. We post this message and then call mgr.Start(...) which blocks until a SIGTERM, so there's no easy was I can add extra logging there. Though that said, controller-runtime does include a bunch of logs about starting the controllers and would show that we have finished starting up

@JoelSpeed
Copy link
Contributor Author

/retest

pkg/controller/gittrackobject/handler.go Outdated Show resolved Hide resolved
pkg/controller/gittrackobject/handler.go Outdated Show resolved Hide resolved
pkg/controller/gittrackobject/handler_test.go Show resolved Hide resolved
pkg/controller/gittrackobject/status.go Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Contributor Author

@mthssdrbrg Good spot on those inconsistencies, have fixed

@JoelSpeed JoelSpeed merged commit 5bfef6b into master May 1, 2019
@pusher-ci pusher-ci deleted the debug-logs branch May 1, 2019 11:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Info logs logged to stderr
5 participants