From ae192b3e5de474c459fbf7f712be9a5345dfe6d0 Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Sun, 8 Dec 2019 15:46:12 -0500 Subject: [PATCH] Review feedback. Adds more tests for parsing labels. Skip monitored resource if discovery fails. --- pkg/metrics/exporter.go | 66 ++++++++++++----------- pkg/metrics/util.go | 17 ++++-- pkg/metrics/util_test.go | 42 ++++++--------- site/content/en/docs/Installation/helm.md | 4 +- 4 files changed, 67 insertions(+), 62 deletions(-) diff --git a/pkg/metrics/exporter.go b/pkg/metrics/exporter.go index 14e7c9aa30..3a6d50784d 100644 --- a/pkg/metrics/exporter.go +++ b/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 +} diff --git a/pkg/metrics/util.go b/pkg/metrics/util.go index fc70b343ac..189eace637 100644 --- a/pkg/metrics/util.go +++ b/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 } diff --git a/pkg/metrics/util_test.go b/pkg/metrics/util_test.go index 420688273f..43e772e942 100644 --- a/pkg/metrics/util_test.go +++ b/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) { diff --git a/site/content/en/docs/Installation/helm.md b/site/content/en/docs/Installation/helm.md index 386306fbaa..a2d8b36513 100644 --- a/site/content/en/docs/Installation/helm.md +++ b/site/content/en/docs/Installation/helm.md @@ -30,7 +30,7 @@ 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 run on regular nodes, but that's not recommended for production use. For instructions on setting up a decidated node -pool for Agones, see the [Agones installation instructions]({{< relref "../_index.md" >}}) for your preferred environment. +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. @@ -104,7 +104,9 @@ The following tables lists the configurable parameters of the Agones chart and t | `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 | `` | +{{% 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` |