[MM-16620] Properly handle analytics client life cycle

Properly handle analytics client life cycle

This change improves the life cycle management of the analytics
client. It does so in two primary ways:
 - Move the client from being a shared pointer to belonging to the
   Server struct.
 - Expose the client.Close() method for proper shutdown of the
   client.

app/diagnostics.go

@@ -58,15 +58,13 @@ const (
 	TRACK_PLUGINS  = "plugins"
 )
 
-var client *analytics.Client
-
 func (a *App) SendDailyDiagnostics() {
 	a.sendDailyDiagnostics(false)
 }
 
 func (a *App) sendDailyDiagnostics(override bool) {
 	if *a.Config().LogSettings.EnableDiagnostics && a.IsLeader() && (!strings.Contains(SEGMENT_KEY, "placeholder") || override) {
-		a.initDiagnostics("")
+		a.Srv.initDiagnostics("")
 		a.trackActivity()
 		a.trackConfig()
 		a.trackLicense()
@@ -76,24 +74,8 @@ func (a *App) sendDailyDiagnostics(override bool) {
 	}
 }
 
-func (a *App) initDiagnostics(endpoint string) {
-	if client == nil {
-		client = analytics.New(SEGMENT_KEY)
-		client.Logger = a.Log.StdLog(mlog.String("source", "segment"))
-		// For testing
-		if endpoint != "" {
-			client.Endpoint = endpoint
-			client.Verbose = true
-			client.Size = 1
-		}
-		client.Identify(&analytics.Identify{
-			UserId: a.DiagnosticId(),
-		})
-	}
-}
-
 func (a *App) SendDiagnostic(event string, properties map[string]interface{}) {
-	client.Track(&analytics.Track{
+	a.Srv.diagnosticClient.Track(&analytics.Track{
 		Event:      event,
 		UserId:     a.DiagnosticId(),
 		Properties: properties,

app/diagnostics_test.go

@@ -4,32 +4,18 @@
 package app
 
 import (
-	"bytes"
-	"io"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-server/model"
 )
 
-func newTestServer() (chan string, *httptest.Server) {
-	result := make(chan string, 100)
-
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		buf := bytes.NewBuffer(nil)
-		io.Copy(buf, r.Body)
-
-		result <- buf.String()
-	}))
-
-	return result, server
-}
-
 func TestPluginSetting(t *testing.T) {
 	settings := &model.PluginSettings{
 		Plugins: map[string]map[string]interface{}{
@@ -57,42 +43,42 @@ func TestPluginActivated(t *testing.T) {
 }
 
 func TestDiagnostics(t *testing.T) {
-	th := Setup(t).InitBasic()
-	defer th.TearDown()
-
 	if testing.Short() {
 		t.SkipNow()
 	}
 
-	data, server := newTestServer()
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	data := make(chan string, 100)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, err := ioutil.ReadAll(r.Body)
+		require.NoError(t, err)
+
+		data <- string(body)
+	}))
 	defer server.Close()
 
-	diagnosticId := "i am not real"
-	th.App.SetDiagnosticId(diagnosticId)
-	th.App.initDiagnostics(server.URL)
+	diagnosticID := "test-diagnostic-id-12345"
+	th.App.SetDiagnosticId(diagnosticID)
+	th.Server.initDiagnostics(server.URL)
 
 	// Should send a client identify message
 	select {
 	case identifyMessage := <-data:
-		t.Log("Got idmessage:\n" + identifyMessage)
-		if !strings.Contains(identifyMessage, diagnosticId) {
-			t.Fail()
-		}
+		require.Contains(t, identifyMessage, diagnosticID)
 	case <-time.After(time.Second * 1):
 		t.Fatal("Did not receive ID message")
 	}
 
 	t.Run("Send", func(t *testing.T) {
-		const TEST_VALUE = "stuff548959847"
+		testValue := "test-send-value-6789"
 		th.App.SendDiagnostic("Testing Diagnostic", map[string]interface{}{
-			"hey": TEST_VALUE,
+			"hey": testValue,
 		})
 		select {
 		case result := <-data:
-			t.Log("Got diagnostic:\n" + result)
-			if !strings.Contains(result, TEST_VALUE) {
-				t.Fail()
-			}
+			require.Contains(t, result, testValue)
 		case <-time.After(time.Second * 1):
 			t.Fatal("Did not receive diagnostic")
 		}
@@ -101,7 +87,7 @@ func TestDiagnostics(t *testing.T) {
 	t.Run("SendDailyDiagnostics", func(t *testing.T) {
 		th.App.sendDailyDiagnostics(true)
 
-		info := ""
+		var info string
 		// Collect the info sent.
 	Loop:
 		for {
@@ -142,9 +128,7 @@ func TestDiagnostics(t *testing.T) {
 			TRACK_CONFIG_MESSAGE_EXPORT,
 			TRACK_PLUGINS,
 		} {
-			if !strings.Contains(info, item) {
-				t.Fatal("Sent diagnostics missing item: " + item)
-			}
+			require.Contains(t, info, item)
 		}
 	})
 

app/server.go

@@ -20,6 +20,7 @@ import (
 	"github.com/gorilla/mux"
 	"github.com/pkg/errors"
 	"github.com/rs/cors"
+	analytics "github.com/segmentio/analytics-go"
 	"github.com/throttled/throttled"
 	"golang.org/x/crypto/acme/autocert"
 
@@ -100,7 +101,9 @@ type Server struct {
 	clientConfig        map[string]string
 	clientConfigHash    string
 	limitedClientConfig map[string]string
-	diagnosticId        string
+
+	diagnosticId     string
+	diagnosticClient *analytics.Client
 
 	phase2PermissionsMigrationComplete bool
 
@@ -321,6 +324,11 @@ func (s *Server) Shutdown() error {
 
 	s.RunOldAppShutdown()
 
+	err := s.shutdownDiagnostics()
+	if err != nil {
+		mlog.Error(fmt.Sprintf("Unable to cleanly shutdown diagnostic client: %s", err))
+	}
+
 	s.StopHTTPServer()
 	s.WaitForGoroutines()
 
@@ -734,3 +742,30 @@ func (s *Server) StartElasticsearch() {
 		}
 	})
 }
+
+func (s *Server) initDiagnostics(endpoint string) {
+	if s.diagnosticClient == nil {
+		client := analytics.New(SEGMENT_KEY)
+		client.Logger = s.Log.StdLog(mlog.String("source", "segment"))
+		// For testing
+		if endpoint != "" {
+			client.Endpoint = endpoint
+			client.Verbose = true
+			client.Size = 1
+		}
+		client.Identify(&analytics.Identify{
+			UserId: s.diagnosticId,
+		})
+
+		s.diagnosticClient = client
+	}
+}
+
+// ShutdownDiagnostics closes the diagnostic client.
+func (s *Server) shutdownDiagnostics() error {
+	if s.diagnosticClient != nil {
+		return s.diagnosticClient.Close()
+	}
+
+	return nil
+}