Define SDKServer LogLevel early #3631

Kalaiselvi84 · 2024-02-02T18:16:13Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3629

Special notes for your reviewer:

Kalaiselvi84 · 2024-02-02T18:20:01Z

I've defined the sdkServer logLevel at the beginning of the sdk-server/main.go file. Please let me know if this is the correct approach and what are the other necessary changes required?

agones-bot · 2024-02-02T18:26:19Z

Build Failed 😱

Build Id: 5f6842c0-3883-45d3-8b3f-8c9c3e4a7a85

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

markmandel · 2024-02-06T06:37:43Z

I've defined the sdkServer logLevel at the beginning of the sdk-server/main.go file. Please let me know if this is the correct approach and what are the other necessary changes required?

To add the environment variable to the sidecar, you will need to edit this function to add an environment variable to the set that already exists:

agones/pkg/gameservers/controller.go

Line 641 in 5f58fce

func (c *Controller) sidecar(gs *agonesv1.GameServer) corev1.Container {

agones-bot · 2024-02-06T22:37:01Z

Build Failed 😱

Build Id: cabb1ae6-ad2b-4e07-bbbf-3fd75e5007a0

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

Kalaiselvi84 · 2024-02-06T22:37:34Z

To add the environment variable to the sidecar, you will need to edit this function to add an environment variable to the set that already exists:

I've added an environment variable to the sidecar method in the pkg/gameservers/controller.go file and updated the Run method to reflect this capture in the pkg/sdkserver/sdkserver.go file. Does this change seem correct to you? Please let me know if anything else needs to be modified.

agones-bot · 2024-02-06T23:04:46Z

Build Failed 😱

Build Id: dcd467b6-3e41-46bc-8c16-9cdb2c2bed38

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

agones-bot · 2024-02-07T00:44:27Z

Build Succeeded 👏

Build Id: 044857f0-d3fe-4edc-b250-877ecf04dc9e

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

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

https://5fbabcc-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-5fbabcc-amd64

markmandel · 2024-02-12T04:49:21Z

pkg/gameservers/controller.go

@@ -659,6 +659,10 @@ func (c *Controller) sidecar(gs *agonesv1.GameServer) corev1.Container {
 				Name:  "FEATURE_GATES",
 				Value: runtime.EncodeFeatures(),
 			},
+			{
+				Name:  "LOG_LEVEL",
+				Value: "info",


This shouldn't be hard coded, it should take the value from the logLevel field on https://agones.dev/site/docs/reference/agones_crd_api_reference/#agones.dev/v1.SdkServer

Then the user can configure it themselves.

markmandel · 2024-02-12T05:00:17Z

pkg/sdkserver/sdkserver.go

-	// grab configuration details
-	if gs.Spec.SdkServer.LogLevel != "" {
-		logLevel = gs.Spec.SdkServer.LogLevel
+	var logLevel string


So the one that really needs changing is here:

agones/cmd/sdk-server/main.go

Lines 72 to 75 in 8b2c442

func main() {

ctlConf := parseEnvFlags()

logger.WithField("version", pkg.Version).WithField("featureGates", runtime.EncodeFeatures()).

WithField("ctlConf", ctlConf).Info("Starting sdk sidecar")

Which is some of the Info level output that was happening when logging was set to Warning/Error only.

What I'm thinking we should do here, is take a logrus.Level as part of NewSDKServer (which will mean refactoring some unit tests, and some error handling code) and passing it in after parsing in main.go.

Then we can set the s.logger.Logger.SetLevel(level) right under:
https://github.com/googleforgames/agones/pull/3631/files#diff-2b47120b02bc51219ccd6d97b8fe68f9d5043997eba480c0655babcafd771e4eR185 (line 185), so that is also covers all logging in this file.

Hopefully that all was clear!

Following your guidance, I made below modifications:

Removed the hardcoded log level in controller.go and replaced it with gs.Spec.SdkServer.LogLevel.

Added log level as a parameter in the NewSDKServer function to accept the log level.

Added log level parsing in main.go and passed it to NewSDKServer function.

Set s.logger.Logger.SetLevel(logLevel) right after its initialization in the NewSDKServer function.

Modified the controller_test.go and sdkserver_test.go files based on the lint suggestion.

Please let me know if anything missing

agones-bot · 2024-02-12T20:29:14Z

Build Failed 😱

Build Id: 1369b9d8-b25e-4b05-9a01-0e4c6b8420e1

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

agones-bot · 2024-02-12T23:04:03Z

Build Succeeded 👏

Build Id: a6b36772-984b-46fe-ba64-8ad66374399a

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-e3ea7a8-amd64

markmandel

Couple of small things, but overall looks good.

Will need a manual test!

To test, take a single GameSever such as: https://github.com/googleforgames/agones/blob/main/examples/simple-game-server/gameserver.yaml and set different log levels (Info, warn, error, debug). See how it changes the log output for the sidecar.

markmandel · 2024-02-12T23:44:07Z

cmd/sdk-server/main.go

@@ -70,6 +71,14 @@ var (
 )

 func main() {
+	logLevelEnv := os.Getenv("LOG_LEVEL")


Rather than use os.Getenv - please make this part of the parseEnvFlags() function and object that it returns - then we have all argument and environment variable parsing in a single place.

Quick question: I noticed that we're setting default values in our parseEnvFlags() function and binding an environment variable with the key. Do we need to follow the same pattern for the log level by using viper.SetDefault("logLevel", "info")?

Please ignore the above question. I've tested the latest code changes with the Info, Error, Debug, and Warn log levels. Please review the logs at https://gist.github.com/Kalaiselvi84/74fe77e6cce7ebc55485efbeb1e0d79d and share your feedback.

Could you please confirm if the manual testing looks fine? Is this how we should be testing it?

Yep! Looks perfect! 👍🏻

markmandel · 2024-02-12T23:45:10Z

pkg/sdkserver/sdkserver.go

-	if gs.Spec.SdkServer.LogLevel != "" {
-		logLevel = gs.Spec.SdkServer.LogLevel
+	var logLevel string
+	logLevelEnv := os.Getenv("LOG_LEVEL")


This section can be deleted since we set the log level in the constructor.

agones-bot · 2024-02-13T04:16:02Z

Build Failed 😱

Build Id: 70b31dca-e43f-45b7-b6a3-ce00b6f2dc14

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

gongmax · 2024-02-13T17:56:53Z

#6 34.23 Err:1 http://deb.debian.org/debian bullseye/main amd64 wget amd64 1.21-1+deb11u1
#6 34.23   Could not connect to debian.map.fastlydns.net:80 (199.232.98.132), connection timed out Unable to connect to deb.debian.org:http:
#6 34.24 E: Failed to fetch http://deb.debian.org/debian/pool/main/w/wget/wget_1.21-1%2bdeb11u1_amd64.deb  Could not connect to debian.map.fastlydns.net:80 (199.232.98.132), connection timed out Unable to connect to deb.debian.org:http:
#6 34.24 E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
#6 ERROR: executor failed running [/bin/sh -c apt-get --allow-releaseinfo-change update &&     apt-get install -y wget]: exit code: 100
------
 > [2/7] RUN apt-get --allow-releaseinfo-change update &&     apt-get install -y wget:

3.917 The following NEW packages will be installed:
3.918   wget
34.23 0 upgraded, 1 newly installed, 0 to remove and 22 not upgraded.
34.23 Need to get 964 kB of archives.
34.23 After this operation, 3559 kB of additional disk space will be used.
34.23 Err:1 http://deb.debian.org/debian bullseye/main amd64 wget amd64 1.21-1+deb11u1
34.23   Could not connect to debian.map.fastlydns.net:80 (199.232.98.132), connection timed out Unable to connect to deb.debian.org:http:
34.24 E: Failed to fetch http://deb.debian.org/debian/pool/main/w/wget/wget_1.21-1%2bdeb11u1_amd64.deb  Could not connect to debian.map.fastlydns.net:80 (199.232.98.132), connection timed out Unable to connect to deb.debian.org:http:
34.24 E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
------
ERROR: failed to solve: executor failed running [/bin/sh -c apt-get --allow-releaseinfo-change update &&     apt-get install -y wget]: exit code: 100
make[5]: *** [includes/sdk.mk:109: build-build-sdk-image] Error 1
make[5]: Leaving directory '/workspace/build'
make[4]: Leaving directory '/workspace/build'
make[4]: *** [includes/build-image.mk:63: ensure-image] Error 2
make[3]: *** [includes/sdk.mk:126: ensure-build-sdk-image] Error 2
make[3]: Leaving directory '/workspace/build'
Unable to find image 'agones-build-sdk-csharp:dbf73e956b' locally
docker: Error response from daemon: pull access denied for agones-build-sdk-csharp, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
make[2]: *** [includes/sdk.mk:91: run-sdk-command] Error 125
make[2]: Leaving directory '/workspace/build'
make[1]: *** [includes/sdk.mk:87: run-sdk-command-csharp] Error 2
make[1]: Leaving directory '/workspace/build'
make: *** [includes/sdk.mk:239: test-gen-all-sdk-grpc] Error 2

agones-bot · 2024-02-13T18:53:37Z

Build Succeeded 👏

Build Id: 03e12482-1462-48d3-a138-3b52a193d4a9

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

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

https://954cd1f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-954cd1f-amd64

agones-bot · 2024-02-13T20:03:14Z

Build Succeeded 👏

Build Id: e48caa8d-e3c2-40f8-bef9-97058efa8bac

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

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

https://63b3640-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-63b3640-amd64

igooch · 2024-02-15T01:59:09Z

cmd/sdk-server/main.go

 	gwruntime "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"


@markmandel it looks like this is the same package that we use in our other controllers. Is there a reason we use this one over https://pkg.go.dev/log/slog?

Slog didn't exist when we started this project 😄

agones-bot · 2024-02-15T03:02:18Z

Build Succeeded 👏

Build Id: 713b3e1e-1b3b-470f-9801-b58ebd74360e

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

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

https://63b3640-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-63b3640-amd64

markmandel

Looks great - a couple of lines to remove, and I think this is good to go 👍🏻

markmandel · 2024-02-15T03:48:43Z

pkg/sdkserver/sdkserver.go

+	currentLogLevel := s.logger.Logger.GetLevel()
+	s.logger.WithField("logLevel", currentLogLevel).Debug("Setting LogLevel configuration")


Suggested change

currentLogLevel := s.logger.Logger.GetLevel()

s.logger.WithField("logLevel", currentLogLevel).Debug("Setting LogLevel configuration")

Don't think we need these lines anymore.

I've taken out these two lines.

agones-bot · 2024-02-15T05:54:35Z

Build Succeeded 👏

Build Id: 62d29939-5812-45c9-81bf-220eb59399af

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

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

https://87071a8-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-87071a8-amd64

markmandel · 2024-02-20T04:51:23Z

Just a note - this is approved, you can take it out of draft 😄

agones-bot · 2024-02-20T17:54:55Z

Build Succeeded 👏

Build Id: 1f78b3c0-acb5-423f-bf73-670c082fc4fa

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

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

https://9b35dbf-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3631/head:pr_3631 && git checkout pr_3631
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-9b35dbf-amd64

Define SDKServer LogLevel early

9deaad3

Kalaiselvi84 requested review from markmandel and gongmax February 2, 2024 18:16

github-actions bot added kind/other size/S labels Feb 2, 2024

Modify sidecar and Run method

c64c0cc

fix lint

47909d5

add assertion for LOG_LEVEL

5fbabcc

markmandel reviewed Feb 12, 2024

View reviewed changes

Reveiw changes

014c3a6

fix expected and acutal values in assertion

e3ea7a8

markmandel reviewed Feb 12, 2024

View reviewed changes

review changes

954cd1f

Merge branch 'main' into pr-3629

63b3640

igooch reviewed Feb 15, 2024

View reviewed changes

markmandel reviewed Feb 15, 2024

View reviewed changes

remove log from sdkserver.go

87071a8

markmandel approved these changes Feb 15, 2024

View reviewed changes

Kalaiselvi84 marked this pull request as ready for review February 20, 2024 16:44

Kalaiselvi84 added 2 commits February 20, 2024 16:45

Merge branch 'main' into pr-3629

d9fbd32

Merge branch 'main' into pr-3629

9b35dbf

markmandel merged commit 492cbb9 into googleforgames:main Feb 20, 2024
4 checks passed

Kalaiselvi84 added kind/bug These are bugs. and removed kind/other labels Mar 8, 2024

Kalaiselvi84 deleted the pr-3629 branch March 15, 2024 01:01

KAllan357 mentioned this pull request Jun 5, 2024

sdk-server expects SDK_LOG_LEVEL #3858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define SDKServer LogLevel early #3631

Define SDKServer LogLevel early #3631

Kalaiselvi84 commented Feb 2, 2024 •

edited by roberthbailey

Loading

Kalaiselvi84 commented Feb 2, 2024

agones-bot commented Feb 2, 2024

markmandel commented Feb 6, 2024

agones-bot commented Feb 6, 2024

Kalaiselvi84 commented Feb 6, 2024

agones-bot commented Feb 6, 2024

agones-bot commented Feb 7, 2024

markmandel Feb 12, 2024

markmandel Feb 12, 2024

Kalaiselvi84 Feb 12, 2024

agones-bot commented Feb 12, 2024

agones-bot commented Feb 12, 2024

markmandel left a comment

markmandel Feb 12, 2024

Kalaiselvi84 Feb 13, 2024

Kalaiselvi84 Feb 13, 2024

Kalaiselvi84 Feb 15, 2024

markmandel Feb 15, 2024

markmandel Feb 12, 2024

agones-bot commented Feb 13, 2024

gongmax commented Feb 13, 2024

agones-bot commented Feb 13, 2024

agones-bot commented Feb 13, 2024

igooch Feb 15, 2024

markmandel Feb 15, 2024

agones-bot commented Feb 15, 2024

markmandel left a comment

markmandel Feb 15, 2024

Kalaiselvi84 Feb 15, 2024

agones-bot commented Feb 15, 2024

markmandel commented Feb 20, 2024

agones-bot commented Feb 20, 2024

	func main() {
	ctlConf := parseEnvFlags()
	logger.WithField("version", pkg.Version).WithField("featureGates", runtime.EncodeFeatures()).
	WithField("ctlConf", ctlConf).Info("Starting sdk sidecar")

		currentLogLevel := s.logger.Logger.GetLevel()
		s.logger.WithField("logLevel", currentLogLevel).Debug("Setting LogLevel configuration")

Define SDKServer LogLevel early #3631

Define SDKServer LogLevel early #3631

Conversation

Kalaiselvi84 commented Feb 2, 2024 • edited by roberthbailey Loading

Kalaiselvi84 commented Feb 2, 2024

agones-bot commented Feb 2, 2024

markmandel commented Feb 6, 2024

agones-bot commented Feb 6, 2024

Kalaiselvi84 commented Feb 6, 2024

agones-bot commented Feb 6, 2024

agones-bot commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Feb 12, 2024

agones-bot commented Feb 12, 2024

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Feb 13, 2024

gongmax commented Feb 13, 2024

agones-bot commented Feb 13, 2024

agones-bot commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Feb 15, 2024

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Feb 15, 2024

markmandel commented Feb 20, 2024

agones-bot commented Feb 20, 2024

Kalaiselvi84 commented Feb 2, 2024 •

edited by roberthbailey

Loading