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.

Sign up for GitHub

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

Jump to bottom

✨ logging: allow override default logger for compatibility #1971

Closed
wants to merge 2 commits into from
Closed

✨ logging: allow override default logger for compatibility #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99bf5b1
Allow override default logger for compat
timonwong Aug 8, 2022
135d3dc
Merge branch 'kubernetes-sigs:master' into allow-override-default-log
timonwong Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion pkg/builder/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ func (blder *Builder) WithLogConstructor(logConstructor func(*reconcile.Request)
return blder
}

// WithLogger overrides the controller options's GetDefaultLogger.
func (blder *Builder) WithLogger(log logr.Logger) *Builder {
blder.ctrlOptions.GetDefaultLogger = func() logr.Logger {
return log
}
return blder
}

// Named sets the name of the controller to the given name. The name shows up
// in metrics, among other things, and thus should be a prometheus compatible name
// (underscores and alphanumeric characters only).
Expand Down Expand Up @@ -309,7 +317,14 @@ func (blder *Builder) doController(r reconcile.Reconciler) error {

// Setup the logger.
if ctrlOptions.LogConstructor == nil {
log := blder.mgr.GetLogger().WithValues(
var log logr.Logger
if ctrlOptions.GetDefaultLogger != nil {
log = ctrlOptions.GetDefaultLogger()
} else {
log = blder.mgr.GetLogger()
}

Copy link
Member

@camilamacedo86 camilamacedo86 Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IHMO we should have only one log approach and ideally follow the k8s standards.
Why?

  • That helps those that are looking for Oberservaility, many tools will try to check the logs centralized. So that we can ensure that any project built with controller-runtime will follow up the same approach/standard
  • Maintainability. We do not increase the burn to have more implementation for logs
  • Encourage standards and good practices

In this way, I'd like to raise a question: What is the k8s format? Is k8s doing the logs using the format of WithLogConstructor or of the removed WithLogger?

/hold

Copy link
Author

@timonwong timonwong Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

k8s uses klog, which is derived from good-old glog.
In that case, I think it's better to avoid the use of logr, which backend is configurable (zap, logrus, zerolog, go-kit, klog, etc... leads users to choose their fav logger library), we can stick to klog instead.

Copy link
Member

@camilamacedo86 camilamacedo86 Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @timonwong,

I think that what matters, in this case, is more the output/format of the logs. According to the description of #1827 the goal was: align to Kubernetes structured logging, add reconcileID

However, I think would be nice if we could check how the format is outputted by k8s as the dep (which you already checked) and if we can or do not follow the same standards. if not, why not? Also, have we any Kubernetes good practices definition about it? If so, I think we should also follow up on that. By last, I think also would be nice to understand why we are using Logger and not klog. What was the motivation for this adoption instead of klog?

(IMHO) the above assessment needs to be done before we propose changes. If we figure out that we need to change then, maybe open an issue describing the proposal and motivations for that can help out in the discussion. Also, a PR proposing the changes based on the motivations can be helpful for sure too.

WDYT?

Copy link
Member

@alvaroaleman alvaroaleman Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@camilamacedo86 I think a lot of what you are asking here is orthogonal to this change. This changes goal is to have the same fields that get set when a logger comes from the mgr when a custom logger is used.

I agree that it is not great to have two ways to set a custom logger that are different in a very subtle way.

log = log.WithValues(
"controller", controllerName,
"controllerGroup", gvk.Group,
"controllerKind", gvk.Kind,
Expand Down
22 changes: 22 additions & 0 deletions pkg/builder/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,28 @@ var _ = Describe("application", func() {
Expect(instance).NotTo(BeNil())
})

It("should honor default log options", func() {
logger := &testLogger{}
newController = func(name string, mgr manager.Manager, options controller.Options) (controller.Controller, error) {
if options.GetDefaultLogger != nil && options.GetDefaultLogger().GetSink() == logger {
return controller.New(name, mgr, options)
}
return nil, fmt.Errorf("logger expected %T but found %T", logger, options.LogConstructor)
}

By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

instance, err := ControllerManagedBy(m).
For(&appsv1.ReplicaSet{}).
Owns(&appsv1.ReplicaSet{}).
WithLogger(logr.New(logger)).
Build(noop)
Expect(err).NotTo(HaveOccurred())
Expect(instance).NotTo(BeNil())
})

It("should prefer reconciler from options during creation of controller", func() {
newController = func(name string, mgr manager.Manager, options controller.Options) (controller.Controller, error) {
if options.Reconciler != (typedNoop{}) {
Expand Down
13 changes: 12 additions & 1 deletion pkg/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ type Options struct {
// to each reconciliation via the context field.
LogConstructor func(request *reconcile.Request) logr.Logger

// GetDefaultLogger returns this controller's root logger, used only if LogConstructor is not set.
// By defaults, manager.GetDefaultLogger will be used.
GetDefaultLogger func() logr.Logger

// CacheSyncTimeout refers to the time limit set to wait for syncing caches.
// Defaults to 2 minutes if not set.
CacheSyncTimeout time.Duration
Expand Down Expand Up @@ -107,7 +111,14 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
}

if options.LogConstructor == nil {
log := mgr.GetLogger().WithValues(
var log logr.Logger
if options.GetDefaultLogger != nil {
log = options.GetDefaultLogger()
} else {
log = mgr.GetLogger()
}

log = log.WithValues(
"controller", name,
)
options.LogConstructor = func(req *reconcile.Request) logr.Logger {
Expand Down