Health check error message #2649

XiaNi · 2022-06-28T14:06:04Z

What type of PR is this?
/kind feature

What this PR does / Why we need it:
it turns faceless error like:

Error: write after end
    at new NodeError (node:internal/errors:371:5)
    at _write (node:internal/streams/writable:319:11)
    at ClientHttp2Stream.Writable.write (node:internal/streams/writable:334:10)
    at Http2CallStream.writeMessageToStream (/home/xiani/work/msa-xrengine/node_modules/@grpc/grpc-js/src/call-stream.ts:499:23)
    at /home/xiani/work/msa-xrengine/node_modules/@grpc/grpc-js/src/call-stream.ts:851:14

to

Error: health ping connection failure: write after end
    at /home/xiani/work/msa-xrengine/packages/instanceserver/src/start.ts:85:15
    at onwriteError (node:internal/streams/writable:415:3)
    at onwrite (node:internal/streams/writable:457:7)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

where developer can see who was trying to write and why.

Which issue(s) this PR fixes:

Special notes for your reviewer:

if for some reason agones server connection fails - `healthStream.write` throws error that I fail to catch but at least we can show source of error

google-cla · 2022-06-28T14:06:11Z

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 · 2022-06-28T14:31:16Z

Build Succeeded 👏

Build Id: 61df732c-6124-4d68-bf6b-1881dce156f3

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-4030481-amd64

agones-bot · 2022-06-28T14:46:20Z

Build Succeeded 👏

Build Id: 1cd024d1-4c22-4dce-8f4f-6ec5010cd787

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-1e701f9-amd64

steven-supersolid · 2022-06-28T16:30:16Z

Nice addition! Could you check coverage with npm run cover and see if it is now reduced from 100%? Then add a test if so. I think we have some existing tests for errors that can be used as a reference

XiaNi · 2022-06-29T11:04:07Z

@steven-supersolid recheck please, if it's ok to make health() function to be able to take optional errorCallback?
I made so to make users be able to catch stream write error if it's needed by providing errorCallback - if it's there - it will be called on error, otherwise error will be thrown as usual.
3 tests cases added to provide 100% coverage.

agones-bot · 2022-06-29T11:23:44Z

Build Succeeded 👏

Build Id: 79ea6279-9377-458f-995f-74c150a9908c

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-1e9bb92-amd64

agones-bot · 2022-06-29T11:30:55Z

Build Failed 😱

Build Id: 4dc8857c-51f1-48b6-815d-0314335b66ec

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

markmandel · 2022-06-29T16:56:27Z

--- FAIL: TestGameServerAllocationDuringGameServerDeletion (0.00s)
    --- FAIL: TestGameServerAllocationDuringGameServerDeletion/scale_down (16.42s)
        fleet_test.go:947: 
            	Error Trace:	fleet_test.go:947
            	            				fleet_test.go:959
            	Error:      	Should NOT be empty, but was []
            	Test:       	TestGameServerAllocationDuringGameServerDeletion/scale_down

e2e feature gates - that's a new one, but looks unrelated to the change at hand.

agones-bot · 2022-06-29T17:35:13Z

Build Succeeded 👏

Build Id: 53b02beb-0ffb-4bfc-9a89-2859f87705b2

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-c442152-amd64

steven-supersolid · 2022-06-29T19:38:43Z

sdks/nodejs/spec/agonesSDK.spec.js

