Logging improvements by mdemirhan · Pull Request #1620 · knative/serving

mdemirhan · 2018-07-18T19:59:04Z

Change log levels dynamically for activator, webhook and autoscaler
Remove the use of runtime.Handle and instead use logger.Errorf since that is pretty much all runtime.Handle does today.
Disable glog in activator and autoscaler to prevent k8s logs appearing in these services. We already log errors and extra & unstructured k8s logs are not helpful.
Simplify the monitoring installation by getting rid of dev profile. Instead, we now by default watch for logs in knative components, but the logging level is set to fatal. To enable Knative logging, we just need to edit config-logging configmap and change those values to info. The updates will be picked up immediately without the need to restart any components.

* Remove the use of runtime.Handle and instead use logger.Errorf since that is pretty much all runtime.Handle does today. * Disable glog in activator and autoscaler to prevent k8s logs appearing in these services. We already log errors and extra & unstructured k8s logs are not helpful. * Simplify the monitoring installation by getting rid of dev profile. Instead, we now by default watch for logs in knative components, but the logging level is set to fatal. To enable Knative logging, we just need to edit config-logging configmap and change those values to info. The updates will be picked up immediately without the need to restart any components.

mdemirhan · 2018-07-18T19:59:45Z

Holding it until the branch is open for checkins again.

yanweiguo · 2018-07-19T16:52:32Z

 ```shell
 ko apply -f config/
+
+# Set the logging threhold to info for all Knative components and reapply the config


typo: threshold

yanweiguo · 2018-07-19T17:09:08Z

      @id fluentd-containers.log
      @type tail
-      path /var/log/containers/*user-container-*.log,/var/log/containers/*build-step-*.log,/var/log/containers/*controller-*.log,/var/log/containers/*webhook-*.log,/var/log/containers/*autoscaler-*.log,/var/log/containers/*queue-proxy-*.log,/var/log/containers/*activator-*.log
+      path /var/log/containers/*user-container-*.log,/var/log/containers/*build-step-*.log,/var/log/containers/controller-*controller-*.log,/var/log/containers/webhook-*webhook-*.log,/var/log/containers/*autoscaler-*autoscaler-*.log,/var/log/containers/*queue-proxy-*.log,/var/log/containers/activator-*activator-*.log


Extra controller-, webhook-, autoscaler-, activator-? The file names don't look right to me.

The changes are intentional. With the changes, we will collect logs from only specific pods - for example, we won't collect logs from all containers named controller, but only containers in a pod that starts with controller.

Could you also do this change to the configmap for stackdriver? They are not consistent currently.

yanweiguo · 2018-07-19T17:14:13Z


-1. First, go through [OpenCensus Go Documentation](https://godoc.org/go.opencensus.io).
-2. Add the following to your application startup:
+1.First, go through [OpenCensus Go Documentation](https://godoc.org/go.opencensus.io).


Will markdown regard this as ordered list without space after the number?

Good catch. Fixed it.

…loggers.

mdemirhan · 2018-07-20T21:24:20Z

/hold cancel

mattmoor

I like the reduction in code / config :)

mattmoor · 2018-07-21T15:24:47Z

 	mux.HandleFunc("/", handler)
 	mux.Handle("/metrics", exporter)
 	http.ListenAndServe(":"+servingAutoscalerPort, mux)
+	close(stopCh)


use defer close(stopCh) right after the make?

mattmoor · 2018-07-21T15:26:45Z

+  loglevel.autoscaler: "fatal"
+  loglevel.queueproxy: "fatal"
+  loglevel.webhook: "fatal"
+  loglevel.activator: "fatal"


This seems like a high default, are we sure we don't want error to start?

I think before we change this default (at all) we should make it really clear this is changing in slack, or folks are going to get confused

Will change this back to info level.

# Conflicts: # docs/telemetry.md # hack/release.sh

mdemirhan · 2018-07-23T19:56:11Z

/hold
Holding to test the latest merge.

knative-metrics-robot · 2018-07-23T19:59:09Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	96.6%	96.5%	-0.0
pkg/logging/config.go	95.6%	96.3%	0.7

mdemirhan · 2018-07-23T20:02:11Z

/retest

mdemirhan · 2018-07-23T20:22:57Z

/hold cancel

mdemirhan · 2018-07-23T20:23:55Z

@mattmoor this is ready for another look. I changed the default level to info.

mattmoor

/lgtm
/approve

google-prow-robot · 2018-07-23T20:44:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, mdemirhan

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~~ [mattmoor]

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

AceHack · 2020-09-22T02:19:59Z

What are the possible log levels?

mdemirhan requested review from mattmoor and yanweiguo July 18, 2018 19:59

google-prow-robot requested review from grantr, josephburnett, srinivashegde86 and tcnghia July 18, 2018 19:59

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

mdemirhan added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2018

Unit test fixes.

d1194e1

mdemirhan assigned mattmoor Jul 18, 2018

yanweiguo reviewed Jul 19, 2018

View reviewed changes

mdemirhan added 3 commits July 20, 2018 11:19

Spelling fixes.

3123684

Add controller type information to activator, autoscaler and webhook …

a5ba247

…loggers.

Fixes.

e1a899c

google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2018

mattmoor reviewed Jul 21, 2018

View reviewed changes

Mustafa Demirhan added 2 commits July 23, 2018 12:51

Change the default level to info

140c3ad

Merge branch 'master-upstream' into loggingimprovements

ed52f81

# Conflicts: # docs/telemetry.md # hack/release.sh

google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2018

google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2018

mattmoor reviewed Jul 23, 2018

View reviewed changes

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

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

google-prow-robot merged commit 5441a18 into knative:master Jul 23, 2018

mdemirhan deleted the loggingimprovements branch July 23, 2018 22:43

tcnghia mentioned this pull request Oct 19, 2018

Add mdemirhan to /OWNERS #2264

Merged

Conversation

mdemirhan commented Jul 18, 2018

Uh oh!

mdemirhan commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanweiguo Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mdemirhan commented Jul 20, 2018

Uh oh!

mattmoor 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!

mdemirhan commented Jul 23, 2018

Uh oh!

knative-metrics-robot commented Jul 23, 2018

Uh oh!

mdemirhan commented Jul 23, 2018

Uh oh!

mdemirhan commented Jul 23, 2018

Uh oh!

mdemirhan commented Jul 23, 2018

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

google-prow-robot commented Jul 23, 2018

Uh oh!

AceHack commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yanweiguo Jul 19, 2018 •

edited

Loading