Nodejs counters and lists #3726

steven-supersolid · 2024-03-24T17:52:16Z

What type of PR is this?
/kind feature

What this PR does / Why we need it:

Counts SDK
Lists SDK
Unit tests

Which issue(s) this PR fixes:
Closes #3645

Special notes for your reviewer:

The example would require a local game server created with a specifically named counter so unsure whether to continue with that
Conformance tests are not done yet although there seems to be overlap with the Node.js example, however the example does a lot more

agones-bot · 2024-03-24T18:09:07Z

Build Failed 😱

Build Id: cd4e2795-51eb-4952-82ad-43e4d70abc71

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

steven-supersolid · 2024-03-25T15:21:25Z

It is a bit difficult to work with the CounterUpdateRequest in JavaScript due to the type google.protobuf.Int64Value instead of int64

From alpha.protoc

// A representation of a Counter Update Request.
message CounterUpdateRequest {
  option (google.api.resource) = {
      type: "agones.dev/CounterUpdateRequest"
      pattern: "counterUpdateRequests/{counterUpdateRequest}"
  };
  // The name of the Counter to update
  string name = 1;
  // The value to set the Counter Count
  google.protobuf.Int64Value count = 2;
  // The value to set the Counter Capacity
  google.protobuf.Int64Value capacity = 3;
  // countDiff tracks if a Counter Update Request is CountIncrement (positive), CountDecrement
  // (negative), 0 if a CountSet or CapacitySet request
  int64 countDiff = 4;
}

I got it working with the following but this seems like an internal type. It's a bit hacky to require/import at a nested path because it is not a supported export of the library, so will break if the library gets refactored#

const jspbWrappers = require('google-protobuf/google/protobuf/wrappers_pb');
...
const capacity = new jspbWrappers.Int64Value();
capacity.setValue(amount);

I'm wondering why we don't use int64 instead as we use this in other places in the SDK (edited)

steven-supersolid · 2024-03-25T15:23:41Z

@igooch Would you be able to shed any light on this?

igooch · 2024-03-25T20:21:44Z

I'm wondering why we don't use int64 instead as we use this in other places in the SDK

The google.protobuf.Int64Value is there so that it will be a nullable object rather than a primitive type that has a default value (0 in the case of Golang). In the case of updating the Count or Capacity 0 is a meaningful value, so we don't want 0 to be the default. 0 for the "diff" isn't meaningful, since that's just adding the diff to the Count.

steven-supersolid · 2024-03-26T15:55:31Z

Thank you. Reading around, it also seems that google.protobuf.Int64Value is a recommended way to use a nullable int, so I wonder if there's a better way to import wrappers_pb in JavaScript. It may just be an issue with the library not exporting this useful type the correct way.

For my curiosity I was wondering if we could use a wrapper type e.g. the Count that we use in SetPlayerCapacity?

// Store a count variable.
message Count {
    int64 count = 1;
}

Also did we consider using -1 as a special value for count and capacity in the update to mean unspecified, as these would be invalid values? I don't know if these could be set by default to -1, so not setting them would leave them at -1.

markmandel · 2024-03-27T16:26:53Z

So we use these standard wrappers all over the place - I asked the gRPC team what the canonical way to import the standard wrappers is and will get back to you.

markmandel · 2024-03-27T16:57:59Z

Feedback I got was basically "if const jspbWrappers = require('google-protobuf/google/protobuf/wrappers_pb'); works, then that is the correct solution, and why that is there."

So I think we should move forward with this approach 👍🏻

steven-supersolid · 2024-03-28T15:23:20Z

Thank you and so it seems the library is missing some code as we should be able to do the following so a library user does not need to know the internal path

const jspbWrappers = require('google-protobuf').jspbWrappers

Or in the more modern format

const {jspbWrappers} = require('google-protobuf')

But yes, can go ahead with the current solution

steven-supersolid · 2024-03-28T15:29:31Z

protocolbuffers/protobuf-javascript#197

