Agones: gRPC allocate can't get some metrics

[8398a7]

What happened:

I made an allocate request according to https://github.com/googleforgames/agones/blob/release-1.9.0/examples/allocator-client/main.go.
I am also enabling prometheus metrics.

I looked at the grafana dashboard, the Allocations dashboard was not displaying correctly.
refs: https://github.com/googleforgames/agones/blob/master/build/grafana/dashboard-allocations.yaml

What you expected to happen:

The grafana dashboard must be displayed correctly.

How to reproduce it (as minimally and precisely as possible):

Execution of https://github.com/googleforgames/agones/blob/release-1.9.0/examples/allocator-client/main.go

Anything else we need to know?:

This appears to be a failure to respond.
https://github.com/googleforgames/agones/blob/master/pkg/gameserverallocations/controller.go#L111-L117

I've written a patch and will create a PR if it looks good to make one.

diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go
index 3b09cb63..e0dd1d08 100644
--- a/pkg/gameserverallocations/allocator.go
+++ b/pkg/gameserverallocations/allocator.go
@@ -37,6 +37,7 @@ import (
    "agones.dev/agones/pkg/util/runtime"
    "github.com/pkg/errors"
    "github.com/sirupsen/logrus"
+   "go.opencensus.io/tag"
    "google.golang.org/grpc"
    "google.golang.org/grpc/codes"
    "google.golang.org/grpc/credentials"
@@ -180,7 +181,17 @@ func (c *Allocator) Sync(stop <-chan struct{}) error {
 }

 // Allocate CRDHandler for allocating a gameserver.
-func (c *Allocator) Allocate(gsa *allocationv1.GameServerAllocation, stop <-chan struct{}) (k8sruntime.Object, error) {
+func (c *Allocator) Allocate(gsa *allocationv1.GameServerAllocation, stop <-chan struct{}) (out k8sruntime.Object, err error) {
+   ctx := context.Background()
+   latency := c.newMetrics(ctx)
+   defer func() {
+       if err != nil {
+           latency.setError()
+       }
+       latency.record()
+   }()
+   latency.setRequest(gsa)
+
    // server side validation
    if causes, ok := gsa.Validate(); !ok {
        s := &metav1.Status{
@@ -207,8 +218,6 @@ func (c *Allocator) Allocate(gsa *allocationv1.GameServerAllocation, stop <-chan
    }

    // If multi-cluster setting is enabled, allocate base on the multicluster allocation policy.
-   var out *allocationv1.GameServerAllocation
-   var err error
    if gsa.Spec.MultiClusterSetting.Enabled {
        out, err = c.applyMultiClusterAllocation(gsa, stop)
    } else {
@@ -219,6 +228,7 @@ func (c *Allocator) Allocate(gsa *allocationv1.GameServerAllocation, stop <-chan
        c.loggerForGameServerAllocation(gsa).WithError(err).Error("allocation failed")
        return nil, err
    }
+   latency.setResponse(out)

    return out, nil
 }
@@ -600,3 +610,17 @@ func addPort(ip string) string {
    }
    return fmt.Sprintf("%s:%s", ip, allocatorPort)
 }
+
+// newMetrics creates a new gsa latency recorder.
+func (c *Allocator) newMetrics(ctx context.Context) *metrics {
+   ctx, err := tag.New(ctx, latencyTags...)
+   if err != nil {
+       c.baseLogger.WithError(err).Warn("failed to tag latency recorder.")
+   }
+   return &metrics{
+       ctx:              ctx,
+       gameServerLister: c.readyGameServerCache.gameServerLister,
+       logger:           c.baseLogger,
+       start:            time.Now(),
+   }
+}
diff --git a/pkg/gameserverallocations/controller.go b/pkg/gameserverallocations/controller.go
index 13cf5ba0..7c01a22f 100644
--- a/pkg/gameserverallocations/controller.go
+++ b/pkg/gameserverallocations/controller.go
@@ -15,23 +15,13 @@
 package gameserverallocations

 import (
-   "context"
    "io/ioutil"
    "mime"
    "net/http"
-   "time"

-   allocationv1 "agones.dev/agones/pkg/apis/allocation/v1"
-   "agones.dev/agones/pkg/client/clientset/versioned"
-   "agones.dev/agones/pkg/client/informers/externalversions"
-   "agones.dev/agones/pkg/gameservers"
-   "agones.dev/agones/pkg/util/apiserver"
-   "agones.dev/agones/pkg/util/https"
-   "agones.dev/agones/pkg/util/runtime"
    "github.com/heptiolabs/healthcheck"
    "github.com/pkg/errors"
    "github.com/sirupsen/logrus"
-   "go.opencensus.io/tag"
    corev1 "k8s.io/api/core/v1"
    metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
    k8sruntime "k8s.io/apimachinery/pkg/runtime"
@@ -41,6 +31,14 @@ import (
    "k8s.io/client-go/kubernetes/scheme"
    typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
    "k8s.io/client-go/tools/record"
+
+   allocationv1 "agones.dev/agones/pkg/apis/allocation/v1"
+   "agones.dev/agones/pkg/client/clientset/versioned"
+   "agones.dev/agones/pkg/client/informers/externalversions"
+   "agones.dev/agones/pkg/gameservers"
+   "agones.dev/agones/pkg/util/apiserver"
+   "agones.dev/agones/pkg/util/https"
+   "agones.dev/agones/pkg/util/runtime"
 )

 // Controller is a the GameServerAllocation controller
@@ -108,14 +106,6 @@ func (c *Controller) Run(_ int, stop <-chan struct{}) error {
 }

 func (c *Controller) processAllocationRequest(w http.ResponseWriter, r *http.Request, namespace string, stop <-chan struct{}) (err error) {
-   latency := c.newMetrics(r.Context())
-   defer func() {
-       if err != nil {
-           latency.setError()
-       }
-       latency.record()
-   }()
-
    if r.Body != nil {
        defer r.Body.Close() // nolint: errcheck
    }
@@ -125,7 +115,6 @@ func (c *Controller) processAllocationRequest(w http.ResponseWriter, r *http.Req
    if r.Method != http.MethodPost {
        log.Warn("allocation handler only supports POST")
        http.Error(w, "Method not supported", http.StatusMethodNotAllowed)
-       latency.setError()
        return
    }

@@ -134,7 +123,6 @@ func (c *Controller) processAllocationRequest(w http.ResponseWriter, r *http.Req
        return err
    }

-   latency.setRequest(gsa)

    result, err := c.allocator.Allocate(gsa, stop)
    if err != nil {
@@ -144,25 +132,10 @@ func (c *Controller) processAllocationRequest(w http.ResponseWriter, r *http.Req
        w.WriteHeader(int(status.Code))
    }

-   latency.setResponse(result)
    err = c.serialisation(r, w, result, scheme.Codecs)
    return err
 }

-// newMetrics creates a new gsa latency recorder.
-func (c *Controller) newMetrics(ctx context.Context) *metrics {
-   ctx, err := tag.New(ctx, latencyTags...)
-   if err != nil {
-       c.baseLogger.WithError(err).Warn("failed to tag latency recorder.")
-   }
-   return &metrics{
-       ctx:              ctx,
-       gameServerLister: c.allocator.readyGameServerCache.gameServerLister,
-       logger:           c.baseLogger,
-       start:            time.Now(),
-   }
-}
-
 // allocationDeserialization processes the request and namespace, and attempts to deserialise its values
 // into a GameServerAllocation. Returns an error if it fails for whatever reason.
 func (c *Controller) allocationDeserialization(r *http.Request, namespace string) (*allocationv1.GameServerAllocation, error) {

Environment:

• Agones version: 1.9.0
• Kubernetes version (use kubectl version): 1.16
• Cloud provider or hardware configuration: GKE
• Install method (yaml/helm): helm
• Troubleshooting guide log(s):
• Others:

[8398a7]

I'm wondering if this patch is a good idea because the latency measurement points change slightly.

[markmandel]

Sorry I didn't respond to this earlier. The usual path is to write a PR so it can be reviewed within the PR system.

@pooneh-m you are most familiar with this part of the system, does the general gist of this seem correct to you?

[pooneh-m]

Thanks for the issue and suggestion @8398a7. Can you please send the PR with the fix?