Implementation of Local SDK Server Player Tracking #1496

markmandel · 2020-04-25T16:35:10Z

What type of PR is this?

Uncomment only one /kind <> line, hit 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

What this PR does / Why we need it:

Implementation and unit tests for Player Tracking for the local sdk
server.

Which issue(s) this PR fixes:

Work on #1033

Special notes for your reviewer:

Conformance tests will come after this.

markmandel · 2020-04-25T16:35:47Z

pkg/sdkserver/localsdk.go

@@ -498,7 +605,7 @@ func (l *LocalSDKServer) EqualSets(expected, received []string) bool {
 func (l *LocalSDKServer) compare() {
 	if l.testMode {
 		if !l.EqualSets(l.expectedSequence, l.requestSequence) {
-			l.logger.Errorf("Testing Failed %v %v", l.expectedSequence, l.requestSequence)
+			logrus.WithField("expected", l.expectedSequence).WithField("received", l.requestSequence).Info("Testing Failed")


@aLekSer I snuck a small tweak to your conformance test reporting in here. WDYT?

Great, now it is much more readable. But same as above:

Suggested change

logrus.WithField("expected", l.expectedSequence).WithField("received", l.requestSequence).Info("Testing Failed")

l.logger.WithField("expected", l.expectedSequence).WithField("received", l.requestSequence).Info("Testing Failed")

agones-bot · 2020-04-25T16:52:42Z

Build Succeeded 👏

Build Id: c7ad5aa3-3746-4c3d-a35e-a1502bfd4c74

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1496/head:pr_1496 && git checkout pr_1496
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-c32bd85

aLekSer · 2020-04-29T21:01:27Z

pkg/sdkserver/localsdk.go

@@ -197,6 +198,8 @@ func (l *LocalSDKServer) recordRequestWithValue(request string, value string, ob
 			fieldVal = l.gs.ObjectMeta.Uid
 		case "PlayerCapacity":
 			fieldVal = strconv.FormatInt(l.gs.Status.Players.Capacity, 10)
+		case "PlayerIDs":
+			fieldVal = strings.Join(l.gs.Status.Players.IDs, "")


Should we use separator here? This seems to be for test, but might be more readable.

Just tried it, and it didn't break any tests! So yep, adding this in, in case people want to test against multiple values in the future.

aLekSer · 2020-04-29T21:03:11Z

pkg/sdkserver/localsdk.go

+	if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) {
+		return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")
+	}
+	logrus.Info("Getting Player Count")


I have added logger field recently, so we can set log level to all this logs.

Suggested change

logrus.Info("Getting Player Count")

l.logger.Info("Getting Player Count")

aLekSer · 2020-04-29T21:05:57Z

pkg/sdkserver/localsdk_test.go

@@ -374,7 +502,7 @@ func TestSDKConformanceFunctionality(t *testing.T) {
 	setAnnotation := "setannotation"
 	l.gs.ObjectMeta.Uid = exampleUID

-	expected := []string{}
+	var expected []string


Curious why this is better than before.

Oh, my IDE gave me a hint, and I clicked it. It's nicer because it doesn't allocate space to an array that is empty when nil will work just the same -- but for a test it's relatively inconsequential.

I can remove it if you would prefer?

No need to change, I think it would be better this way.

aLekSer · 2020-04-29T21:09:43Z

pkg/sdkserver/localsdk.go

+
+// IsPlayerConnected returns if the playerID is currently connected to the GameServer.
+// [Stage:Alpha]
+// [FeatureFlag:PlayerTesting]


Should it be PlayerTracking as in FeatureFlag Parse below?

Suggested change

// [FeatureFlag:PlayerTesting]

// [FeatureFlag:PlayerTracking]

Thank you - I missed that.

aLekSer · 2020-04-29T23:28:38Z

pkg/sdkserver/localsdk.go

+	if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) {
+		return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")
+	}
+	logrus.WithField("playerID", id.PlayerID).Info("Player Connected")


Suggested change

logrus.WithField("playerID", id.PlayerID).Info("Player Connected")

l.logger.WithField("playerID", id.PlayerID).Info("Player Connected")

Ah very nice, I missed that. Making these changes 👍

aLekSer · 2020-04-29T23:28:55Z

pkg/sdkserver/localsdk.go

+	if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) {
+		return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")
+	}
+	logrus.WithField("playerID", id.PlayerID).Info("Player Disconnected")


Suggested change

logrus.WithField("playerID", id.PlayerID).Info("Player Disconnected")

l.logger.WithField("playerID", id.PlayerID).Info("Player Disconnected")

aLekSer · 2020-04-29T23:29:11Z

pkg/sdkserver/localsdk.go

+	}
+
+	result := &alpha.Bool{Bool: false}
+	logrus.WithField("playerID", id.PlayerID).Info("Is a Player Connected?")


Suggested change

logrus.WithField("playerID", id.PlayerID).Info("Is a Player Connected?")

l.logger.WithField("playerID", id.PlayerID).Info("Is a Player Connected?")

aLekSer · 2020-04-29T23:29:23Z

pkg/sdkserver/localsdk.go

+	if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) {
+		return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")
+	}
+	logrus.Info("Getting Connected Players")


same as above

aLekSer · 2020-04-29T23:30:10Z

pkg/sdkserver/localsdk_test.go

+	assert.NoError(t, err)
+	assert.Equal(t, []string{id.PlayerID}, list.List)
+
+	// should return an error if we try and add another, since we're at capacity


Suggested change

// should return an error if we try and add another, since we're at capacity

// should return an error if we try to add another, since we're at capacity

aLekSer

There are few nits which are pending.

agones-bot · 2020-04-30T00:27:11Z

Build Succeeded 👏

Build Id: 29c06855-0f17-4f40-b42a-d61aa31e9a7f

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

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

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

https://8d3ddde-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1496/head:pr_1496 && git checkout pr_1496
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-8d3ddde

aLekSer

Great, all nits are fixed.

google-oss-robot · 2020-04-30T16:31:00Z

[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

Implementation and unit tests for Player Tracking for the local sdk server. Conformance tests will come after this. Work on googleforgames#1033

google-oss-robot · 2020-04-30T20:32:25Z

New changes are detected. LGTM label has been removed.

agones-bot · 2020-04-30T20:51:01Z

Build Succeeded 👏

Build Id: 79c3ed1b-70a4-4d18-ae15-da950640011f

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

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

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

https://71dd532-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1496/head:pr_1496 && git checkout pr_1496
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-71dd532

Implementation and unit tests for Player Tracking for the local sdk server. Conformance tests will come after this. Work on googleforgames#1033

google-oss-robot requested review from pooneh-m and roberthbailey April 25, 2020 16:35

google-oss-robot added approved size/L labels Apr 25, 2020

markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones and removed approved size/L labels Apr 25, 2020

markmandel commented Apr 25, 2020

View reviewed changes

markmandel added the size/L label Apr 25, 2020

aLekSer reviewed Apr 29, 2020

View reviewed changes

markmandel force-pushed the feature/local-tracking branch from c32bd85 to 8d3ddde Compare April 30, 2020 00:09

google-oss-robot added the approved label Apr 30, 2020

aLekSer approved these changes Apr 30, 2020

View reviewed changes

google-oss-robot assigned aLekSer Apr 30, 2020

google-oss-robot added the lgtm label Apr 30, 2020

Implementation of Local SDK Server Player Tracking

71dd532

Implementation and unit tests for Player Tracking for the local sdk server. Conformance tests will come after this. Work on googleforgames#1033

markmandel force-pushed the feature/local-tracking branch from 8d3ddde to 71dd532 Compare April 30, 2020 20:32

google-oss-robot removed the lgtm label Apr 30, 2020

markmandel merged commit 4d2ac66 into googleforgames:master Apr 30, 2020

markmandel added this to the 1.6.0 milestone Apr 30, 2020

markmandel deleted the feature/local-tracking branch April 30, 2020 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Local SDK Server Player Tracking #1496

Implementation of Local SDK Server Player Tracking #1496

markmandel commented Apr 25, 2020

markmandel Apr 25, 2020

aLekSer Apr 29, 2020

agones-bot commented Apr 25, 2020

aLekSer Apr 29, 2020 •

edited

markmandel Apr 30, 2020

aLekSer Apr 29, 2020 •

edited

aLekSer Apr 29, 2020

markmandel Apr 29, 2020

aLekSer Apr 29, 2020

aLekSer Apr 29, 2020 •

edited

markmandel Apr 29, 2020

aLekSer Apr 29, 2020

markmandel Apr 29, 2020

aLekSer Apr 29, 2020

aLekSer Apr 29, 2020

aLekSer Apr 29, 2020

aLekSer Apr 29, 2020

aLekSer left a comment

agones-bot commented Apr 30, 2020

aLekSer left a comment

google-oss-robot commented Apr 30, 2020

google-oss-robot commented Apr 30, 2020

agones-bot commented Apr 30, 2020

	logrus.WithField("expected", l.expectedSequence).WithField("received", l.requestSequence).Info("Testing Failed")
	l.logger.WithField("expected", l.expectedSequence).WithField("received", l.requestSequence).Info("Testing Failed")

	logrus.Info("Getting Player Count")
	l.logger.Info("Getting Player Count")

	// [FeatureFlag:PlayerTesting]
	// [FeatureFlag:PlayerTracking]

	logrus.WithField("playerID", id.PlayerID).Info("Player Connected")
	l.logger.WithField("playerID", id.PlayerID).Info("Player Connected")

	// should return an error if we try and add another, since we're at capacity
	// should return an error if we try to add another, since we're at capacity

Implementation of Local SDK Server Player Tracking #1496

Implementation of Local SDK Server Player Tracking #1496

Conversation

markmandel commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Apr 25, 2020

aLekSer Apr 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer Apr 29, 2020 • edited

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

aLekSer Apr 29, 2020 • edited

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

Choose a reason for hiding this comment

aLekSer left a comment

Choose a reason for hiding this comment

agones-bot commented Apr 30, 2020

aLekSer left a comment

Choose a reason for hiding this comment

google-oss-robot commented Apr 30, 2020

google-oss-robot commented Apr 30, 2020

agones-bot commented Apr 30, 2020

aLekSer Apr 29, 2020 •

edited

aLekSer Apr 29, 2020 •

edited

aLekSer Apr 29, 2020 •

edited