Review feedback.

Adds more tests for parsing labels.
Skip monitored resource if discovery fails.

pkg/metrics/exporter.go

@@ -21,14 +21,13 @@ import (
 
 	"cloud.google.com/go/compute/metadata"
 	"contrib.go.opencensus.io/exporter/stackdriver"
+	"github.com/pkg/errors"
 	prom "github.com/prometheus/client_golang/prometheus"
 	"go.opencensus.io/exporter/prometheus"
 	"go.opencensus.io/stats/view"
 	"google.golang.org/genproto/googleapis/api/monitoredres"
 )
 
-const unknown = "unknown"
-
 // RegisterPrometheusExporter register a prometheus exporter to OpenCensus with a given prometheus metric registry.
 // It will automatically add go runtime and process metrics using default prometheus collectors.
 // The function return an http.handler that you can use to expose the prometheus endpoint.
@@ -54,22 +53,10 @@ func RegisterPrometheusExporter(registry *prom.Registry) (http.Handler, error) {
 // RegisterStackdriverExporter register a Stackdriver exporter to OpenCensus.
 // It will add Agones metrics into Stackdriver on Google Cloud.
 func RegisterStackdriverExporter(projectID string, defaultLabels string) (*stackdriver.Exporter, error) {
-	instanceID, err := metadata.InstanceID()
+	monitoredRes, err := getMonitoredResource(projectID)
 	if err != nil {
-		logger.WithError(err).Warn("error getting instance ID")
-		instanceID = unknown
+		logger.WithError(err).Warn("error discovering monitored resource")
 	}
-	zone, err := metadata.Zone()
-	if err != nil {
-		logger.WithError(err).Warn("error getting zone")
-		zone = unknown
-	}
-	clusterName, err := metadata.InstanceAttributeValue("cluster-name")
-	if err != nil {
-		logger.WithError(err).Warn("error getting cluster-name")
-		clusterName = unknown
-	}
-
 	labels, err := parseLabels(defaultLabels)
 	if err != nil {
 		return nil, err
@@ -78,21 +65,8 @@ func RegisterStackdriverExporter(projectID string, defaultLabels string) (*stack
 	sd, err := stackdriver.NewExporter(stackdriver.Options{
 		ProjectID: projectID,
 		// MetricPrefix helps uniquely identify your metrics.
-		MetricPrefix: "agones",
-		Resource: &monitoredres.MonitoredResource{
-			Type: "k8s_container",
-			Labels: map[string]string{
-				"project_id":   projectID,
-				"instance_id":  instanceID,
-				"zone":         zone,
-				"cluster_name": clusterName,
-
-				// See: https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/
-				"namespace_id":   os.Getenv("POD_NAMESPACE"),
-				"pod_id":         os.Getenv("POD_NAME"),
-				"container_name": os.Getenv("CONTAINER_NAME"),
-			},
-		},
+		MetricPrefix:            "agones",
+		Resource:                monitoredRes,
 		DefaultMonitoringLabels: labels,
 	})
 	if err != nil {
@@ -119,3 +93,33 @@ func SetReportingPeriod(prometheus, stackdriver bool) {
 		view.SetReportingPeriod(reportingPeriod)
 	}
 }
+
+func getMonitoredResource(projectID string) (*monitoredres.MonitoredResource, error) {
+	instanceID, err := metadata.InstanceID()
+	if err != nil {
+		return nil, errors.Wrap(err, "error getting instance ID")
+	}
+	zone, err := metadata.Zone()
+	if err != nil {
+		return nil, errors.Wrap(err, "error getting zone")
+	}
+	clusterName, err := metadata.InstanceAttributeValue("cluster-name")
+	if err != nil {
+		return nil, errors.Wrap(err, "error getting cluster-name")
+	}
+
+	return &monitoredres.MonitoredResource{
+		Type: "k8s_container",
+		Labels: map[string]string{
+			"project_id":   projectID,
+			"instance_id":  instanceID,
+			"zone":         zone,
+			"cluster_name": clusterName,
+
+			// See: https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/
+			"namespace_id":   os.Getenv("POD_NAMESPACE"),
+			"pod_id":         os.Getenv("POD_NAME"),
+			"container_name": os.Getenv("CONTAINER_NAME"),
+		},
+	}, nil
+}

pkg/metrics/util.go

@@ -16,8 +16,10 @@ package metrics
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
+	"unicode/utf8"
 
 	"agones.dev/agones/pkg/util/runtime"
 	"contrib.go.opencensus.io/exporter/stackdriver"
@@ -58,15 +60,20 @@ func parseLabels(s string) (*stackdriver.Labels, error) {
 		return res, nil
 	}
 	pairs := strings.Split(s, ",")
-	if len(pairs) == 0 {
-		return res, nil
-	}
 	for _, p := range pairs {
 		keyValue := strings.Split(p, "=")
 		if len(keyValue) != 2 {
-			return nil, fmt.Errorf("invalid labels format: %s, expect key=value,key2=value2", s)
+			return nil, fmt.Errorf("invalid labels: %s, expect key=value,key2=value2", s)
+		}
+		key := strings.TrimSpace(keyValue[0])
+		value := strings.TrimSpace(keyValue[1])
+		if !utf8.ValidString(key) || !utf8.ValidString(value) {
+			return nil, errors.New("invalid labels: must be a valid utf-8 string")
+		}
+		if key == "" || value == "" {
+			return nil, errors.New("invalid labels: must not be empty string")
 		}
-		res.Set(keyValue[0], keyValue[1], "")
+		res.Set(key, value, "")
 	}
 	return res, nil
 }

pkg/metrics/util_test.go

@@ -300,31 +300,23 @@ func Test_parseLabels(t *testing.T) {
 		want    *stackdriver.Labels
 		wantErr bool
 	}{
-		{
-			"",
-			labelsFromMap(nil),
-			false,
-		},
-		{
-			"a=b",
-			labelsFromMap(map[string]string{"a": "b"}),
-			false,
-		},
-		{
-			"a=b,",
-			nil,
-			true,
-		},
-		{
-			"a=b,c",
-			nil,
-			true,
-		},
-		{
-			"a=b,c=d",
-			labelsFromMap(map[string]string{"a": "b", "c": "d"}),
-			false,
-		},
+		// valids
+		{"", labelsFromMap(nil), false},
+		{"a=b", labelsFromMap(map[string]string{"a": "b"}), false},
+		{"a=b,c=d", labelsFromMap(map[string]string{"a": "b", "c": "d"}), false},
+		{"a=b, c=d", labelsFromMap(map[string]string{"a": "b", "c": "d"}), false},
+		{"a=b , c = d ", labelsFromMap(map[string]string{"a": "b", "c": "d"}), false},
+		{" a = b , c = d ", labelsFromMap(map[string]string{"a": "b", "c": "d"}), false},
+		{" a = b , c = d,c=f ", labelsFromMap(map[string]string{"a": "b", "c": "f"}), false},
+
+		// errors
+		{"e", nil, true},
+		{"a=b,", nil, true},
+		{"a= =,", nil, true},
+		{"a=b,c", nil, true},
+		{"a=b,c =", nil, true},
+		{"a=b , c ==", nil, true},
+		{"a=b , c =\xc3\x28", nil, true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.input, func(t *testing.T) {

site/content/en/docs/Installation/helm.md

@@ -36,7 +36,11 @@ _We recommend to install Agones in its own namespaces (like `agones-system` as s
 you can use the helm `--namespace` parameter to specify a different namespace._
 
 When running in production, Agones should be scheduled on a dedicated pool of nodes, distinct from where Game Servers are scheduled for better isolation and resiliency. By default Agones prefers to be scheduled on nodes labeled with `agones.dev/agones-system=true` and tolerates node taint `agones.dev/agones-system=true:NoExecute`. If no dedicated nodes are available, Agones will
+<<<<<<< HEAD
 run on regular nodes, but that's not recommended for production use. For instructions on setting up a dedicated node
+=======
+run on regular nodes, but that's not recommended for production use. For instructions on setting up a decidated node
+>>>>>>> Review feedback.
 pool for Agones, see the [Agones installation instructions]({{< relref "../_index.md" >}}) for your preferred environment.
 
 The command deploys Agones on the Kubernetes cluster with the default configuration. The [configuration](#configuration) section lists the parameters that can be configured during installation.
@@ -115,7 +119,10 @@ The following tables lists the configurable parameters of the Agones chart and t
 | `agones.metrics.prometheusServiceDiscovery`         | Adds annotations for Prometheus ServiceDiscovery (and also Strackdriver)                        | `true`                 |
 | `agones.metrics.prometheusEnabled`                  | Enables controller metrics on port `8080` and path `/metrics`                                   | `true`                 |
 | `agones.metrics.stackdriverEnabled`                 | Enables Stackdriver exporter of controller metrics                                              | `false`                |
-| `agones.metrics.stackdriverProjectID`               | This overrides the default gcp project id for use with Stackdriver                              | ``                     |
+| `agones.metrics.stackdriverProjectID`               | This overrides the default gcp project id for use with stackdriver                              | ``                     |
+{{% feature publishVersion="1.3.0" %}}
+| `agones.metrics.stackdriverLabels`                  | A set of default labels to add to all stackdriver metrics generated in form of key value pair (`key=value,key2=value2`). By default metadata are automatically added using Kubernetes API and GCP metadata enpoint.                              | ``                     |
+{{% /feature %}}
 | `agones.serviceaccount.controller`                  | Service account name for the controller                                                         | `agones-controller`    |
 | `agones.serviceaccount.sdk`                         | Service account name for the sdk                                                                | `agones-sdk`           |
 | `agones.image.registry`                             | Global image registry for all images                                                            | `gcr.io/agones-images` |