More review comments & CI appeasement

dannykopping · dannykopping · commit 92be1d608ab3 · 2024-03-11T10:17:41.000+02:00
Signed-off-by: Danny Kopping &lt;danny@coder.com&gt;
diff --git a/coderd/agentmetrics/labels.go b/coderd/agentmetrics/labels.go
@@ -1,5 +1,11 @@
 package agentmetrics
 
+import (
+	"strings"
+
+	"golang.org/x/xerrors"
+)
+
 const (
 	LabelAgentName     = "agent_name"
 	LabelUsername      = "username"
@@ -11,3 +17,22 @@ var (
 	LabelAll        = []string{LabelAgentName, LabelTemplateName, LabelUsername, LabelWorkspaceName}
 	LabelAgentStats = []string{LabelAgentName, LabelUsername, LabelWorkspaceName}
 )
+
+// ValidateAggregationLabels ensures a given set of labels are valid aggregation labels.
+func ValidateAggregationLabels(labels []string) error {
+	acceptable := LabelAll
+
+	seen := make(map[string]any, len(acceptable))
+	for _, label := range acceptable {
+		seen[label] = nil
+	}
+
+	for _, label := range labels {
+		if _, found := seen[label]; !found {
+			return xerrors.Errorf("%q is not a valid aggregation label; only one or more of %q are acceptable",
+				label, strings.Join(acceptable, ", "))
+		}
+	}
+
+	return nil
+}
diff --git a/coderd/agentmetrics/labels_test.go b/coderd/agentmetrics/labels_test.go
@@ -0,0 +1,48 @@
+package agentmetrics_test
+
+import (
+	"testing"
+
+	"github.com/coder/coder/v2/coderd/agentmetrics"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateAggregationLabels(t *testing.T) {
+	tests := []struct {
+		name        string
+		labels      []string
+		expectedErr bool
+	}{
+		{
+			name: "empty list is valid",
+		},
+		{
+			name:   "single valid entry",
+			labels: []string{agentmetrics.LabelTemplateName},
+		},
+		{
+			name:   "multiple valid entries",
+			labels: []string{agentmetrics.LabelTemplateName, agentmetrics.LabelUsername},
+		},
+		{
+			name:   "repeated valid entries are not invalid",
+			labels: []string{agentmetrics.LabelTemplateName, agentmetrics.LabelUsername, agentmetrics.LabelUsername, agentmetrics.LabelUsername},
+		},
+		{
+			name:        "empty entry is invalid",
+			labels:      []string{""},
+			expectedErr: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := agentmetrics.ValidateAggregationLabels(tc.labels)
+			if tc.expectedErr {
+				require.Error(t, err)
+			}
+		})
+	}
+}
diff --git a/coderd/prometheusmetrics/aggregator.go b/coderd/prometheusmetrics/aggregator.go
@@ -335,6 +335,10 @@ func (ma *MetricsAggregator) Run(ctx context.Context) func() {
 				var input []annotatedMetric
 				output := make([]prometheus.Metric, 0, len(ma.store))
 
+				if len(ma.aggregateByLabels) == 0 {
+					ma.aggregateByLabels = agentmetrics.LabelAll
+				}
+
 				// If custom aggregation labels have not been chosen, generate Prometheus metrics without any pre-aggregation.
 				// This results in higher cardinality, but may be desirable in larger deployments.
 				//
diff --git a/coderd/prometheusmetrics/aggregator_test.go b/coderd/prometheusmetrics/aggregator_test.go
@@ -561,6 +561,23 @@ func TestLabelsAggregation(t *testing.T) {
 			// Nothing will be returned.
 			expected: []*agentproto.Stats_Metric{},
 		},
+		{
+			// Scenario: validation fails and an empty list is given for aggregation.
+			name:        "empty label aggregation list",
+			aggregateOn: []string{},
+			given: []statCollection{
+				{
+					labels: testLabels,
+					metrics: []*agentproto.Stats_Metric{
+						{Name: "user_counter", Type: agentproto.Stats_Metric_COUNTER, Value: 1},
+					},
+				},
+			},
+			// Default aggregation will be used.
+			expected: []*agentproto.Stats_Metric{
+				{Name: "user_counter", Type: agentproto.Stats_Metric_COUNTER, Value: 1, Labels: commonLabels},
+			},
+		},
 	}
 
 	for _, tc := range tests {
diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -956,19 +956,7 @@ when required by your organization's security policy.`,
 					return nil
 				}
 
-				acceptable := make(map[string]any, len(agentmetrics.LabelAll))
-				for _, label := range agentmetrics.LabelAll {
-					acceptable[label] = nil
-				}
-
-				for _, label := range value.Value() {
-					if _, found := acceptable[label]; !found {
-						return xerrors.Errorf("%q is not a valid aggregation label; only one or more of %q are acceptable",
-							label, strings.Join(agentmetrics.LabelAll, ", "))
-					}
-				}
-
-				return nil
+				return agentmetrics.ValidateAggregationLabels(value.Value())
 			}),
 			Group:   &deploymentGroupIntrospectionPrometheus,
 			YAML:    "aggregate_agent_stats_by",
diff --git a/docs/cli/server.md b/docs/cli/server.md