markmandel · 2024-04-01T22:21:09Z

That's a new one

ERROR: (gcloud.container.clusters.get-credentials) ResponseError: code=404, message=Not found: projects/agones-images/locations/asia-east1/clusters/standard-e2e-test-cluster-1-26.

You can ignore that.

Restarting test.

agones-bot · 2024-04-01T22:38:02Z

Build Failed 😱

Build Id: 14d2f577-ac55-42ea-a735-4d181de07309

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

markmandel · 2024-04-01T22:53:34Z

Oooh, you will need to rebase against main - there is no 1.26 cluster anymore.

igooch · 2024-04-01T23:21:03Z

@steven-supersolid we're updating the SDK, so that most of the methods that return (bool, error) are now simply returning (error). I updated the issue #3645. (Discussion: #3581#discussion_r1505135182). Also tracked in #3737.

steven-supersolid · 2024-04-02T08:35:24Z

Great, I think that is closer to the rest of the API. Will update

igooch · 2024-05-09T07:01:56Z

@steven-supersolid we moved Counts and Lists from Alpha to Beta #3806. This is the last major expected change (until it goes into stable, but I imagine that will be several more releases away).

github-actions · 2024-05-23T15:32:36Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-05-23T15:49:08Z

Build Failed 😱

Build Id: 2e4fb29a-2a11-4ea5-885b-8479cb907465

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

github-actions · 2024-05-23T16:00:13Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-05-23T17:01:56Z

Build Succeeded 👏

Build Id: 505e54a6-c93d-48d0-9f55-b4ec3f4acf36

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3726/head:pr_3726 && git checkout pr_3726
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.41.0-dev-e7d5c6c-amd64

steven-supersolid · 2024-05-23T17:44:44Z

@igooch this should now be ready

igooch

A caveat that I'm not familiar with nodejs, so if you know of someone with more language specific knowledge that might have more insight, feel free to tag them in the comments.

Overall looks good, just a few clarifying comments.

Some of the language SDKs have unit tests. Do you think this would be beneficial to add, or would it not add anything beyond what's already being tested in test/sdk/nodejs/testSDKClient.js?

igooch · 2024-05-28T20:03:24Z

test/sdk/nodejs/testSDKClient.js

