Create loggers in controllers main function. by n3wscott · Pull Request #303 · knative/eventing

n3wscott · 2018-08-01T18:08:58Z

There is a small amount of config that needs to be in our repo to integrate with knative/pkg/logging. This is that config.

Also creating the loggers (but not passing them to the controller impls.)

n3wscott · 2018-08-01T18:17:27Z

/assign @mattmoor

…nting.

evankanderson · 2018-08-01T18:24:17Z

+// If configuration is empty, a fallback configuration is used.
+// If configuration cannot be used to instantiate a logger,
+// the same fallback configuration is used.
+func NewLogger(configJSON string, levelOverride string) (*zap.SugaredLogger, zap.AtomicLevel) {


Why wrap these? Just to avoid repeating components...?

evankanderson · 2018-08-01T18:26:07Z

+limitations under the License.
+*/
+
+package logkey


What about putting just this and the yaml in pkg/logconfig (and then callers would import knative/pkg/logging and knative/eventing/pkg/logconfig)?

you may hate the new change more... Lets see. I moved the relevant keys to apis.

evankanderson · 2018-08-01T18:27:44Z

/assign evankanderson
/unassign mattmoor

knative-metrics-robot · 2018-08-01T18:53:01Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/logging/logger.go	Do not exist	100.0%
pkg/logging/config.go	Do not exist	80.0%

n3wscott · 2018-08-01T20:07:14Z

Ready. PTAL @evankanderson

pmorie · 2018-08-01T23:22:28Z

/retest

pmorie · 2018-08-01T23:23:52Z


+	// Watch the logging config map and dynamically update logging levels.
+	configMapWatcher := configmap.NewDefaultWatcher(kubeClient, system.Namespace)
+	configMapWatcher.Watch(logconfig.ConfigName, logging.UpdateLevelFromConfigMap(logger, atomicLevel, logconfig.Controller, logconfig.Controller))


That is very cool and will be extremely useful :)

I'm glad you like it :)

pmorie

LGTM, one thing I noticed

pmorie · 2018-08-01T23:28:35Z

@@ -44,10 +56,46 @@ type ProvideFunc func(manager.Manager) (controller.Controller, error)

 func main() {
 	flag.Parse()


Not an issue w/ your PR, but we should consider renaming these binaries slightly; 'controller' runs the bus controllers and 'controller-manager' runs the flow, feed, etc controllers.

I'll create an issue for this.

I have some code that combines these into a single deployment, but haven't PR'd it: master...grantr:combine-controllers

evankanderson · 2018-08-01T21:46:13Z

+	if err != nil {
+		log.Fatalf("Error parsing logging configuration: %v", err)
+	}
+	logger, atomicLevel := logging.NewLoggerFromConfig(config, logconfig.ControllerManager)


where do we use atomicLevel?

configMapWatcher.Watch(logging.ConfigName, logging.UpdateLevelFromConfigMap(logger, atomicLevel, logLevelKey))

evankanderson · 2018-08-01T21:48:19Z

+
+	// Watch the logging config map and dynamically update logging levels.
+	configMapWatcher := configmap.NewDefaultWatcher(kubeClient, system.Namespace)
+	configMapWatcher.Watch(logconfig.ConfigName, logging.UpdateLevelFromConfigMap(logger, atomicLevel, logconfig.ControllerManager, logconfig.ControllerManager))


It would be nice to be able to encapsulate the 35 new lines in pkg/logging, but I think that's a follow-on PR.

I agree. Had the same thought as I was adding this.

evankanderson · 2018-08-01T21:49:19Z

+
 	// Setup a Manager
-	mrg, err := manager.New(config.GetConfigOrDie(), manager.Options{})
+	mrg, err := manager.New(clientconfig.GetConfigOrDie(), manager.Options{})


I presume this rename is to distinguish from logconfig?

yes but it could go back now...

evankanderson · 2018-08-01T21:50:53Z

 	defer cancel()
 	srv.Shutdown(ctx)
-
-	glog.Flush()


Hah, I think you caught a bug here. Nice!

evankanderson · 2018-08-01T21:52:11Z

+
+	logger.Info("Starting the Controller Manager")
+
 	logf.SetLogger(logf.ZapLogger(false))


Add a comment that this doesn't do anything yet.

@grantr for details, this does nothing yet right?

This tells controller-runtime to use zap to log internal messages. We could eventually use the https://github.com/go-logr/logr abstraction for controller logging also, but there's no need for it right now.

evankanderson · 2018-08-01T23:26:05Z

+        volumeMounts:
+          - name: config-logging
+            mountPath: /etc/config-logging
+      volumes:


It looks like we're both mounting a ConfigMap, and using the k8s watcher on the ConfigMap object (because you're feeding in a k8s client). We should just be able to use inotify on the file:

