Configurable Log Level for Agones Controller #1220

aLekSer · 2019-12-09T12:44:59Z

Add log level parameter to Helm.

Part of #1218.

agones-bot · 2019-12-09T12:50:19Z

Build Failed 😱

Build Id: d8e619de-e315-48a4-b17b-f98a2883dcd4

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2019-12-09T12:51:09Z

Build Failed 😱

Build Id: 99534370-57d0-400b-9051-ff1ed55707b4

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel · 2019-12-09T22:12:47Z

cmd/controller/main.go

@@ -103,6 +104,16 @@ func main() {
 		setupLogging(ctlConf.LogDir, ctlConf.LogSizeLimitMB)
 	}

+	logger.WithField("logLevel", ctlConf.LogLevel).Info("Setting LogLevel configuration")


This all looks really good 👍

Thanks, we can split that up into two parts. I just scanned through the logging, when nothing happens, no events for new fleets, gameservers, but some messages like syncing, enqueuing appears always.
Need to add log example to the ticket.

markmandel · 2019-12-09T22:14:24Z

pkg/fleetautoscalers/controller.go

@@ -115,7 +115,7 @@ func (c *Controller) Run(workers int, stop <-chan struct{}) error {
 		return err
 	}

-	c.baseLogger.Info("Wait for cache sync")
+	c.baseLogger.Debug("Wait for cache sync")


I'm wondering if we want to move the info=>debug logging items to a separate PR?

I'm just wondering if we want to come up with some rules of thumb on what should be info and what should be debug? @pooneh-m @roberthbailey what do you think? I've no idea if there is prior art here?

Just thinking long term, and also for consistency?

Rules of thumb sound good, but we should discuss them in an issue (so they aren't lost in this PR) and document them somewhere.

I've created #1223.

https://reflectoring.io/logging-levels/ has some suggestions. :) I don't see an easy way of describing logging level other than use your judgement and get a code review for a second opinion. My judgement is if I delete a log would it reduce my confidence that the system is doing what is expected, if so make it Info, otherwise Debug.

markmandel · 2019-12-12T23:14:24Z

If we want to push this PR through, happy to take the logLevel configuration options (and fix the linting issues).

If we remove the logging level changes, I can re-review, and likely approve.

markmandel · 2019-12-13T00:26:37Z

Couple of other thoughts:

In this PR, should we edit the make install commands to install at the debug/trace logging level? Seems like we would want this as we are developing?
If you set the logging level for the controller, should that also be the default log level for all gameservers as well?

Conflicts when attempting to update Kubernetes resources is expected and will happen frequently in Agones. Currently they are reported as errors, and (a) produce a lot of noise, and (b) are reported as errors, which can often be seen as potential issues by end users, as well as red-herring reasons for actual issues. We may want to wait until googleforgames#1220 is complete before merging. Will help with issues also raised in googleforgames#1218.

Conflicts when attempting to update Kubernetes resources is expected and will happen frequently in Agones. Currently they are reported as errors, and (a) produce a lot of noise, and (b) are reported as errors, which can often be seen as potential issues by end users, as well as red-herring reasons for actual issues. We may want to wait until #1220 is complete before merging. Will help with issues also raised in #1218.

aLekSer · 2019-12-16T12:21:52Z

In this PR, should we edit the make install commands to install at the debug/trace logging level? Seems like we would want this as we are developing?

Yes, I will add this into install make target.

If you set the logging level for the controller, should that also be the default log level for all gameservers as well?

This is a bit more tricky one, I see at as a different PR.

aLekSer · 2019-12-16T12:23:27Z

Moving out log level changes into a separate branch.

agones-bot · 2019-12-16T12:30:37Z

Build Failed 😱

Build Id: 6116020e-a5f2-4bc2-84ae-739bc827feb8

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Add log level parameter to Helm. Use debug as default level for the `make install` target.

agones-bot · 2019-12-16T13:01:22Z

Build Succeeded 👏

Build Id: 79f29941-7cee-4fe3-998e-8ef3c33e74e1

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-155209f
image: gcr.io/agones-images/agones-sdk:1.3.0-155209f
image: gcr.io/agones-images/agones-ping:1.3.0-155209f
Linux C++ SDK (build): agonessdk-1.3.0-155209f-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-155209f.zip

A preview of the website (the last 30 builds are retained):

https://155209f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1220/head:pr_1220 && git checkout pr_1220
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-155209f

agones-bot · 2019-12-16T13:13:28Z

Build Failed 😱

Build Id: dd3be0a4-2c4f-4b3f-adc2-01780a040ba9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel · 2019-12-16T22:42:42Z

Retrying. Looks like a flaky Multi-allocator e2e test.

agones-bot · 2019-12-16T22:55:03Z

Build Succeeded 👏

Build Id: 4977e6e0-b9cc-4b1b-89ae-a736858c0586

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-a7e199b
image: gcr.io/agones-images/agones-sdk:1.3.0-a7e199b
image: gcr.io/agones-images/agones-ping:1.3.0-a7e199b
Linux C++ SDK (build): agonessdk-1.3.0-a7e199b-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-a7e199b.zip

A preview of the website (the last 30 builds are retained):

https://a7e199b-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1220/head:pr_1220 && git checkout pr_1220
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-a7e199b

markmandel

Love it!

google-oss-robot · 2019-12-16T23:07:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, markmandel

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markmandel]

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

