From 74533371b291fcd59128cc9e5115d0144387aaff Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Thu, 26 Sep 2019 23:49:43 -0300 Subject: [PATCH] MM-18115: fix segment v3 usage (#12317) * diagnostics_test.go: fix spacing * diagnostics_test.go: explicitly assert payload This fails, since the package is currently receiving struct pointers and won't set the MessageId or Timestamp on the corresponding Message. * MM-18115: fix segment v3 usage In v5.14, we updated [github.com/segmentio/analytics-go](https://github.com/segmentio/analytics-go) to v3 as part of https://mattermost.atlassian.net/browse/MM-12389. As noted in the [migration guide](https://segment.com/docs/sources/server/go/#migrating-from-v2), the API subtly changed to expect a struct value and not a struct pointer: ```go // in v2, you would call the `Track` method with a `Track` struct. client.Track(&track) // in v3, you would call the `Enqueue` method with a `Track` struct. // Note that a pointer is not used here. client.Enqueue(track) ``` Unfortunately, we kept passing a pointer, and the package didn't complain since it only required an interface -- which the pointer to these structs still implemented. Internally, it only checked for the value types, and failed to annotate our payloads with the requisite metadata. Upstream, segment.io accepted the payload, but then discarded it silently. This has since been reported and fixed in https://github.com/segmentio/analytics-go/pull/146, but isn't yet part of a tagged release of the package. Fix our code to pass struct values instead. Fixes: MM-18115 --- app/diagnostics.go | 2 +- app/diagnostics_test.go | 66 +++++++++++++++++++++++++++++++++-------- app/server.go | 2 +- 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/app/diagnostics.go b/app/diagnostics.go index d5541b5814..758b25ace4 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -75,7 +75,7 @@ func (a *App) sendDailyDiagnostics(override bool) { } func (a *App) SendDiagnostic(event string, properties map[string]interface{}) { - a.Srv.diagnosticClient.Enqueue(&analytics.Track{ + a.Srv.diagnosticClient.Enqueue(analytics.Track{ Event: event, UserId: a.DiagnosticId(), Properties: properties, diff --git a/app/diagnostics_test.go b/app/diagnostics_test.go index 0d70beef0e..8d9b51da1c 100644 --- a/app/diagnostics_test.go +++ b/app/diagnostics_test.go @@ -4,6 +4,7 @@ package app import ( + "encoding/json" "io/ioutil" "net/http" "net/http/httptest" @@ -50,12 +51,34 @@ func TestDiagnostics(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - data := make(chan string, 100) + type payload struct { + MessageId string + SentAt time.Time + Batch []struct { + MessageId string + UserId string + Event string + Timestamp time.Time + Properties map[string]interface{} + } + Context struct { + Library struct { + Name string + Version string + } + } + } + + data := make(chan payload, 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) + var p payload + err = json.Unmarshal(body, &p) + require.NoError(t, err) + + data <- p })) defer server.Close() @@ -63,12 +86,30 @@ func TestDiagnostics(t *testing.T) { th.App.SetDiagnosticId(diagnosticID) th.Server.initDiagnostics(server.URL) + assertPayload := func(t *testing.T, actual payload, event string, properties map[string]interface{}) { + assert.NotEmpty(t, actual.MessageId) + assert.False(t, actual.SentAt.IsZero()) + if assert.Len(t, actual.Batch, 1) { + assert.NotEmpty(t, actual.Batch[0].MessageId, "message id should not be empty") + assert.Equal(t, diagnosticID, actual.Batch[0].UserId) + if event != "" { + assert.Equal(t, event, actual.Batch[0].Event) + } + assert.False(t, actual.Batch[0].Timestamp.IsZero(), "batch timestamp should not be the zero value") + if properties != nil { + assert.Equal(t, properties, actual.Batch[0].Properties) + } + } + assert.Equal(t, "analytics-go", actual.Context.Library.Name) + assert.Equal(t, "3.0.0", actual.Context.Library.Version) + } + // Should send a client identify message select { case identifyMessage := <-data: - require.Contains(t, identifyMessage, diagnosticID) + assertPayload(t, identifyMessage, "", nil) case <-time.After(time.Second * 1): - require.Fail(t,"Did not receive ID message") + require.Fail(t, "Did not receive ID message") } t.Run("Send", func(t *testing.T) { @@ -78,30 +119,31 @@ func TestDiagnostics(t *testing.T) { }) select { case result := <-data: - require.Contains(t, result, testValue) + assertPayload(t, result, "Testing Diagnostic", map[string]interface{}{ + "hey": testValue, + }) case <-time.After(time.Second * 1): - require.Fail(t,"Did not receive diagnostic") + require.Fail(t, "Did not receive diagnostic") } }) t.Run("SendDailyDiagnostics", func(t *testing.T) { th.App.sendDailyDiagnostics(true) - var info string + var info []string // Collect the info sent. Loop: for { select { case result := <-data: - info += result + assertPayload(t, result, "", nil) + info = append(info, result.Batch[0].Event) case <-time.After(time.Second * 1): break Loop } } for _, item := range []string{ - TRACK_CONFIG_SERVICE, - TRACK_CONFIG_TEAM, TRACK_CONFIG_SERVICE, TRACK_CONFIG_TEAM, TRACK_CONFIG_SQL, @@ -137,7 +179,7 @@ func TestDiagnostics(t *testing.T) { select { case <-data: - require.Fail(t,"Should not send diagnostics when the segment key is not set") + require.Fail(t, "Should not send diagnostics when the segment key is not set") case <-time.After(time.Second * 1): // Did not receive diagnostics } @@ -150,7 +192,7 @@ func TestDiagnostics(t *testing.T) { select { case <-data: - require.Fail(t,"Should not send diagnostics when they are disabled") + require.Fail(t, "Should not send diagnostics when they are disabled") case <-time.After(time.Second * 1): // Did not receive diagnostics } diff --git a/app/server.go b/app/server.go index 7a832530ed..d8568011f7 100644 --- a/app/server.go +++ b/app/server.go @@ -760,7 +760,7 @@ func (s *Server) initDiagnostics(endpoint string) { config.BatchSize = 1 } client, _ := analytics.NewWithConfig(SEGMENT_KEY, config) - client.Enqueue(&analytics.Identify{ + client.Enqueue(analytics.Identify{ UserId: s.diagnosticId, })