New metric - state duration #1468

aLekSer · 2020-04-13T19:28:49Z

Add GameServer state duration metric reusing current function recordGameServerStatusChanges()

aLekSer · 2020-04-13T19:31:35Z

This approach uses current subscription to gsInformer:

	gsInformer.AddEventHandlerWithResyncPeriod(cache.ResourceEventHandlerFuncs{
		UpdateFunc: c.recordGameServerStatusChanges,
	}, 0)

Need to perform more tests on this approach.
LRU cache was used instead of map because it could grow under certain circumstances indefinitely.

agones-bot · 2020-04-13T19:36:29Z

Build Failed 😱

Build Id: 9b0b8dc4-0e0a-46e2-91c9-c514d9286d8c

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

agones-bot · 2020-04-13T20:15:08Z

Build Failed 😱

Build Id: f21ef69c-cbe7-41eb-83e9-5617e7987a7f

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

pkg/metrics/controller.go

pkg/metrics/controller_metrics.go

akremsa · 2020-04-14T10:40:19Z

pkg/metrics/controller_metrics.go

+		&view.View{
+			Name:        "gameserver_state_duration",
+			Measure:     gsStateDuration,
+			Description: "The time gameserver exist in the current state in seconds",


Gameserver state duration\lasting (seconds)

pkg/metrics/controller_test.go

pkg/metrics/controller.go

agones-bot · 2020-04-14T12:07:16Z

Build Failed 😱

Build Id: ed1b20fb-5d6c-41c2-86d5-145e8de25405

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

agones-bot · 2020-04-14T12:43:23Z

Build Succeeded 👏

Build Id: e02053b3-be94-4a44-82c0-ec7b56d5712b

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

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

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

https://6753f40-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1468/head:pr_1468 && git checkout pr_1468
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-6753f40

aLekSer · 2020-04-15T17:04:41Z

This is how new metric could look like in Grafana (title was not updated yet), need to add by (type) to get graph for each state, currently all are presented in one.

Also need to skip some incoming timestamps from evicted keys. Could achieve this by switching to {GameState, time} structure for values and namespace, name for keys in the cache.

aLekSer · 2020-04-15T19:58:30Z

There is an issue with Shutdown state, which seems to be inaccurate.
Two examples of charts for different state durations;

markmandel · 2020-04-20T22:21:52Z

Does this need review of some kind? I generally don't look at PR's in Draft unless specifically asked to, but saw this one seemed to have been sitting here for some time.

aLekSer · 2020-04-21T06:52:49Z

Will update this and my other PRs today. I switched to another task related to Terraform yesterday.
I wanted to add proper Grafana graph for this PR, that's why I got stuck.

agones-bot · 2020-04-21T12:12:40Z

Build Succeeded 👏

Build Id: fc8c0619-27b4-428e-a2de-e26b29c1f08c

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-40b6527
image: gcr.io/agones-images/agones-sdk:1.6.0-40b6527
image: gcr.io/agones-images/agones-ping:1.6.0-40b6527
Linux C++ SDK (build): agonessdk-1.6.0-40b6527-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.6.0-40b6527.zip

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

https://40b6527-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1468/head:pr_1468 && git checkout pr_1468
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-40b6527

aLekSer · 2020-06-08T21:29:07Z

@cyriltovena Cyril, I think I have applied all your comments. Can you please re-review?

agones-bot · 2020-06-08T21:58:14Z

Build Succeeded 👏

Build Id: 407af734-0699-46b6-b79f-79f94f46288f

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1468/head:pr_1468 && git checkout pr_1468
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-f400f53

markmandel · 2020-06-16T17:50:10Z

Just checking in on this. See how it's doing?

aLekSer · 2020-06-16T20:24:02Z

This version was finalised. @cyriltovena one more bump on this.

cyriltovena · 2020-06-16T20:31:08Z

pkg/metrics/controller_metrics.go

@@ -21,6 +21,7 @@ import (
 )

 var (
+	distributionSeconds       = []float64{0, 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384}


nit: I would use a different name or not use a variable.

I think this boundaries only fits your metrics.

Thanks for the update I would rename it to stateDurationSeconds.

cyriltovena

LGTM

google-oss-robot · 2020-06-16T20:31:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, cyriltovena

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~~ [cyriltovena]

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

google-oss-robot · 2020-06-16T20:42:48Z

New changes are detected. LGTM label has been removed.

It needs to switch from map to LRU cache to limit the memory usage.

State in Key could be used to test that we are not skipping some particular State changes.

Rewrited the test.

Refactored test for calcDuration - table tests are used.

Fix linter issues. Now we have namespace/fleet/gameserver/state as a key.

Added description on cache size.

State Duration Metric uses LRU cache to limit the memory usage. * Add LRU cache to calculate time for state changes State in Key could be used to test that we are not skipping some particular State changes. * Move LRU cache to a controller local field * Fixed return parameters for calcDuration Rewrote the test. * Add negative state duration check * Return 0 duration with error Refactored test for calcDuration - table tests are used. * Add documentation for the new metric * Added Fleet to LRU key Fix linter issues. Now we have namespace/fleet/gameserver/state as a key. Added description on cache size. * Add Namespace into metric labels * Rename of StateDuration boundaries

google-oss-robot added the do-not-merge/work-in-progress label Apr 13, 2020

google-oss-robot requested review from markmandel and pooneh-m April 13, 2020 19:28

google-oss-robot added the size/M label Apr 13, 2020

aLekSer force-pushed the metrics/state-duration branch from 5c65fd6 to 41608bd Compare April 13, 2020 20:07

markmandel requested a review from cyriltovena April 13, 2020 20:12

markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Apr 13, 2020