Stats: Stop counting the same user multiple times (#26777)

* Stats: Cache based stats implementation

* Stats: Correct logic and add larger scale test

* Stats: linter

* Stats: SQL implementation

* Stats: cleanup SQL

* Stats: Tab -> Spaces

* Update pkg/services/sqlstore/stats.go

Co-authored-by: Sofia Papagiannaki <papagian@users.noreply.github.com>

* Stats: Quote 'user' table with dialect.Quote

* Stats: Ensure test is run as integration test

* Stats: Use boolean value

...because if (v) { true } else { false } is unnecessary at best.

Co-authored-by: Sofia Papagiannaki <papagian@users.noreply.github.com>

pkg/models/stats.go

@@ -92,13 +92,15 @@ type GetSystemUserCountStatsQuery struct {
 	Result *SystemUserCountStats
 }
 
-type ActiveUserStats struct {
+type UserStats struct {
-	ActiveUsers   int64
+	Users   int64
-	ActiveAdmins  int64
+	Admins  int64
-	ActiveEditors int64
+	Editors int64
-	ActiveViewers int64
+	Viewers int64
 }
 
-type GetActiveUserStatsQuery struct {
+type GetUserStatsQuery struct {
-	Result *ActiveUserStats
+	MustUpdate bool
+	Active     bool
+	Result     UserStats
 }

pkg/services/sqlstore/stats.go

@@ -2,6 +2,7 @@ package sqlstore
 
 import (
 	"context"
+	"strconv"
 	"time"
 
 	"github.com/grafana/grafana/pkg/bus"
@@ -13,12 +14,12 @@ func init() {
 	bus.AddHandler("sql", GetDataSourceStats)
 	bus.AddHandler("sql", GetDataSourceAccessStats)
 	bus.AddHandler("sql", GetAdminStats)
-	bus.AddHandler("sql", GetActiveUserStats)
+	bus.AddHandler("sql", GetUserStats)
 	bus.AddHandlerCtx("sql", GetAlertNotifiersUsageStats)
 	bus.AddHandlerCtx("sql", GetSystemUserCountStats)
 }
 
-var activeUserTimeLimit = time.Hour * 24 * 30
+const activeUserTimeLimit = time.Hour * 24 * 30
 
 func GetAlertNotifiersUsageStats(ctx context.Context, query *models.GetAlertNotifierUsageStatsQuery) error {
 	var rawSql = `SELECT COUNT(*) AS count, type FROM ` + dialect.Quote("alert_notification") + ` GROUP BY type`
@@ -80,9 +81,7 @@ func GetSystemStats(query *models.GetSystemStatsQuery) error {
 	sb.Write(`(SELECT COUNT(id) FROM ` + dialect.Quote("team") + `) AS teams,`)
 	sb.Write(`(SELECT COUNT(id) FROM ` + dialect.Quote("user_auth_token") + `) AS auth_tokens,`)
 
-	sb.Write(roleCounterSQL("Viewer", "viewers", false)+`,`, activeUserDeadlineDate)
+	sb.Write(roleCounterSQL())
-	sb.Write(roleCounterSQL("Editor", "editors", false)+`,`, activeUserDeadlineDate)
-	sb.Write(roleCounterSQL("Admin", "admins", false)+``, activeUserDeadlineDate)
 
 	var stats models.SystemStats
 	_, err := x.SQL(sb.GetSqlString(), sb.params...).Get(&stats)
@@ -95,22 +94,15 @@ func GetSystemStats(query *models.GetSystemStatsQuery) error {
 	return nil
 }
 
-func roleCounterSQL(role string, alias string, onlyActive bool) string {
+func roleCounterSQL() string {
-	var sqlQuery = `
+	_ = updateUserRoleCountsIfNecessary(false)
-		(
+	sqlQuery :=
-			SELECT COUNT(DISTINCT u.id)
+		strconv.FormatInt(userStatsCache.total.Admins, 10) + ` AS admins, ` +
-			FROM ` + dialect.Quote("user") + ` AS u, org_user
+			strconv.FormatInt(userStatsCache.total.Editors, 10) + ` AS editors, ` +
-			WHERE u.last_seen_at > ? AND ( org_user.user_id=u.id AND org_user.role='` + role + `' )
+			strconv.FormatInt(userStatsCache.total.Viewers, 10) + ` AS viewers, ` +
-		) AS active_` + alias
+			strconv.FormatInt(userStatsCache.active.Admins, 10) + ` AS active_admins, ` +
-
+			strconv.FormatInt(userStatsCache.active.Editors, 10) + ` AS active_editors, ` +
-	if !onlyActive {
+			strconv.FormatInt(userStatsCache.active.Viewers, 10) + ` AS active_viewers`
-		sqlQuery += `,
-		(
-			SELECT COUNT(DISTINCT u.id)
-			FROM ` + dialect.Quote("user") + ` AS u, org_user
-			WHERE ( org_user.user_id=u.id AND org_user.role='` + role + `' )
-		) AS ` + alias
-	}
 
 	return sqlQuery
 }
@@ -159,9 +151,7 @@ func GetAdminStats(query *models.GetAdminStatsQuery) error {
 			SELECT COUNT(*)
 			FROM ` + dialect.Quote("user") + ` WHERE last_seen_at > ?
 		) AS active_users,
-		` + roleCounterSQL("Admin", "admins", false) + `,
+		` + roleCounterSQL() + `,
-		` + roleCounterSQL("Editor", "editors", false) + `,
-		` + roleCounterSQL("Viewer", "viewers", false) + `,
 		(
 			SELECT COUNT(*)
 			FROM ` + dialect.Quote("user_auth_token") + ` WHERE rotated_at > ?
@@ -192,23 +182,105 @@ func GetSystemUserCountStats(ctx context.Context, query *models.GetSystemUserCou
 	})
 }
 
-func GetActiveUserStats(query *models.GetActiveUserStatsQuery) error {
+func GetUserStats(query *models.GetUserStatsQuery) error {
-	activeUserDeadlineDate := time.Now().Add(-activeUserTimeLimit)
+	err := updateUserRoleCountsIfNecessary(query.MustUpdate)
-	sb := &SqlBuilder{}
-
-	sb.Write(`SELECT `)
-	sb.Write(`(SELECT COUNT(*) FROM `+dialect.Quote("user")+` WHERE last_seen_at > ?) AS active_users,`, activeUserDeadlineDate)
-	sb.Write(roleCounterSQL("Viewer", "viewers", true)+`,`, activeUserDeadlineDate)
-	sb.Write(roleCounterSQL("Editor", "editors", true)+`,`, activeUserDeadlineDate)
-	sb.Write(roleCounterSQL("Admin", "admins", true)+``, activeUserDeadlineDate)
-
-	var stats models.ActiveUserStats
-	_, err := x.SQL(sb.GetSqlString(), sb.params...).Get(&stats)
 	if err != nil {
 		return err
 	}
 
-	query.Result = &stats
+	if query.Active {
+		query.Result = userStatsCache.active
+	} else {
+		query.Result = userStatsCache.total
+	}
 
 	return nil
 }
+
+func updateUserRoleCountsIfNecessary(forced bool) error {
+	memoizationPeriod := time.Now().Add(-userStatsCacheLimetime)
+	if forced || userStatsCache.memoized.Before(memoizationPeriod) {
+		err := updateUserRoleCounts()
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+type memoUserStats struct {
+	active models.UserStats
+	total  models.UserStats
+
+	memoized time.Time
+}
+
+var (
+	userStatsCache         = memoUserStats{}
+	userStatsCacheLimetime = 5 * time.Minute
+)
+
+func updateUserRoleCounts() error {
+	query := `
+SELECT role AS bitrole, active, COUNT(role) AS count FROM
+    (SELECT active, SUM(role) AS role
+     FROM (SELECT
+               u.id,
+               CASE org_user.role
+                   WHEN 'Admin' THEN 4
+                   WHEN 'Editor' THEN 2
+                   ELSE 1
+                   END AS role,
+               u.last_seen_at>? AS active
+           FROM ` + dialect.Quote("user") + ` AS u LEFT JOIN org_user ON org_user.user_id = u.id
+           GROUP BY u.id, u.last_seen_at, org_user.role) AS t2
+     GROUP BY active, id) AS t1
+GROUP BY active, role;`
+
+	activeUserDeadline := time.Now().Add(-activeUserTimeLimit)
+
+	type rolebitmap struct {
+		Active  bool
+		Bitrole int64
+		Count   int64
+	}
+
+	bitmap := []rolebitmap{}
+	err := x.SQL(query, activeUserDeadline).Find(&bitmap)
+	if err != nil {
+		return err
+	}
+
+	memo := memoUserStats{memoized: time.Now()}
+	for _, role := range bitmap {
+		roletype := models.ROLE_VIEWER
+		if role.Bitrole&0b100 != 0 {
+			roletype = models.ROLE_ADMIN
+		} else if role.Bitrole&0b10 != 0 {
+			roletype = models.ROLE_EDITOR
+		}
+
+		memo.total = addToStats(memo.total, roletype, role.Count)
+		if role.Active {
+			memo.active = addToStats(memo.active, roletype, role.Count)
+		}
+	}
+
+	userStatsCache = memo
+	return nil
+}
+
+func addToStats(base models.UserStats, role models.RoleType, count int64) models.UserStats {
+	base.Users += count
+
+	switch role {
+	case models.ROLE_ADMIN:
+		base.Admins += count
+	case models.ROLE_EDITOR:
+		base.Editors += count
+	default:
+		base.Viewers += count
+	}
+
+	return base
+}

pkg/services/sqlstore/stats_integration_test.go

@@ -0,0 +1,71 @@
+// +build integration
+
+package sqlstore
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/setting"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIntegration_GetUserStats(t *testing.T) {
+	InitTestDB(t)
+
+	cmd := &models.CreateUserCommand{
+		Email:   "admin@test.com",
+		Name:    "Admin",
+		Login:   "admin",
+		OrgName: mainOrgName,
+		IsAdmin: true,
+	}
+	err := CreateUser(context.Background(), cmd)
+	require.NoError(t, err)
+	firstUser := cmd.Result
+
+	{
+		defaultAutoAssign := setting.AutoAssignOrg
+		defaultOrgID := setting.AutoAssignOrgId
+		defaultRole := setting.AutoAssignOrgRole
+
+		setting.AutoAssignOrg = true
+		setting.AutoAssignOrgId = int(firstUser.OrgId)
+		setting.AutoAssignOrgRole = "Editor"
+
+		defer func() {
+			setting.AutoAssignOrg = defaultAutoAssign
+			setting.AutoAssignOrgId = defaultOrgID
+			setting.AutoAssignOrgRole = defaultRole
+		}()
+	}
+
+	users := make([]models.User, 5)
+
+	for i := range users {
+		cmd := &models.CreateUserCommand{
+			Email: fmt.Sprintf("usertest%v@test.com", i),
+			Name:  fmt.Sprintf("user name %v", i),
+			Login: fmt.Sprintf("user_test_%v_login", i),
+			OrgId: firstUser.OrgId,
+		}
+		err := CreateUser(context.Background(), cmd)
+		require.NoError(t, err)
+		users[i] = cmd.Result
+	}
+
+	query := models.GetUserStatsQuery{
+		MustUpdate: true,
+	}
+	err = GetUserStats(&query)
+	require.NoError(t, err)
+	assert.EqualValues(t, models.UserStats{
+		Users:   6,
+		Admins:  1,
+		Editors: 5,
+		Viewers: 0,
+	}, query.Result)
+}

pkg/services/sqlstore/stats_test.go

@@ -11,60 +11,60 @@ import (
 )
 
 func TestStatsDataAccess(t *testing.T) {
-	t.Run("Testing Stats Data Access", func(t *testing.T) {
+	InitTestDB(t)
-		InitTestDB(t)
+	populateDB(t)
 
-		t.Run("Get system stats should not results in error", func(t *testing.T) {
+	t.Run("Get system stats should not results in error", func(t *testing.T) {
-			populateDB(t)
+		query := models.GetSystemStatsQuery{}
+		err := GetSystemStats(&query)
+		require.NoError(t, err)
+		assert.Equal(t, int64(3), query.Result.Users)
+		assert.Equal(t, 0, query.Result.Editors)
+		assert.Equal(t, 0, query.Result.Viewers)
+		assert.Equal(t, 3, query.Result.Admins)
+	})
 
-			query := models.GetSystemStatsQuery{}
+	t.Run("Get system user count stats should not results in error", func(t *testing.T) {
-			err := GetSystemStats(&query)
+		query := models.GetSystemUserCountStatsQuery{}
-			require.NoError(t, err)
+		err := GetSystemUserCountStats(context.Background(), &query)
-			assert.Equal(t, int64(3), query.Result.Users)
+		assert.NoError(t, err)
-			assert.Equal(t, 1, query.Result.Editors)
+	})
-			assert.Equal(t, 1, query.Result.Viewers)
-			assert.Equal(t, 3, query.Result.Admins)
-		})
 
-		t.Run("Get system user count stats should not results in error", func(t *testing.T) {
+	t.Run("Get datasource stats should not results in error", func(t *testing.T) {
-			query := models.GetSystemUserCountStatsQuery{}
+		query := models.GetDataSourceStatsQuery{}
-			err := GetSystemUserCountStats(context.Background(), &query)
+		err := GetDataSourceStats(&query)
-			assert.NoError(t, err)
+		assert.NoError(t, err)
-		})
+	})
 
-		t.Run("Get datasource stats should not results in error", func(t *testing.T) {
+	t.Run("Get datasource access stats should not results in error", func(t *testing.T) {
-			query := models.GetDataSourceStatsQuery{}
+		query := models.GetDataSourceAccessStatsQuery{}
-			err := GetDataSourceStats(&query)
+		err := GetDataSourceAccessStats(&query)
-			assert.NoError(t, err)
+		assert.NoError(t, err)
-		})
+	})
 
-		t.Run("Get datasource access stats should not results in error", func(t *testing.T) {
+	t.Run("Get alert notifier stats should not results in error", func(t *testing.T) {
-			query := models.GetDataSourceAccessStatsQuery{}
+		query := models.GetAlertNotifierUsageStatsQuery{}
-			err := GetDataSourceAccessStats(&query)
+		err := GetAlertNotifiersUsageStats(context.Background(), &query)
-			assert.NoError(t, err)
+		assert.NoError(t, err)
-		})
+	})
 
-		t.Run("Get alert notifier stats should not results in error", func(t *testing.T) {
+	t.Run("Get admin stats should not result in error", func(t *testing.T) {
-			query := models.GetAlertNotifierUsageStatsQuery{}
+		query := models.GetAdminStatsQuery{}
-			err := GetAlertNotifiersUsageStats(context.Background(), &query)
+		err := GetAdminStats(&query)
-			assert.NoError(t, err)
+		assert.NoError(t, err)
-		})
+	})
 
-		t.Run("Get admin stats should not result in error", func(t *testing.T) {
+	t.Run("Get active user count stats should not result in error", func(t *testing.T) {
-			query := models.GetAdminStatsQuery{}
+		query := models.GetUserStatsQuery{
-			err := GetAdminStats(&query)
+			MustUpdate: true,
-			assert.NoError(t, err)
+			Active:     true,
-		})
+		}
-
+		err := GetUserStats(&query)
-		t.Run("Get active user count stats should not result in error", func(t *testing.T) {
+		require.NoError(t, err)
-			query := models.GetActiveUserStatsQuery{}
+		assert.Equal(t, int64(1), query.Result.Users)
-			err := GetActiveUserStats(&query)
+		assert.Equal(t, int64(1), query.Result.Admins)
-			require.NoError(t, err)
+		assert.Equal(t, int64(0), query.Result.Editors)
-			assert.Equal(t, int64(1), query.Result.ActiveUsers)
+		assert.Equal(t, int64(0), query.Result.Viewers)
-			assert.Equal(t, int64(1), query.Result.ActiveAdmins)
-			assert.Equal(t, int64(0), query.Result.ActiveEditors)
-			assert.Equal(t, int64(0), query.Result.ActiveViewers)
-		})
 	})
 }
 
@@ -127,4 +127,12 @@ func populateDB(t *testing.T) {
 	}
 	err = UpdateUserLastSeenAt(updateUserLastSeenAtCmd)
 	require.NoError(t, err)
+
+	// force renewal of user stats
+	query := models.GetUserStatsQuery{
+		MustUpdate: true,
+		Active:     true,
+	}
+	err = GetUserStats(&query)
+	require.NoError(t, err)
 }