+	// LocalSDKServer starting "rooms": {Count: 1, Capacity: 10}
+	const counter = "rooms";
+
+	try {


Genuine question here as I'm not familiar with nodejs, is there an advantage to using a try catch block over an assert statement like https://nodejs.org/api/assert.html#assertdoesnotrejectasyncfn-error-message?

I've not used the Assert class much because it is usual to rely on a test runner framework such as jasmine or mocha instead.
In runtime code it is standard to rely on try/catch when dealing with async functions. In test code it is convenient to do the same.

I think in this case as we want to also check the return value it is easier to write the code with try/catch. We end up nesting in a try/catch block vs. nesting in a function call, which is preferable.

It may be worth investigating in another PR and also investigate if we could swap out jasmine for the new node.js built in test runner, which relies on assert.

igooch · 2024-05-28T20:34:48Z

build/includes/sdk.mk

@@ -165,7 +165,8 @@ run-sdk-conformance-test-cpp:
 	$(MAKE) run-sdk-conformance-test SDK_FOLDER=cpp GRPC_PORT=9003 HTTP_PORT=9103

 run-sdk-conformance-test-node:
-	$(MAKE) run-sdk-conformance-test SDK_FOLDER=node GRPC_PORT=9002 HTTP_PORT=9102
+	# run with on-by-default (Beta) feature flags enabled
+	$(MAKE) run-sdk-conformance-test SDK_FOLDER=node GRPC_PORT=9002 HTTP_PORT=9102 TESTS=$(DEFAULT_CONFORMANCE_TESTS),$(COUNTS_AND_LISTS_TESTS)


By itself, this fails with error:

~/agones/build$ make run-sdk-conformance-test-node ... > node ./testSDKClient.js node:internal/modules/cjs/loader:1148 throw err; ^ Error: Cannot find module '@grpc/grpc-js' Require stack: - /go/src/agones.dev/agones/sdks/nodejs/src/agonesSDK.js - /go/src/agones.dev/agones/sdks/nodejs/src/index.js - /go/src/agones.dev/agones/test/sdk/nodejs/testSDKClient.js at Module._resolveFilename (node:internal/modules/cjs/loader:1145:15) at Module._load (node:internal/modules/cjs/loader:986:27) at Module.require (node:internal/modules/cjs/loader:1233:19) at require (node:internal/modules/helpers:179:18) at Object.<anonymous> (/go/src/agones.dev/agones/sdks/nodejs/src/agonesSDK.js:15:14) at Module._compile (node:internal/modules/cjs/loader:1358:14) at Module._extensions..js (node:internal/modules/cjs/loader:1416:10) at Module.load (node:internal/modules/cjs/loader:1208:32) at Module._load (node:internal/modules/cjs/loader:1024:12) at Module.require (node:internal/modules/cjs/loader:1233:19) { code: 'MODULE_NOT_FOUND', requireStack: [ '/go/src/agones.dev/agones/sdks/nodejs/src/agonesSDK.js', '/go/src/agones.dev/agones/sdks/nodejs/src/index.js', '/go/src/agones.dev/agones/test/sdk/nodejs/testSDKClient.js' ] }

Once I run ~/agones/build$ SDK_FOLDER=node make test-sdk it installs the dependencies successfully, and then the command ~/agones/build$ make run-sdk-conformance-test-node runs successfully. If there's not a quick way to check that the build script was run as part of the run-sdk-conformance-test-node we should probably add a comment noting that SDK_FOLDER=node make test-sdk needs to be run first.

We would need to execute npm install in the sdk folder, e.g. in build-sdk-test.sh the minimum required is

cd /go/src/agones.dev/agones/test/sdk/nodejs npm install --quiet

Could we add something like that here? I don't know enough about these makefiles

Probably not worth modifying the make command, since the test gets build properly during CI / CD, so this only applies to local testing. I added in a suggested comment change above.

igooch · 2024-05-28T21:44:11Z

examples/nodejs-simple/gameserver-counts-and-lists.yaml

+    - name: default
+      portPolicy: Dynamic
+      containerPort: 7654
+  counters:


Did you want the example to have the same counter and list as the local SDK Server? The sample local SDK server counters is "rooms" and lists is "players"

agones/pkg/sdkserver/localsdk.go

Lines 76 to 83 in 4b2910b

if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {

gs.Status.Counters = map[string]*sdk.GameServer_Status_CounterStatus{

"rooms": {Count: 1, Capacity: 10},

}

gs.Status.Lists = map[string]*sdk.GameServer_Status_ListStatus{

"players": {Values: []string{"test0", "test1", "test2"}, Capacity: 100},

}

}

steven-supersolid · 2024-05-29T08:18:45Z

Some of the language SDKs have unit tests. Do you think this would be beneficial to add, or would it not add anything beyond what's already being tested in test/sdk/nodejs/testSDKClient.js?

The unit tests for the SDK can be found in sdks/nodejs/spec/ and all the new functions were covered with unit tests in this file sdks/nodejs/spec/betaAgonesSDK.spec.js in the PR. These are the must have tests and can be run by using npm run test in the sdk folder (assuming npm install has been run first). Code coverage is also set up and can be checked with npm run cover, the results are

78 specs, 0 failures
Finished in 0.198 seconds
--------------|---------|----------|---------|---------|-------------------
File          | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------|---------|----------|---------|---------|-------------------
All files     |     100 |      100 |   98.96 |     100 | 
 agonesSDK.js |     100 |      100 |   97.14 |     100 | 
 alpha.js     |     100 |      100 |     100 |     100 | 
 beta.js      |     100 |      100 |     100 |     100 | 
--------------|---------|----------|---------|---------|-------------------

There is an uncovered inner function in an error handler on line 103 of agonesSDK.js which may not be possible to test. It's unrelated to counts and lists.

github-actions · 2024-05-29T19:13:18Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-05-29T20:17:19Z

Build Succeeded 👏

Build Id: 0675c440-0b25-4c0b-a6c9-6102e035527f

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3726/head:pr_3726 && git checkout pr_3726
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.41.0-dev-935b47a-amd64

igooch

LGTM. Thanks for all your work on this!

build/includes/sdk.mk

igooch · 2024-05-31T01:43:39Z

build/includes/sdk.mk

@@ -165,7 +165,8 @@ run-sdk-conformance-test-cpp:
 	$(MAKE) run-sdk-conformance-test SDK_FOLDER=cpp GRPC_PORT=9003 HTTP_PORT=9103

 run-sdk-conformance-test-node:
-	$(MAKE) run-sdk-conformance-test SDK_FOLDER=node GRPC_PORT=9002 HTTP_PORT=9102
+	# run with on-by-default (Beta) feature flags enabled
+	$(MAKE) run-sdk-conformance-test SDK_FOLDER=node GRPC_PORT=9002 HTTP_PORT=9102 TESTS=$(DEFAULT_CONFORMANCE_TESTS),$(COUNTS_AND_LISTS_TESTS)


Probably not worth modifying the make command, since the test gets build properly during CI / CD, so this only applies to local testing. I added in a suggested comment change above.

Co-authored-by: igooch <igooch@google.com>

github-actions · 2024-05-31T08:12:28Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-05-31T08:13:00Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-05-31T08:59:24Z

Build Failed 😱

Build Id: a5ee1237-fa39-429b-b8a1-5f01066c1e56

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

steven-supersolid · 2024-05-31T14:19:39Z

HTML tests failing after merging in main

Html Test: Attempt 20
htmltest started at 08:58:03 on /tmp/website
========================================================================
site/docs/guides/client-sdks/unreal/index.html
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/setting-up-dedicated-servers-in-unreal-engine/
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/game-mode-and-game-state-in-unreal-engine/
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/API/Runtime/Engine/GameFramework/AGameMode/
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/API/Runtime/Engine/GameFramework/AGameSession/
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/build-operations-cooking-packaging-deploying-and-running-projects-in-unreal-engine/
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/API/Runtime/Engine/GameFramework/AGameMode/index.html
  Non-OK status: 403 --- site/docs/guides/client-sdks/unreal/index.html --> https://docs.unrealengine.com/en-US/API/Runtime/Engine/GameFramework/AGameSession/index.html
========================================================================
✘✘✘ failed in 2.372015068s
8 errors in 198 documents
make[1]: Leaving directory '/workspace/build'
make: *** [includes/website.mk:76: site-test] Error 2

igooch · 2024-05-31T16:58:38Z

HTML tests failing after merging in main

Oh yep. Fix in #3847. This PR should be good to merge once the fix goes through.

agones-bot · 2024-05-31T18:31:34Z

Build Failed 😱

Build Id: 8a22074e-7196-438f-95db-59623fdf143f

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

markmandel · 2024-05-31T21:35:42Z

@steven-supersolid we can never auto-update your PRs 😃 we're heading into the weekend, so please rebase against main and it'll automerge when passing CI 👍🏻

github-actions · 2024-06-02T08:56:17Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-06-02T09:55:01Z

Build Succeeded 👏

Build Id: e92653c2-feda-4dc6-9564-04fe6bccff2e

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3726/head:pr_3726 && git checkout pr_3726
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.41.0-dev-495abda-amd64

steven-supersolid added 10 commits March 9, 2024 19:15

Add getCounterCount

e2d614d

Add types

276201d

Implement incrementCounter

7f2510c

Implement decrementCounter

ba75ad0

Implement setCounterCount

f7f3308

Implement getCounterCapacity

c98a9bc

Implement setCounterCapacity

1a45bc1

Implement list requests

77e2f33

Fix int64 values

ab4e918

Example usage

2f35140

github-actions bot added kind/feature New features for Agones size/L labels Mar 24, 2024

steven-supersolid changed the title ~~Nodejs.counts and lists~~ Nodejs counters and lists Mar 24, 2024

steven-supersolid added 2 commits May 12, 2024 16:15

Remove boolean return types. Update packages

f3a4992

Update example, fix SDK and tests

b0a1666

Enable standard counts and lists tests

e79c64a

alpha --> beta

e7d5c6c

igooch reviewed May 28, 2024

View reviewed changes

Update nodejs-simple

935b47a

igooch approved these changes May 31, 2024

View reviewed changes

Update build/includes/sdk.mk

ca61c4b

Co-authored-by: igooch <igooch@google.com>

Merge branch 'main' into nodejs.CountsAndLists

441aa48

igooch enabled auto-merge (squash) May 31, 2024 16:59

igooch disabled auto-merge May 31, 2024 18:36

igooch enabled auto-merge (squash) May 31, 2024 18:37

Merge branch 'main' into nodejs.CountsAndLists

495abda

igooch merged commit 8835510 into googleforgames:main Jun 2, 2024
4 checks passed

igooch mentioned this pull request Jun 5, 2024

Update Node.js Page to Reflect that Counters and Lists is Implemented #3857

Open

	if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
	gs.Status.Counters = map[string]*sdk.GameServer_Status_CounterStatus{
	"rooms": {Count: 1, Capacity: 10},
	}
	gs.Status.Lists = map[string]*sdk.GameServer_Status_ListStatus{
	"players": {Values: []string{"test0", "test1", "test2"}, Capacity: 100},
	}
	}