Conflicts when attempting to update Kubernetes resources is expected and will happen frequently in Agones. Currently they are reported as errors, and (a) produce a lot of noise, and (b) are reported as errors, which can often be seen as potential issues by end users, as well as red-herring reasons for actual issues. We may want to wait until googleforgames#1220 is complete before merging. Will help with issues also raised in googleforgames#1218.

Add log level parameter to Helm. Use debug as default level for the `make install` target.

google-oss-robot added the do-not-merge/work-in-progress label Dec 9, 2019

google-oss-robot requested review from EricFortin and roberthbailey December 9, 2019 12:45

google-oss-robot added the size/M label Dec 9, 2019

aLekSer force-pushed the feature/loglevel branch from 7265ab0 to 4fe59cb Compare December 9, 2019 12:45

markmandel added feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) area/operations Installation, updating, metrics etc kind/feature New features for Agones labels Dec 9, 2019

markmandel reviewed Dec 9, 2019

View reviewed changes

markmandel mentioned this pull request Dec 9, 2019

Configurable Log Level for Agones controllers #1218

Closed

roberthbailey mentioned this pull request Dec 10, 2019

Create and document rules of thumb for log levels in agones code #1223

Closed

markmandel removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Dec 12, 2019

markmandel mentioned this pull request Dec 13, 2019

Move conflict messaged to Debug Logging #1232

Merged

aLekSer force-pushed the feature/loglevel branch from 4fe59cb to fa8073a Compare December 16, 2019 12:24

aLekSer force-pushed the feature/loglevel branch from fa8073a to 155209f Compare December 16, 2019 12:47

google-oss-robot added size/S and removed size/M labels Dec 16, 2019

Configurable Log level for Agones Controller

a7e199b

Add log level parameter to Helm. Use debug as default level for the `make install` target.

aLekSer force-pushed the feature/loglevel branch from 155209f to a7e199b Compare December 16, 2019 13:00

aLekSer marked this pull request as ready for review December 16, 2019 13:02

google-oss-robot removed the do-not-merge/work-in-progress label Dec 16, 2019

markmandel approved these changes Dec 16, 2019

View reviewed changes

google-oss-robot assigned markmandel Dec 16, 2019

google-oss-robot added the lgtm label Dec 16, 2019

google-oss-robot added the approved label Dec 16, 2019

markmandel merged commit 05b58df into googleforgames:master Dec 16, 2019

markmandel added this to the 1.3.0 milestone Dec 16, 2019

aLekSer deleted the feature/loglevel branch December 17, 2019 09:24

ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020

Configurable Log level for Agones Controller (googleforgames#1220)

57f06c7

Add log level parameter to Helm. Use debug as default level for the `make install` target.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable Log Level for Agones Controller #1220

Configurable Log Level for Agones Controller #1220

aLekSer commented Dec 9, 2019 •

edited

Loading

agones-bot commented Dec 9, 2019

agones-bot commented Dec 9, 2019

markmandel Dec 9, 2019

aLekSer Dec 10, 2019 •

edited

Loading

markmandel Dec 9, 2019 •

edited

Loading

roberthbailey Dec 10, 2019

pooneh-m Dec 10, 2019 •

edited

Loading

markmandel commented Dec 12, 2019

markmandel commented Dec 13, 2019

aLekSer commented Dec 16, 2019 •

edited

Loading

aLekSer commented Dec 16, 2019

agones-bot commented Dec 16, 2019

agones-bot commented Dec 16, 2019

agones-bot commented Dec 16, 2019

markmandel commented Dec 16, 2019

agones-bot commented Dec 16, 2019

markmandel left a comment

google-oss-robot commented Dec 16, 2019

Configurable Log Level for Agones Controller #1220

Configurable Log Level for Agones Controller #1220

Conversation

aLekSer commented Dec 9, 2019 • edited Loading

agones-bot commented Dec 9, 2019

agones-bot commented Dec 9, 2019

markmandel Dec 9, 2019

Choose a reason for hiding this comment

aLekSer Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

markmandel Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

roberthbailey Dec 10, 2019

Choose a reason for hiding this comment

pooneh-m Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

markmandel commented Dec 12, 2019

markmandel commented Dec 13, 2019

aLekSer commented Dec 16, 2019 • edited Loading

aLekSer commented Dec 16, 2019

agones-bot commented Dec 16, 2019

agones-bot commented Dec 16, 2019

agones-bot commented Dec 16, 2019

markmandel commented Dec 16, 2019

agones-bot commented Dec 16, 2019

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Dec 16, 2019

aLekSer commented Dec 9, 2019 •

edited

Loading

aLekSer Dec 10, 2019 •

edited

Loading

markmandel Dec 9, 2019 •

edited

Loading

pooneh-m Dec 10, 2019 •

edited

Loading

aLekSer commented Dec 16, 2019 •

edited

Loading