Review comments, expanding tests

Signed-off-by: Danny Kopping <danny@coder.com>
coder · dannykopping · Mar 19, 2024 · Mar 19, 2024 · Mar 19, 2024 · Mar 19, 2024
commit 11abaa6ecf9c6f09367748ae4119c2dfda30db63
diff --git a/cli/server.go b/cli/server.go
@@ -894,7 +894,12 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
 				}
 				defer closeAgentsFunc()
 
-				if err = prometheusmetrics.Experiments(logger, options.PrometheusRegistry, options.DeploymentValues.Experiments.Value(), codersdk.ExperimentsAll); err != nil {
+				var active codersdk.Experiments
+				for _, exp := range options.DeploymentValues.Experiments.Value() {
+					active = append(active, codersdk.Experiment(exp))
+				}
+
+				if err = prometheusmetrics.Experiments(options.PrometheusRegistry, active); err != nil {
 					return xerrors.Errorf("register experiments metric: %w", err)
 				}
 			}

diff --git a/coderd/prometheusmetrics/prometheusmetrics.go b/coderd/prometheusmetrics/prometheusmetrics.go
@@ -517,7 +517,7 @@ func AgentStats(ctx context.Context, logger slog.Logger, registerer prometheus.R
 }
 
 // Experiments registers a metric which indicates whether each experiment is enabled or not.
-func Experiments(_ slog.Logger, registerer prometheus.Registerer, exps []string, all codersdk.Experiments) error {
+func Experiments(registerer prometheus.Registerer, active codersdk.Experiments) error {
 	experimentsGauge := prometheus.NewGaugeVec(prometheus.GaugeOpts{
 		Namespace: "coderd",
 		Name:      "experiments",
@@ -527,10 +527,10 @@ func Experiments(_ slog.Logger, registerer prometheus.Registerer, exps []string,
 		return err
 	}
 
-	for _, exp := range all {
+	for _, exp := range codersdk.ExperimentsAll {
 		var val float64
-		for _, enabled := range exps {
-			if string(exp) == enabled {
+		for _, enabled := range active {
+			if exp == enabled {
 				val = 1
 				break
 			}

diff --git a/coderd/prometheusmetrics/prometheusmetrics_test.go b/coderd/prometheusmetrics/prometheusmetrics_test.go
@@ -503,37 +503,82 @@ func TestAgentStats(t *testing.T) {
 func TestExperimentsMetric(t *testing.T) {
 	t.Parallel()
 
-	log := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
-	reg := prometheus.NewRegistry()
+	tests := []struct {
+		name        string
+		experiments codersdk.Experiments
+		expected    map[codersdk.Experiment]float64
+	}{
+		{
+			name:        "Enabled experiment is exported in metrics",
+			experiments: codersdk.Experiments{codersdk.ExperimentSharedPorts},
+			expected: map[codersdk.Experiment]float64{
+				codersdk.ExperimentSharedPorts: 1,
+			},
+		},
+		{
+			name:        "Disabled experiment is exported in metrics",
+			experiments: codersdk.Experiments{},
+			expected: map[codersdk.Experiment]float64{
+				codersdk.ExperimentSharedPorts: 0,
+			},
+		},
+		{
+			name:        "Unknown experiment is not exported in metrics",
+			experiments: codersdk.Experiments{codersdk.Experiment("bob")},
+			expected:    map[codersdk.Experiment]float64{},
+		},
+	}
 
-	const (
-		a codersdk.Experiment = "a"
-		b codersdk.Experiment = "b"
-		c codersdk.Experiment = "c"
-	)
-	allExps := codersdk.Experiments{a, b, c}
-	require.NoError(t, prometheusmetrics.Experiments(log, reg, []string{string(b), string(c)}, allExps))
+	for _, tc := range tests {
+		tc := tc
 
-	expectation := map[codersdk.Experiment]float64{
-		a: 0,
-		b: 1,
-		c: 1,
-	}
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			reg := prometheus.NewRegistry()
 
-	out, err := reg.Gather()
-	require.NoError(t, err)
-	require.Lenf(t, out, 1, "unexpected number of registered metrics")
+			require.NoError(t, prometheusmetrics.Experiments(reg, tc.experiments))
+
+			out, err := reg.Gather()
+			require.NoError(t, err)
+			require.Lenf(t, out, 1, "unexpected number of registered metrics")
+
+			seen := make(map[codersdk.Experiment]float64)
+
+			for _, metric := range out[0].GetMetric() {
+				require.Equal(t, "coderd_experiments", out[0].GetName())
+
+				labels := metric.GetLabel()
+				require.Lenf(t, labels, 1, "unexpected number of labels")
 
-	for _, metric := range out[0].GetMetric() {
-		require.Equal(t, "coderd_experiments", out[0].GetName())
+				experiment := codersdk.Experiment(labels[0].GetValue())
+				value := metric.GetGauge().GetValue()
 
-		labels := metric.GetLabel()
-		require.Lenf(t, labels, 1, "unexpected number of labels")
+				seen[experiment] = value
 
-		experiment := labels[0].GetValue()
-		expected, found := expectation[codersdk.Experiment(experiment)]
-		require.Truef(t, found, "did not find experiment %q in expectations", experiment)
-		require.EqualValues(t, expected, metric.GetGauge().GetValue())
+				expectedValue := 0
+
+				// Find experiment we expect to be enabled.
+				for _, exp := range tc.experiments {
+					if experiment == exp {
+						expectedValue = 1
+						break
+					}
+				}
+
+				require.EqualValuesf(t, expectedValue, value, "expected %d value for experiment %q", expectedValue, experiment)
+			}
+
+			// We don't want to define the state of all experiments because codersdk.ExperimentAll will change at some
+			// point and break these tests; so we only validate the experiments we know about.
+			for exp, val := range seen {
+				expectedVal, found := tc.expected[exp]
+				if !found {
+					t.Logf("ignoring experiment %q; it is not listed in expectations", exp)
+					continue
+				}
+				require.Equalf(t, expectedVal, val, "experiment %q did not match expected value %v", exp, expectedVal)
+			}
+		})
 	}
 }