Improves group membership telemetry. (#20437)

* Improves groups membership telemetry.

* Removes unnecessary where clause (it's added by default).

* Cleans up data.

* Testing the relative group counts.

* Adds missing test mock.

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

services/telemetry/telemetry.go

@@ -1164,22 +1164,34 @@ func (ts *TelemetryService) trackGroups() {
 		mlog.Debug("Could not get distinct_group_member_count", mlog.Err(err))
 	}
 
+	distinctCustomGroupMemberCount, err := ts.dbStore.Group().DistinctGroupMemberCountForSource(model.GroupSourceCustom)
+	if err != nil {
+		mlog.Debug("Could not get distinct_custom_group_member_count", mlog.Err(err))
+	}
+
+	distinctLdapGroupMemberCount, err := ts.dbStore.Group().DistinctGroupMemberCountForSource(model.GroupSourceLdap)
+	if err != nil {
+		mlog.Debug("Could not get distinct_ldap_group_member_count", mlog.Err(err))
+	}
+
 	groupCountWithAllowReference, err := ts.dbStore.Group().GroupCountWithAllowReference()
 	if err != nil {
 		mlog.Debug("Could not get group_count_with_allow_reference", mlog.Err(err))
 	}
 
 	ts.SendTelemetry(TrackGroups, map[string]interface{}{
-		"group_count":                      groupCount,
-		"ldap_group_count":                 ldapGroupCount,
-		"custom_group_count":               customGroupCount,
-		"group_team_count":                 groupTeamCount,
-		"group_channel_count":              groupChannelCount,
-		"group_synced_team_count":          groupSyncedTeamCount,
-		"group_synced_channel_count":       groupSyncedChannelCount,
-		"group_member_count":               groupMemberCount,
-		"distinct_group_member_count":      distinctGroupMemberCount,
-		"group_count_with_allow_reference": groupCountWithAllowReference,
+		"group_count":                        groupCount,
+		"ldap_group_count":                   ldapGroupCount,
+		"custom_group_count":                 customGroupCount,
+		"group_team_count":                   groupTeamCount,
+		"group_channel_count":                groupChannelCount,
+		"group_synced_team_count":            groupSyncedTeamCount,
+		"group_synced_channel_count":         groupSyncedChannelCount,
+		"group_member_count":                 groupMemberCount,
+		"distinct_group_member_count":        distinctGroupMemberCount,
+		"distinct_custom_group_member_count": distinctCustomGroupMemberCount,
+		"distinct_ldap_group_member_count":   distinctLdapGroupMemberCount,
+		"group_count_with_allow_reference":   groupCountWithAllowReference,
 	})
 }
 

services/telemetry/telemetry_test.go

@@ -242,6 +242,7 @@ func initializeMocks(cfg *model.Config, cloudLicense bool) (*mocks.ServerIface,
 	groupStore.On("GroupCountWithAllowReference").Return(int64(13), nil)
 	groupStore.On("GroupCountBySource", model.GroupSourceCustom).Return(int64(10), nil)
 	groupStore.On("GroupCountBySource", model.GroupSourceLdap).Return(int64(2), nil)
+	groupStore.On("DistinctGroupMemberCountForSource", mock.AnythingOfType("model.GroupSource")).Return(int64(1), nil)
 
 	schemeStore := storeMocks.SchemeStore{}
 	schemeStore.On("CountByScope", "channel").Return(int64(8), nil)

store/opentracinglayer/opentracinglayer.go

@@ -3908,6 +3908,24 @@ func (s *OpenTracingLayerGroupStore) DistinctGroupMemberCount() (int64, error) {
 	return result, err
 }
 
+func (s *OpenTracingLayerGroupStore) DistinctGroupMemberCountForSource(source model.GroupSource) (int64, error) {
+	origCtx := s.Root.Store.Context()
+	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "GroupStore.DistinctGroupMemberCountForSource")
+	s.Root.Store.SetContext(newCtx)
+	defer func() {
+		s.Root.Store.SetContext(origCtx)
+	}()
+
+	defer span.Finish()
+	result, err := s.GroupStore.DistinctGroupMemberCountForSource(source)
+	if err != nil {
+		span.LogFields(spanlog.Error(err))
+		ext.Error.Set(span, true)
+	}
+
+	return result, err
+}
+
 func (s *OpenTracingLayerGroupStore) Get(groupID string) (*model.Group, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "GroupStore.Get")

store/retrylayer/retrylayer.go

@@ -4394,6 +4394,27 @@ func (s *RetryLayerGroupStore) DistinctGroupMemberCount() (int64, error) {
 
 }
 
+func (s *RetryLayerGroupStore) DistinctGroupMemberCountForSource(source model.GroupSource) (int64, error) {
+
+	tries := 0
+	for {
+		result, err := s.GroupStore.DistinctGroupMemberCountForSource(source)
+		if err == nil {
+			return result, nil
+		}
+		if !isRepeatableError(err) {
+			return result, err
+		}
+		tries++
+		if tries >= 3 {
+			err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
+			return result, err
+		}
+		timepkg.Sleep(100 * timepkg.Millisecond)
+	}
+
+}
+
 func (s *RetryLayerGroupStore) Get(groupID string) (*model.Group, error) {
 
 	tries := 0

store/sqlstore/group_store.go

@@ -1740,6 +1740,26 @@ func (s *SqlGroupStore) DistinctGroupMemberCount() (int64, error) {
 	return s.countTableWithSelectAndWhere("COUNT(DISTINCT UserId)", "GroupMembers", nil)
 }
 
+func (s *SqlGroupStore) DistinctGroupMemberCountForSource(source model.GroupSource) (int64, error) {
+	builder := s.getQueryBuilder().
+		Select("COUNT(DISTINCT GroupMembers.UserId)").
+		From("GroupMembers").
+		Join("UserGroups ON GroupMembers.GroupId = UserGroups.Id").
+		Where(sq.Eq{"UserGroups.Source": source, "GroupMembers.DeleteAt": 0})
+
+	query, args, err := builder.ToSql()
+	if err != nil {
+		return 0, errors.Wrap(err, "distinct_group_member_count_for_source_tosql")
+	}
+
+	var count int64
+	if err = s.GetReplicaX().Get(&count, query, args...); err != nil {
+		return 0, errors.Wrapf(err, "failed to select distinct groupmember count for source %q", source)
+	}
+
+	return count, nil
+}
+
 func (s *SqlGroupStore) GroupCountWithAllowReference() (int64, error) {
 	return s.countTableWithSelectAndWhere("COUNT(*)", "UserGroups", sq.Eq{"AllowReference": true, "DeleteAt": 0})
 }

store/store.go

@@ -886,6 +886,8 @@ type GroupStore interface {
 	// DistinctGroupMemberCount returns the count of records in the GroupMembers table with distinct userID values.
 	DistinctGroupMemberCount() (int64, error)
 
+	DistinctGroupMemberCountForSource(source model.GroupSource) (int64, error)
+
 	// GroupCountWithAllowReference returns the count of records in the Groups table with AllowReference set to true.
 	GroupCountWithAllowReference() (int64, error)
 

store/storetest/group_store.go

@@ -90,6 +90,8 @@ func TestGroupStore(t *testing.T, ss store.Store) {
 
 	t.Run("GetMember", func(t *testing.T) { groupTestGetMember(t, ss) })
 	t.Run("GetNonMemberUsersPage", func(t *testing.T) { groupTestGetNonMemberUsersPage(t, ss) })
+
+	t.Run("DistinctGroupMemberCountForSource", func(t *testing.T) { groupTestDistinctGroupMemberCountForSource(t, ss) })
 }
 
 func testGroupStoreCreate(t *testing.T, ss store.Store) {
@@ -5062,3 +5064,82 @@ func groupTestGetNonMemberUsersPage(t *testing.T, ss store.Store) {
 	require.Error(t, err)
 	require.Nil(t, users)
 }
+
+func groupTestDistinctGroupMemberCountForSource(t *testing.T, ss store.Store) {
+	// get the before counts
+	customGroupCountBefore, err := ss.Group().DistinctGroupMemberCountForSource(model.GroupSourceCustom)
+	require.NoError(t, err)
+	ldapGroupCountBefore, err := ss.Group().DistinctGroupMemberCountForSource(model.GroupSourceLdap)
+	require.NoError(t, err)
+
+	// create 2 groups, 1 custom and 1 ldap
+	g1 := &model.Group{
+		Name:        model.NewString(model.NewId()),
+		DisplayName: model.NewId(),
+		Description: model.NewId(),
+		Source:      model.GroupSourceCustom,
+		RemoteId:    model.NewString(model.NewId()),
+	}
+	customGroup, err := ss.Group().Create(g1)
+	require.NoError(t, err)
+
+	g2 := &model.Group{
+		Name:        model.NewString(model.NewId()),
+		DisplayName: model.NewId(),
+		Description: model.NewId(),
+		Source:      model.GroupSourceLdap,
+		RemoteId:    model.NewString(model.NewId()),
+	}
+	ldapGroup, err := ss.Group().Create(g2)
+	require.NoError(t, err)
+
+	// create a couple of users
+	u1 := &model.User{
+		Email:    MakeEmail(),
+		Username: model.NewId(),
+	}
+	user1, nErr := ss.User().Save(u1)
+	require.NoError(t, nErr)
+
+	u2 := &model.User{
+		Email:    MakeEmail(),
+		Username: model.NewId(),
+	}
+	user2, nErr := ss.User().Save(u2)
+	require.NoError(t, nErr)
+
+	// add both new users to both new groups
+	_, err = ss.Group().UpsertMember(customGroup.Id, user1.Id)
+	require.NoError(t, err)
+	_, err = ss.Group().UpsertMember(ldapGroup.Id, user1.Id)
+	require.NoError(t, err)
+
+	_, err = ss.Group().UpsertMember(customGroup.Id, user2.Id)
+	require.NoError(t, err)
+	_, err = ss.Group().UpsertMember(ldapGroup.Id, user2.Id)
+	require.NoError(t, err)
+
+	// remove one user from a group to ensure the 'where deleteat = 0' clause is working
+	_, err = ss.Group().DeleteMember(ldapGroup.Id, user1.Id)
+	require.NoError(t, err)
+
+	defer func() {
+		ss.Group().DeleteMember(ldapGroup.Id, user2.Id)
+		ss.Group().DeleteMember(customGroup.Id, user1.Id)
+		ss.Group().DeleteMember(customGroup.Id, user2.Id)
+
+		ss.Group().Delete(customGroup.Id)
+		ss.Group().Delete(ldapGroup.Id)
+
+		ss.User().PermanentDelete(user1.Id)
+		ss.User().PermanentDelete(user2.Id)
+	}()
+
+	customGroupCount, err := ss.Group().DistinctGroupMemberCountForSource(model.GroupSourceCustom)
+	require.NoError(t, err)
+	require.Equal(t, customGroupCountBefore+2, customGroupCount)
+
+	ldapGroupCount, err := ss.Group().DistinctGroupMemberCountForSource(model.GroupSourceLdap)
+	require.NoError(t, err)
+	require.Equal(t, ldapGroupCountBefore+1, ldapGroupCount)
+}

store/storetest/mocks/GroupStore.go

@@ -372,6 +372,27 @@ func (_m *GroupStore) DistinctGroupMemberCount() (int64, error) {
 	return r0, r1
 }
 
+// DistinctGroupMemberCountForSource provides a mock function with given fields: source
+func (_m *GroupStore) DistinctGroupMemberCountForSource(source model.GroupSource) (int64, error) {
+	ret := _m.Called(source)
+
+	var r0 int64
+	if rf, ok := ret.Get(0).(func(model.GroupSource) int64); ok {
+		r0 = rf(source)
+	} else {
+		r0 = ret.Get(0).(int64)
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(model.GroupSource) error); ok {
+		r1 = rf(source)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // Get provides a mock function with given fields: groupID
 func (_m *GroupStore) Get(groupID string) (*model.Group, error) {
 	ret := _m.Called(groupID)

store/timerlayer/timerlayer.go

@@ -3563,6 +3563,22 @@ func (s *TimerLayerGroupStore) DistinctGroupMemberCount() (int64, error) {
 	return result, err
 }
 
+func (s *TimerLayerGroupStore) DistinctGroupMemberCountForSource(source model.GroupSource) (int64, error) {
+	start := timemodule.Now()
+
+	result, err := s.GroupStore.DistinctGroupMemberCountForSource(source)
+
+	elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second)
+	if s.Root.Metrics != nil {
+		success := "false"
+		if err == nil {
+			success = "true"
+		}
+		s.Root.Metrics.ObserveStoreMethodDuration("GroupStore.DistinctGroupMemberCountForSource", success, elapsed)
+	}
+	return result, err
+}
+
 func (s *TimerLayerGroupStore) Get(groupID string) (*model.Group, error) {
 	start := time.Now()