metrics: fix application_instances_per_channel disagreeing with UI instance counts

backend/pkg/api/internal/dbreads/metrics.go

@@ -8,13 +8,25 @@ import (
 )
 
 var (
+	// appInstancesPerChannelMetricSQL only counts instances that are still
+	// considered "active" (i.e. within validityInterval of their last
+	// check-in), matching the activity window used everywhere else
+	// (GetApp, GetApps, GetGroupInstancesStats, GetGroupVersionBreakdown,
+	// GetInstances). It uses LEFT JOINs for groups/channel so instances
+	// whose group has no channel assigned (groups.channel_id can be NULL)
+	// are still counted instead of being silently dropped, and it also
+	// reports the channel's arch so that amd64/arm64 channels sharing the
+	// same name are not folded into a single row.
 	appInstancesPerChannelMetricSQL = fmt.Sprintf(`
-SELECT a.name AS app_name, ia.version AS version, c.name AS channel_name, count(ia.version) AS instances_count
-FROM instance_application ia, application a, channel c, groups g
-WHERE a.id = ia.application_id AND ia.group_id = g.id AND g.channel_id = c.id AND %s
-GROUP BY app_name, version, channel_name
-ORDER BY app_name, version, channel_name
-`, ignoreFakeInstanceCondition("ia.instance_id"))
+SELECT a.name AS app_name, ia.version AS version, COALESCE(c.name, '') AS channel_name, COALESCE(c.arch, -1) AS arch, count(ia.version) AS instances_count
+FROM instance_application ia
+JOIN application a ON a.id = ia.application_id
+LEFT JOIN groups g ON ia.group_id = g.id
+LEFT JOIN channel c ON g.channel_id = c.id
+WHERE ia.last_check_for_updates > now() at time zone 'utc' - interval '%[1]s' AND %[2]s
+GROUP BY app_name, version, channel_name, arch
+ORDER BY app_name, version, channel_name, arch
+`, validityInterval, ignoreFakeInstanceCondition("ia.instance_id"))
 
 	failedUpdatesSQL = fmt.Sprintf(`
 SELECT a.name AS app_name, count(*) as fail_count

backend/pkg/api/metrics_test.go

@@ -2,12 +2,17 @@ package api
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 
 	"github.com/flatcar/nebraska/backend/pkg/api/types"
 )
 
+// sampleAppArch is the arch of every channel created by sample_data.sql for
+// "Sample application" (see migration 0007_add_package_arch.sql).
+const sampleAppArch = int(types.ArchAMD64)
+
 func TestGetAppInstancesPerChannelMetrics(t *testing.T) {
 	a := newForTest(t)
 	defer a.Close()
@@ -20,55 +25,175 @@ func TestGetAppInstancesPerChannelMetrics(t *testing.T) {
 			ApplicationName: "Sample application",
 			Version:         "1.0.1",
 			ChannelName:     "Failing",
+			Arch:            sampleAppArch,
 			InstancesCount:  1,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.1",
 			ChannelName:     "Master",
+			Arch:            sampleAppArch,
 			InstancesCount:  1,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.1",
 			ChannelName:     "Stable",
+			Arch:            sampleAppArch,
 			InstancesCount:  1,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.2",
 			ChannelName:     "Master",
+			Arch:            sampleAppArch,
 			InstancesCount:  1,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.2",
 			ChannelName:     "Stable",
+			Arch:            sampleAppArch,
 			InstancesCount:  2,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.3",
 			ChannelName:     "Master",
+			Arch:            sampleAppArch,
 			InstancesCount:  1,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.3",
 			ChannelName:     "Stable",
+			Arch:            sampleAppArch,
 			InstancesCount:  4,
 		},
 		{
 			ApplicationName: "Sample application",
 			Version:         "1.0.4",
 			ChannelName:     "Master",
+			Arch:            sampleAppArch,
 			InstancesCount:  1,
 		},
 	}
 
 	require.Equal(t, expectedMetrics, metrics)
 }
 