+				return stream;
+			});
+			try {
+				agonesSDK.health(() => {});


It may be worth adding an expect in here to test/demonstrate that the error is passed to the callback intact.
Could spy on the callback and I think and check what it was called with

steven-supersolid · 2022-06-29T19:39:49Z

sdks/nodejs/spec/agonesSDK.spec.js

+				fail();
+			}
+		});
+		it('calls the server and re throws stream write error if no callback', async () => {


nit: For consistency with other tests, perhaps we should add a newline here to separate tests

steven-supersolid · 2022-06-29T19:40:41Z

sdks/nodejs/spec/agonesSDK.spec.js

+				expect(error).toEqual('error');
+			}
+		});
+		it('do not call error callback if there was no stream error', async () => {


nit

Suggested change

it('do not call error callback if there was no stream error', async () => {

it('does not call error callback if there was no stream error', async () => {

steven-supersolid · 2022-06-29T19:46:00Z

sdks/nodejs/src/agonesSDK.js

 		if (this.healthStream === undefined) {
 			this.healthStream = this.client.health(() => {
 				// Ignore error as this can't be caught
 			});
+			if (typeof this.healthStream.on === 'function') {


@XiaNi I'm just wondering a couple of things about this change:

Do we need to listen to the event if we are already passing a callback above? Or is this just to make testing easier?

If so, do we need the typeof check? Generally I would trust gRPC client types not to change unless we intentionally update the client and then would change this code too

If we won't listen to event - there will be uncatchable error thrown even with callback. So now we can control to "catch"
error and pass it to callback or re-throw if no callback.
In real life scenario it 99% will be called or always without or always with callback so i could add that event handler only if callback is there on first call. But i think it will add inconsistency. And then is should be mentioned in docs that first health() call will determine behaviour of it's next calls.

I'v added this check to not break tests where we implement just "write" method. So in tests ".on" is usualy undefined and in real environment it will be always 'function'. So i agree that this type check looks strange. Should i add "on" to stream spyobject of all tests with health() call? (i like this option)

Thanks for the detailed explanation. On point 2 I'd add an 'on' to the stream spy object so we can remove this check

I added 'on' to tests and removed that type check

steven-supersolid · 2022-06-29T20:01:17Z

The callback is a great idea - we may have to update some documentation too though, e.g. in https://github.com/googleforgames/agones/blob/main/site/content/en/docs/Guides/Client%20SDKs/nodejs.md

agones-bot · 2022-06-30T10:32:16Z

Build Succeeded 👏

Build Id: 7a1ea6cb-79eb-4e69-a8f6-e46f7779d5d7

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-439a4ee-amd64

XiaNi · 2022-06-30T11:43:36Z

The callback is a great idea - we may have to update some documentation too though, e.g. in https://github.com/googleforgames/agones/blob/main/site/content/en/docs/Guides/Client%20SDKs/nodejs.md

documentation is updated now too

agones-bot · 2022-06-30T12:04:38Z

Build Succeeded 👏

Build Id: 2a459c70-65d6-431c-8642-ea872efc47c7

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-f2a032e-amd64

+ add 'on' method to stream spy object of 'health()' tests

agones-bot · 2022-07-01T14:27:48Z

Build Succeeded 👏

Build Id: 740969af-0483-42c0-ae77-dc24edc06d55

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-497a039-amd64

agones-bot · 2022-07-01T14:43:01Z

Build Succeeded 👏

Build Id: 4dda535d-8f76-4d34-a9bc-67216b3639c9

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2649/head:pr_2649 && git checkout pr_2649
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-4e8c00b-amd64

google-oss-prow · 2022-07-06T19:09:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, steven-supersolid, XiaNi

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

XiaNi added 2 commits June 28, 2022 16:42

add error callback into nodejs health check method

d0b1368

if for some reason agones server connection fails - `healthStream.write` throws error that I fail to catch but at least we can show source of error

err to error

4030481

google-oss-prow bot added the size/XS label Jun 28, 2022

google-oss-prow bot requested review from dzlier-gcp and steven-supersolid June 28, 2022 14:06

spaces to tabs

1e701f9

roberthbailey added the kind/cleanup Refactoring code, fixing up documentation, etc label Jun 28, 2022

update and tests

1e9bb92

google-oss-prow bot added size/M and removed size/XS labels Jun 29, 2022

Merge branch 'main' into health_check_error_message

7943a1b

Merge branch 'main' into health_check_error_message

c442152

steven-supersolid reviewed Jun 29, 2022

View reviewed changes

XiaNi added 2 commits June 30, 2022 13:05

check that the error is passed to the callback intact

e376839

code style + typos

439a4ee

docs update

f2a032e

XiaNi added 2 commits July 1, 2022 17:02

remove type check of 'stream.on'

497a039

+ add 'on' method to stream spy object of 'health()' tests

Merge branch 'main' into health_check_error_message

4e8c00b

steven-supersolid approved these changes Jul 1, 2022

View reviewed changes

google-oss-prow bot assigned steven-supersolid Jul 1, 2022

google-oss-prow bot added the lgtm label Jul 1, 2022

markmandel approved these changes Jul 6, 2022

View reviewed changes

google-oss-prow bot assigned markmandel Jul 6, 2022

markmandel merged commit 45e39b0 into googleforgames:main Jul 6, 2022

google-oss-prow bot added the approved label Jul 6, 2022

SaitejaTamma added this to the 1.25.0 milestone Jul 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Health check error message #2649

Health check error message #2649

XiaNi commented Jun 28, 2022

google-cla bot commented Jun 28, 2022

agones-bot commented Jun 28, 2022

agones-bot commented Jun 28, 2022

steven-supersolid commented Jun 28, 2022

XiaNi commented Jun 29, 2022

agones-bot commented Jun 29, 2022

agones-bot commented Jun 29, 2022

markmandel commented Jun 29, 2022

agones-bot commented Jun 29, 2022

steven-supersolid Jun 29, 2022

XiaNi Jun 30, 2022

steven-supersolid Jun 29, 2022

XiaNi Jun 30, 2022

steven-supersolid Jun 29, 2022

XiaNi Jun 30, 2022

steven-supersolid Jun 29, 2022

XiaNi Jun 30, 2022

steven-supersolid Jun 30, 2022

XiaNi Jul 1, 2022

steven-supersolid commented Jun 29, 2022

agones-bot commented Jun 30, 2022

XiaNi commented Jun 30, 2022

agones-bot commented Jun 30, 2022

agones-bot commented Jul 1, 2022

agones-bot commented Jul 1, 2022

google-oss-prow bot commented Jul 6, 2022

	it('do not call error callback if there was no stream error', async () => {
	it('does not call error callback if there was no stream error', async () => {

Health check error message #2649

Health check error message #2649

Conversation

XiaNi commented Jun 28, 2022

google-cla bot commented Jun 28, 2022

agones-bot commented Jun 28, 2022

agones-bot commented Jun 28, 2022

steven-supersolid commented Jun 28, 2022

XiaNi commented Jun 29, 2022

agones-bot commented Jun 29, 2022

agones-bot commented Jun 29, 2022

markmandel commented Jun 29, 2022

agones-bot commented Jun 29, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-supersolid commented Jun 29, 2022

agones-bot commented Jun 30, 2022

XiaNi commented Jun 30, 2022

agones-bot commented Jun 30, 2022

agones-bot commented Jul 1, 2022

agones-bot commented Jul 1, 2022

google-oss-prow bot commented Jul 6, 2022