https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#mounted-configmaps-are-updated-automatically

https://github.com/fsnotify/fsnotify looks promising.

The autoscaling group found that inotify on configmaps is very slow, normally taking 30+ seconds to propagate updates to volumes. I think we can probably drop the volume mount now that we have the configmap watch.

do tell more !

I vote make this an issue and do a followup pass. Having loggers is more valuable than optimizing i think

Yes, this was intended to be a followup, not a blocker.

n3wscott · 2018-08-02T17:37:12Z

/test pull-knative-eventing-integration-tests

n3wscott · 2018-08-02T19:13:47Z

Ready to go in @evankanderson PTAL?

evankanderson

/lgtm
/approve

One minor nit, but it doesn't block getting this in.

evankanderson · 2018-08-02T22:02:23Z

 		log.Fatalf("Error parsing logging configuration: %v", err)
 	}
-	logger, atomicLevel := logging.NewLoggerFromConfig(config, logconfig.ControllerManager)
+	logger, atomicLevel := logging.NewLoggerFromConfig(cfg, logconfig.ControllerManager)


Do we use atomicLevel anywhere? If not, should we name it "_"?

it is used in the watch call

google-prow-robot · 2018-08-02T22:03:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, n3wscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

grantr · 2018-08-02T23:27:34Z

/retest

* Starting to add webhook. * Adding logging config for eventing to be used with knative/pkg/logging. * Revert webhook change. * Revert webhook change. * Add a test to make sure happy path is happy for getting logger in eventing. * Adding config-logging to the main config. * Remove owners file. * Moving to just use named loggers in the controllers. * Use logging config for controller name. * Add newlines to the yaml. * Adding logging creation and config reading to the controllers now. * Don't dep on glog in cmd. * Cleanup based on review.

Scott Nichols added 2 commits August 1, 2018 11:06

Starting to add webhook.

06438e2

Adding logging config for eventing to be used with knative/pkg/logging.

9905fe6

google-prow-robot requested review from evankanderson and mattmoor August 1, 2018 18:09

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 1, 2018

Scott Nichols added 2 commits August 1, 2018 11:14

Revert webhook change.

d5f789e

Revert webhook change.

6b093cf

google-prow-robot assigned mattmoor Aug 1, 2018

Add a test to make sure happy path is happy for getting logger in eve…

1d4fbab

…nting.

evankanderson reviewed Aug 1, 2018

View reviewed changes

google-prow-robot assigned evankanderson and unassigned mattmoor Aug 1, 2018

Scott Nichols added 2 commits August 1, 2018 11:47

Adding config-logging to the main config.

11fbe8a

Remove owners file.

bdd8375

Scott Nichols added 4 commits August 1, 2018 12:39

Moving to just use named loggers in the controllers.

46bf0ea

Use logging config for controller name.

2743412

Add newlines to the yaml.

9d68d86

Adding logging creation and config reading to the controllers now.

09db04a

n3wscott changed the title ~~Add logging config~~ Create loggers in controllers. Aug 1, 2018

n3wscott changed the title ~~Create loggers in controllers.~~ Create loggers in controllers main function. Aug 1, 2018

Don't dep on glog in cmd.

5ecf818

pmorie reviewed Aug 1, 2018

View reviewed changes

evankanderson reviewed Aug 1, 2018

View reviewed changes

Cleanup based on review.

2336606

n3wscott mentioned this pull request Aug 2, 2018

Unbreak integration tests #299

Closed

evankanderson reviewed Aug 2, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2018

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2018

google-prow-robot merged commit adc94e8 into knative:master Aug 2, 2018

grantr mentioned this pull request Aug 3, 2018

Reconcile with context #315

Merged

		@@ -44,10 +56,46 @@ type ProvideFunc func(manager.Manager) (controller.Controller, error)

		func main() {
		flag.Parse()


		logger.Info("Starting the Controller Manager")

		logf.SetLogger(logf.ZapLogger(false))

Conversation

n3wscott commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n3wscott commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Aug 1, 2018

Uh oh!

knative-metrics-robot commented Aug 1, 2018

Uh oh!

n3wscott commented Aug 1, 2018

Uh oh!

pmorie commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmorie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Aug 2, 2018

Uh oh!

n3wscott commented Aug 2, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-prow-robot commented Aug 2, 2018

Uh oh!

n3wscott commented Aug 1, 2018 •

edited

Loading