+// TestGetAppInstancesPerChannelMetricsExcludesStaleInstances checks that an
+// instance is dropped from the metric once it stops checking in, matching
+// the activity window (validityInterval) used by GetApp/GetApps/GetInstances
+// and friends. Regression test for
+// https://github.com/flatcar/nebraska/issues/1562.
+func TestGetAppInstancesPerChannelMetricsExcludesStaleInstances(t *testing.T) {
+	a := newForTest(t)
+	defer a.Close()
+
+	// Make every sample instance look like it hasn't checked in for 400 days.
+	longAgo := time.Now().UTC().AddDate(0, 0, -400)
+	_, err := a.db().Exec(`UPDATE instance_application SET last_check_for_updates = $1`, longAgo)
+	require.NoError(t, err)
+
+	metrics, err := a.GetAppInstancesPerChannelMetrics()
+	require.NoError(t, err)
+	require.Empty(t, metrics, "instances that stopped checking in long ago must not be counted")
+}
+
+// TestGetAppInstancesPerChannelMetricsIncludesGroupsWithoutChannel checks
+// that instances belonging to a group with no channel assigned
+// (groups.channel_id can be NULL, e.g. after the channel was deleted) are
+// still counted instead of being silently dropped by the join. Regression
+// test for https://github.com/flatcar/nebraska/issues/1562.
+func TestGetAppInstancesPerChannelMetricsIncludesGroupsWithoutChannel(t *testing.T) {
+	a := newForTest(t)
+	defer a.Close()
+
+	// "Prod EC2 us-west-2" (bcaa68bc-...) has 3 active instances pointed at
+	// the "Stable" channel: instance1 (1.0.3), instance2 (1.0.3), instance3 (1.0.2).
+	_, err := a.db().Exec(`UPDATE groups SET channel_id = NULL WHERE id = 'bcaa68bc-5f82-11e5-9d70-feff819cdc9f'`)
+	require.NoError(t, err)
+
+	metrics, err := a.GetAppInstancesPerChannelMetrics()
+	require.NoError(t, err)
+
+	var totalInstances int
+	var noChannel103, noChannel102 int
+	for _, m := range metrics {
+		totalInstances += m.InstancesCount
+		if m.ChannelName == "" {
+			require.Equal(t, -1, m.Arch, "instances without a channel must not report a real arch")
+			switch m.Version {
+			case "1.0.3":
+				noChannel103 = m.InstancesCount
+			case "1.0.2":
+				noChannel102 = m.InstancesCount
+			}
+		}
+	}
+
+	require.Equal(t, 12, totalInstances, "all 12 active sample instances must still be counted, channel or not")
+	require.Equal(t, 2, noChannel103, "instance1 and instance2 should fall into the no-channel bucket")
+	require.Equal(t, 1, noChannel102, "instance3 should fall into the no-channel bucket")
+}
+
+// TestGetAppInstancesPerChannelMetricsSeparatesArch checks that two channels
+// that share a name but differ by arch (e.g. "stable" for amd64 and arm64,
+// see migration 0008-arm-channels-groups.sql) are reported as separate
+// rows instead of being folded together by GROUP BY channel_name. Regression
+// test for https://github.com/flatcar/nebraska/issues/1562.
+func TestGetAppInstancesPerChannelMetricsSeparatesArch(t *testing.T) {
+	a := newForTest(t)
+	defer a.Close()
+
+	const (
+		appID         = "b6458005-8f40-4627-b33b-be70a718c48e" // "Sample application"
+		packageID     = "12697fa4-5f83-11e5-9d70-feff819cdc9f" // existing "Stable" package for appID
+		archChannelID = "cb2deea8-5f83-11e5-9d70-feff819cdc9e"
+		archGroupID   = "bcaa68bc-5f82-11e5-9d70-feff819cda00"
+	)
+
+	// A second "Stable" channel for the same application, but for arm64.
+	_, err := a.db().Exec(`
+		INSERT INTO channel (id, name, color, application_id, package_id, arch)
+		VALUES ($1, 'Stable', '#0099FF', $2, $3, $4)`,
+		archChannelID, appID, packageID, int(types.ArchAArch64))
+	require.NoError(t, err)
+
+	_, err = a.db().Exec(`
+		INSERT INTO groups (id, name, description, policy_period_interval, policy_max_updates_per_period, policy_update_timeout, application_id, channel_id, track)
+		VALUES ($1, 'Prod EC2 us-west-2 (arm64)', 'Production servers, west coast, arm64', '15 minutes', 2, '60 minutes', $2, $3, $4)`,
+		archGroupID, appID, archChannelID, archGroupID)
+	require.NoError(t, err)
+
+	_, err = a.db().Exec(`INSERT INTO instance (id, ip) VALUES ('instance-arm1', '10.0.0.100')`)
+	require.NoError(t, err)
+
+	_, err = a.db().Exec(`
+		INSERT INTO instance_application (version, instance_id, application_id, group_id)
+		VALUES ('1.0.3', 'instance-arm1', $1, $2)`, appID, archGroupID)
+	require.NoError(t, err)
+
+	metrics, err := a.GetAppInstancesPerChannelMetrics()
+	require.NoError(t, err)
+
+	var amd64Stable103, arm64Stable103 int
+	for _, m := range metrics {
+		if m.ApplicationName == "Sample application" && m.ChannelName == "Stable" && m.Version == "1.0.3" {
+			switch m.Arch {
+			case int(types.ArchAMD64):
+				amd64Stable103 = m.InstancesCount
+			case int(types.ArchAArch64):
+				arm64Stable103 = m.InstancesCount
+			}
+		}
+	}
+
+	require.Equal(t, 4, amd64Stable103, "existing amd64 Stable/1.0.3 count must be unaffected")
+	require.Equal(t, 1, arm64Stable103, "the new arm64 instance must show up as its own series")
+}
+
 func TestGetFailedUpdatesMetrics(t *testing.T) {
 	a := newForTest(t)
 	defer a.Close()

backend/pkg/api/types/metrics.go

@@ -4,7 +4,11 @@ type AppInstancesPerChannelMetric struct {
 	ApplicationName string `db:"app_name" json:"app_name"`
 	Version         string `db:"version" json:"version"`
 	ChannelName     string `db:"channel_name" json:"channel_name"`
-	InstancesCount  int    `db:"instances_count" json:"instances_count"`
+	// Arch is the numeric value of the channel's Arch (see arch.go). It is
+	// -1 when the instance's group has no channel (or no group) assigned,
+	// in which case ChannelName is also "".
+	Arch           int `db:"arch" json:"arch"`
+	InstancesCount int `db:"instances_count" json:"instances_count"`
 }
 
 type FailedUpdatesMetric struct {

backend/pkg/metrics/metrics.go

@@ -8,9 +8,15 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/flatcar/nebraska/backend/pkg/api"
+	"github.com/flatcar/nebraska/backend/pkg/api/types"
 	"github.com/flatcar/nebraska/backend/pkg/logger"
 )
 
+// noChannelArchLabel is the "arch" label value used for instances whose
+// group has no channel (or no group) assigned, since there is no
+// architecture to report in that case.
+const noChannelArchLabel = "none"
+
 const (
 	defaultMetricsUpdateInterval = 15 * time.Second
 )
@@ -26,6 +32,12 @@ var (
 			"application",
 			"version",
 			"channel",
+			// arch distinguishes channels that share the same name across
+			// architectures (e.g. "stable" for amd64 and arm64). This is a
+			// new label: existing dashboards/alerts that group solely by
+			// application/version/channel should add `by (arch)` or sum()
+			// over the new label to keep prior totals.
+			"arch",
 		},
 	)
 
@@ -136,7 +148,11 @@ func calculateMetrics(api *api.API) error {
 	}
 
 	for _, metric := range aipcMetrics {
-		appInstancePerChannelGaugeMetric.WithLabelValues(metric.ApplicationName, metric.Version, metric.ChannelName).Set(float64(metric.InstancesCount))
+		archLabel := noChannelArchLabel
+		if metric.Arch >= 0 {
+			archLabel = types.Arch(uint(metric.Arch)).String()
+		}
+		appInstancePerChannelGaugeMetric.WithLabelValues(metric.ApplicationName, metric.Version, metric.ChannelName, archLabel).Set(float64(metric.InstancesCount))
 	}
 
 	fuMetrics, err := api.GetFailedUpdatesMetrics()