Nodejs counters and lists #3726

Nodejs counters and lists #3726

Conversation

steven-supersolid commented Mar 24, 2024

agones-bot commented Mar 24, 2024

steven-supersolid commented Mar 25, 2024

steven-supersolid commented Mar 25, 2024

igooch commented Mar 25, 2024

steven-supersolid commented Mar 26, 2024

markmandel commented Mar 27, 2024

markmandel commented Mar 27, 2024 • edited

steven-supersolid commented Mar 28, 2024

steven-supersolid commented Mar 28, 2024

markmandel commented Apr 1, 2024

agones-bot commented Apr 1, 2024

markmandel commented Apr 1, 2024

igooch commented Apr 1, 2024 • edited

steven-supersolid commented Apr 2, 2024

igooch commented May 9, 2024

github-actions bot commented May 23, 2024

agones-bot commented May 23, 2024

github-actions bot commented May 23, 2024

agones-bot commented May 23, 2024

steven-supersolid commented May 23, 2024

igooch left a comment

Choose a reason for hiding this comment

igooch May 28, 2024

Choose a reason for hiding this comment

steven-supersolid May 29, 2024

Choose a reason for hiding this comment

igooch May 28, 2024

Choose a reason for hiding this comment

steven-supersolid May 29, 2024

Choose a reason for hiding this comment

igooch May 31, 2024

Choose a reason for hiding this comment

igooch May 28, 2024

Choose a reason for hiding this comment

steven-supersolid commented May 29, 2024

github-actions bot commented May 29, 2024

agones-bot commented May 29, 2024

igooch left a comment

Choose a reason for hiding this comment

igooch May 31, 2024

Choose a reason for hiding this comment

github-actions bot commented May 31, 2024

github-actions bot commented May 31, 2024

agones-bot commented May 31, 2024

steven-supersolid commented May 31, 2024

igooch commented May 31, 2024

agones-bot commented May 31, 2024

markmandel commented May 31, 2024

github-actions bot commented Jun 2, 2024

agones-bot commented Jun 2, 2024

markmandel commented Mar 27, 2024 •

edited

igooch commented Apr 1, 2024 •

edited