bump: joonix/log to NewFormater() #3342

jonsch318 · 2023-08-22T21:55:30Z

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:

Bump the version of joonix/log to the (new) joonix.NewFormatter().

I would suggest to remove the dependency all together. But at least keep the version up to date.

Which issue(s) this PR fixes:

Closes #3341

Special notes for your reviewer:

google-cla · 2023-08-22T21:55:34Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

agones-bot · 2023-08-22T23:49:01Z

Build Succeeded 👏

Build Id: 162ee2ba-3ca8-4b55-8a44-ae90e8580f81

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://513996e-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3342/head:pr_3342 && git checkout pr_3342
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.35.0-dev-513996e-amd64

aimuz · 2023-08-23T01:47:42Z

pkg/util/runtime/runtime.go

@@ -36,7 +36,7 @@ type stackTracer interface {

 // replace the standard glog error logger, with a logrus one
 func init() {
-	logrus.SetFormatter(&joonix.FluentdFormatter{})
+	logrus.SetFormatter(joonix.NewFormatter())


Do the fields match exactly?

the fluentd format only uses three field: "time", "severity", "message". The issue joonix/log#2 sets the default JSONFormatter with a FieldMap as a alternative.

Which i have now used in the latest commit. and removed the dependency for joonix

aimuz · 2023-08-23T06:51:44Z

Can you give the log output before this change, as well as the log output after the change, for review?

jonsch318 · 2023-08-23T06:52:03Z

Actually then most of the dependency upgrades is unnecessary because I removed the dependency to joonix/log.

aimuz · 2023-08-23T06:52:57Z

If the log output is not affected, I believe this is a good change.

jonsch318 · 2023-08-23T06:54:17Z

Can you give the log output before this change, as well as the log output after the change, for review?

Yeah let me first try to run it where i can see some logs.

agones-bot · 2023-08-23T08:21:23Z

Build Succeeded 👏

Build Id: 0fd23497-46e3-4a51-bf31-f54a1a35cf57

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://4ed9b78-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3342/head:pr_3342 && git checkout pr_3342
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.35.0-dev-4ed9b78-amd64

aimuz · 2023-08-23T09:48:54Z

unchanged

{"message":"Player Disconnected","playerID":"1234","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T00:27:37.928895153Z"}
{"message":"Gameserver update received","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T00:27:37.928930197Z"}

changed

{"message":"Player Disconnected","playerID":"1234","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T07:08:31Z"}
{"message":"Gameserver update received","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T07:08:31Z"}

Looks like the format of the log has changed, Can you make it consistent?

@JohnnyS318

aimuz · 2023-08-23T09:54:31Z

pkg/util/runtime/runtime.go

@@ -36,7 +35,13 @@ type stackTracer interface {

 // replace the standard glog error logger, with a logrus one
 func init() {
-	logrus.SetFormatter(&joonix.FluentdFormatter{})
+	logrus.SetFormatter(&logrus.JSONFormatter{


We should keep the time format the same as before

jonsch318 · 2023-08-23T10:03:24Z

unchanged

{"message":"Player Disconnected","playerID":"1234","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T00:27:37.928895153Z"}
{"message":"Gameserver update received","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T00:27:37.928930197Z"}

changed

{"message":"Player Disconnected","playerID":"1234","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T07:08:31Z"}
{"message":"Gameserver update received","sdkName":"go","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2023-08-23T07:08:31Z"}

Looks like the format of the log has changed, Can you make it consistent?

@JohnnyS318

Should now be fixed. The FluentDFormatter used the RFC3339Nano timestamp format

agones-bot · 2023-08-23T11:00:19Z

Build Succeeded 👏

Build Id: dbcfff5a-e0ac-4a32-b940-f7909b01a088

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://48a3ae2-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3342/head:pr_3342 && git checkout pr_3342
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.35.0-dev-48a3ae2-amd64

aimuz

/lgtm

@markmandel I think this is a good change, do you have time to look at it?

markmandel · 2023-08-24T21:23:46Z

LGTM, just dragging @gongmax and @zmerlynn in to triple check, since logger changes have such a wide impact.

I don't think the slight change in time format should matter.

"time":"2023-08-23T00:27:37.928930197Z" vs "time":"2023-08-23T07:08:31Z" - Not sure we are required to go to the sub-nano second here 😃

gongmax

LGTM

google-oss-prow · 2023-08-24T21:31:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aimuz, gongmax, JohnnyS318
Once this PR has been reviewed and has the lgtm label, please assign alekser for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

jonsch318 · 2023-08-24T21:51:58Z

LGTM, just dragging @gongmax and @zmerlynn in to triple check, since logger changes have such a wide impact.

I don't think the slight change in time format should matter.

"time":"2023-08-23T00:27:37.928930197Z" vs "time":"2023-08-23T07:08:31Z" - Not sure we are required to go to the sub-nano second here 😃

Actually I'm not shure why the time format has changed. I looked at the joonix/log commit specified in the dependency and it was also using RFC3339Nano as the time format. So it should be exactly equal.

google-oss-prow · 2023-08-24T21:52:12Z

New changes are detected. LGTM label has been removed.

agones-bot · 2023-08-25T00:29:06Z

Build Failed 😱

Build Id: 35661960-721d-4e8d-8066-47d6ae1e919f

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

aimuz · 2023-08-25T05:43:34Z

Is it possible to add a retest command? For quickly retrying incorrect tests

agones-bot · 2023-08-25T23:22:29Z

Build Failed 😱

Build Id: eb65a1c2-d3bc-4ec1-82f3-3a3075e5f698

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

agones-bot · 2023-08-26T01:42:49Z

Build Succeeded 👏

Build Id: 87d0183d-711d-4fab-8697-025324ce51e9

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://a0e7cd7-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3342/head:pr_3342 && git checkout pr_3342
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.35.0-dev-a0e7cd7-amd64

bump: joonix/log to NewFormater()

513996e

google-oss-prow bot requested review from aLekSer and cyriltovena August 22, 2023 21:55

google-oss-prow bot added the size/XXL label Aug 22, 2023

github-actions bot added the kind/bug These are bugs. label Aug 22, 2023

aimuz reviewed Aug 23, 2023

View reviewed changes

remove joonix formatter for logrus FieldMap

4ed9b78

aimuz suggested changes Aug 23, 2023

View reviewed changes

google-oss-prow bot assigned aimuz Aug 23, 2023

fix included timestamp format RFC3339Nano

48a3ae2

aimuz approved these changes Aug 24, 2023

View reviewed changes

google-oss-prow bot added the lgtm label Aug 24, 2023

markmandel requested review from zmerlynn and gongmax and removed request for cyriltovena August 24, 2023 21:21

gongmax approved these changes Aug 24, 2023

View reviewed changes

google-oss-prow bot assigned gongmax Aug 24, 2023

Merge branch 'main' into main

75580d2

google-oss-prow bot removed the lgtm label Aug 24, 2023

markmandel enabled auto-merge (squash) August 24, 2023 22:20

Merge branch 'main' into main

e081da5

Merge branch 'main' into main

55ada02

Merge branch 'main' into main

a0e7cd7

markmandel merged commit 4d116d7 into googleforgames:main Aug 26, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bump: joonix/log to NewFormater() #3342

bump: joonix/log to NewFormater() #3342

jonsch318 commented Aug 22, 2023

google-cla bot commented Aug 22, 2023

agones-bot commented Aug 22, 2023

aimuz Aug 23, 2023

jonsch318 Aug 23, 2023 •

edited

Loading

aimuz commented Aug 23, 2023

jonsch318 commented Aug 23, 2023

aimuz commented Aug 23, 2023

jonsch318 commented Aug 23, 2023

agones-bot commented Aug 23, 2023

aimuz commented Aug 23, 2023

aimuz Aug 23, 2023

jonsch318 commented Aug 23, 2023

agones-bot commented Aug 23, 2023

aimuz left a comment

markmandel commented Aug 24, 2023 •

edited

Loading

gongmax left a comment

google-oss-prow bot commented Aug 24, 2023

jonsch318 commented Aug 24, 2023

google-oss-prow bot commented Aug 24, 2023

agones-bot commented Aug 25, 2023

aimuz commented Aug 25, 2023

agones-bot commented Aug 25, 2023

agones-bot commented Aug 26, 2023

bump: joonix/log to NewFormater() #3342

bump: joonix/log to NewFormater() #3342

Conversation

jonsch318 commented Aug 22, 2023

google-cla bot commented Aug 22, 2023

agones-bot commented Aug 22, 2023

aimuz Aug 23, 2023

Choose a reason for hiding this comment

jonsch318 Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

aimuz commented Aug 23, 2023

jonsch318 commented Aug 23, 2023

aimuz commented Aug 23, 2023

jonsch318 commented Aug 23, 2023

agones-bot commented Aug 23, 2023

aimuz commented Aug 23, 2023

aimuz Aug 23, 2023

Choose a reason for hiding this comment

jonsch318 commented Aug 23, 2023

agones-bot commented Aug 23, 2023

aimuz left a comment

Choose a reason for hiding this comment

markmandel commented Aug 24, 2023 • edited Loading

gongmax left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 24, 2023

jonsch318 commented Aug 24, 2023

google-oss-prow bot commented Aug 24, 2023

agones-bot commented Aug 25, 2023

aimuz commented Aug 25, 2023

agones-bot commented Aug 25, 2023

agones-bot commented Aug 26, 2023

jonsch318 Aug 23, 2023 •

edited

Loading

markmandel commented Aug 24, 2023 •

edited

Loading