Skip to content

Commit

Permalink
[MI-3643] Add metrics to show messages confirmed count (#356)
Browse files Browse the repository at this point in the history 
* [MI-3624] Add some additional metrics

* [MI-3624] Remove extra comments

* [MI-3624] Fix testcases

* [MI-3642] Add metrics to count upstream users

* [MI-3643] Add metrics to show messages confirmed count

* [MI-3624] Update variable names and add new

* [MI-3624] Update function, added util and minor review fixes

* [MI-3624] Reuse constants from other packages

* [MI-3642] Add comments in metrics interface

* [MI-3624] Update function arguments type

* [MI-3642] Remove extra debug log and re-organize interface methods

* [MI-3624] Fix lint

* [MI-3642] Reverting removal of extra information from debug log

* [MI-3643] Move constants from handlers to metrics package and update metrics name"

* Update server/metrics/metrics.go

* [MI-3642] Removed the term "Total" from some of the metrics

* Update server/metrics/metrics.go

Co-authored-by: Manoj Malik <manoj@brightscout.com>

---------

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Manoj <manoj@brightscout.com>
  • Loading branch information
@ayusht2810 @lieut-data @manojmalik20
3 people authored Oct 25, 2023
1 parent e82ec25 commit 17373d2
Show file tree
Hide file tree
Showing 11 changed files with 343 additions and 315 deletions.
12 changes: 6 additions & 6 deletions server/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"testing"
"time"

"github.com/mattermost/mattermost-plugin-msteams-sync/server/handlers"
"github.com/mattermost/mattermost-plugin-msteams-sync/server/metrics"
metricsmocks "github.com/mattermost/mattermost-plugin-msteams-sync/server/metrics/mocks"
"github.com/mattermost/mattermost-plugin-msteams-sync/server/msteams"
clientmocks "github.com/mattermost/mattermost-plugin-msteams-sync/server/msteams/mocks"
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
},
func() {
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", handlers.ActionCreated).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", metrics.ActionCreated).Times(1)
},
http.StatusAccepted,
"",
Expand All @@ -109,7 +109,7 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", handlers.ActionCreated).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", metrics.ActionCreated).Times(1)
},
http.StatusAccepted,
"",
Expand All @@ -128,7 +128,7 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", handlers.ActionCreated).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", metrics.ActionCreated).Times(1)
},
http.StatusAccepted,
"",
Expand All @@ -147,7 +147,7 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", handlers.ActionCreated).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", metrics.ActionCreated).Times(1)
},
http.StatusAccepted,
"",
Expand Down Expand Up @@ -181,7 +181,7 @@ func TestSubscriptionNewMesage(t *testing.T) {
if tc.ExpectedBody != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
} else {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementChangeEventQueueLength", handlers.ActionCreated).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementChangeEventQueueLength", metrics.ActionCreated).Times(1)
}

w := httptest.NewRecorder()
Expand Down
15 changes: 8 additions & 7 deletions server/handlers/attachments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"
"time"

"github.com/mattermost/mattermost-plugin-msteams-sync/server/metrics"
"github.com/mattermost/mattermost-plugin-msteams-sync/server/msteams"
"github.com/mattermost/mattermost-server/v6/app/imaging"
"github.com/mattermost/mattermost-server/v6/model"
Expand Down Expand Up @@ -151,22 +152,22 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
attachmentData, err = ah.handleDownloadFile(a.ContentURL, client)
if err != nil {
ah.plugin.GetAPI().LogError("failed to download the file", "filename", a.Name, "error", err.Error())
ah.plugin.GetMetrics().ObserveFileCount(ActionCreated, actionSourceMSTeams, discardedReasonUnableToGetTeamsData, isDirectMessage)
ah.plugin.GetMetrics().ObserveFileCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonUnableToGetTeamsData, isDirectMessage)
continue
}
} else {
fileSize, downloadURL, err = client.GetFileSizeAndDownloadURL(a.ContentURL)
if err != nil {
ah.plugin.GetAPI().LogError("failed to get file size and download URL", "error", err.Error())
ah.plugin.GetMetrics().ObserveFileCount(ActionCreated, actionSourceMSTeams, discardedReasonUnableToGetTeamsData, isDirectMessage)
ah.plugin.GetMetrics().ObserveFileCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonUnableToGetTeamsData, isDirectMessage)
continue
}

fileSizeAllowed := *ah.plugin.GetAPI().GetConfig().FileSettings.MaxFileSize
if fileSize > fileSizeAllowed {
ah.plugin.GetAPI().LogError("skipping file download from MS Teams because the file size is greater than the allowed size")
errorFound = true
ah.plugin.GetMetrics().ObserveFileCount(ActionCreated, actionSourceMSTeams, discardedReasonMaxFileSizeExceeded, isDirectMessage)
ah.plugin.GetMetrics().ObserveFileCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonMaxFileSizeExceeded, isDirectMessage)
continue
}

Expand All @@ -175,7 +176,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
attachmentData, err = client.GetFileContent(downloadURL)
if err != nil {
ah.plugin.GetAPI().LogError("failed to get file content", "error", err.Error())
ah.plugin.GetMetrics().ObserveFileCount(ActionCreated, actionSourceMSTeams, discardedReasonUnableToGetTeamsData, isDirectMessage)
ah.plugin.GetMetrics().ObserveFileCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonUnableToGetTeamsData, isDirectMessage)
continue
}
}
Expand All @@ -196,15 +197,15 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
}

if fileInfoID == "" {
ah.plugin.GetMetrics().ObserveFileCount(ActionCreated, actionSourceMSTeams, discardedReasonEmptyFileID, isDirectMessage)
ah.plugin.GetMetrics().ObserveFileCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonEmptyFileID, isDirectMessage)
continue
}
attachments = append(attachments, fileInfoID)
ah.plugin.GetMetrics().ObserveFileCount(ActionCreated, actionSourceMSTeams, "", isDirectMessage)
ah.plugin.GetMetrics().ObserveFileCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, "", isDirectMessage)
countFileAttachments++
if countFileAttachments == maxFileAttachmentsSupported {
ah.plugin.GetAPI().LogDebug("discarding the rest of the attachments as Mattermost supports only 10 attachments per post")
ah.plugin.GetMetrics().ObserveFilesCount(ActionCreated, actionSourceMSTeams, discardedReasonFileLimitReached, isDirectMessage, int64(len(msg.Attachments)-countNonFileAttachments-countFileAttachments))
ah.plugin.GetMetrics().ObserveFilesCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonFileLimitReached, isDirectMessage, int64(len(msg.Attachments)-countNonFileAttachments-countFileAttachments))
break
}
}
Expand Down
47 changes: 24 additions & 23 deletions server/handlers/attachments_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

mocksPlugin "github.com/mattermost/mattermost-plugin-msteams-sync/server/handlers/mocks"
"github.com/mattermost/mattermost-plugin-msteams-sync/server/metrics"
mocksMetrics "github.com/mattermost/mattermost-plugin-msteams-sync/server/metrics/mocks"
"github.com/mattermost/mattermost-plugin-msteams-sync/server/msteams"
mocksClient "github.com/mattermost/mattermost-plugin-msteams-sync/server/msteams/mocks"
Expand Down Expand Up @@ -257,7 +258,7 @@ func TestHandleMessageReference(t *testing.T) {
func TestHandleAttachments(t *testing.T) {
for _, testCase := range []struct {
description string
setupPlugin func(plugin *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics)
setupPlugin func(plugin *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics)
setupAPI func(mockAPI *plugintest.API)
setupClient func(*mocksClient.Client)
setupMetrics func(*mocksMetrics.Metrics)
Expand All @@ -269,11 +270,11 @@ func TestHandleAttachments(t *testing.T) {
}{
{
description: "Successfully handled attachments",
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics) {
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
p.On("GetClientForApp").Return(client).Times(1)
p.On("GetAPI").Return(mockAPI).Times(2)
p.On("GetMaxSizeForCompleteDownload").Return(1).Times(1)
p.On("GetMetrics").Return(metrics).Times(1)
p.On("GetMetrics").Return(mockmetrics).Times(1)
},
setupAPI: func(mockAPI *plugintest.API) {
mockAPI.On("GetConfig").Return(&model.Config{
Expand All @@ -289,8 +290,8 @@ func TestHandleAttachments(t *testing.T) {
client.On("GetFileSizeAndDownloadURL", "").Return(int64(5), "mockDownloadURL", nil).Once()
client.On("GetFileContent", "mockDownloadURL").Return([]byte{}, nil).Once()
},
setupMetrics: func(metrics *mocksMetrics.Metrics) {
metrics.On("ObserveFileCount", ActionCreated, actionSourceMSTeams, "", false).Times(1)
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {
mockmetrics.On("ObserveFileCount", metrics.ActionCreated, metrics.ActionSourceMSTeams, "", false).Times(1)
},
attachments: []msteams.Attachment{
{
Expand All @@ -302,15 +303,15 @@ func TestHandleAttachments(t *testing.T) {
},
{
description: "Client is nil",
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics) {
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
p.On("GetClientForApp").Return(nil)
p.On("GetAPI").Return(mockAPI)
},
setupAPI: func(mockAPI *plugintest.API) {
mockAPI.On("LogError", "Unable to get the client").Return()
},
setupClient: func(client *mocksClient.Client) {},
setupMetrics: func(metrics *mocksMetrics.Metrics) {},
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {},
attachments: []msteams.Attachment{
{
Name: "mock-name",
Expand All @@ -319,11 +320,11 @@ func TestHandleAttachments(t *testing.T) {
},
{
description: "Error uploading the file",
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics) {
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
p.On("GetClientForApp").Return(client).Once()
p.On("GetAPI").Return(mockAPI).Times(3)
p.On("GetMaxSizeForCompleteDownload").Return(1).Times(1)
p.On("GetMetrics").Return(metrics).Times(1)
p.On("GetMetrics").Return(mockmetrics).Times(1)
},
setupAPI: func(mockAPI *plugintest.API) {
mockAPI.On("GetConfig").Return(&model.Config{
Expand All @@ -338,8 +339,8 @@ func TestHandleAttachments(t *testing.T) {
client.On("GetFileSizeAndDownloadURL", "").Return(int64(5), "mockDownloadURL", nil).Once()
client.On("GetFileContent", "mockDownloadURL").Return([]byte{}, nil).Once()
},
setupMetrics: func(metrics *mocksMetrics.Metrics) {
metrics.On("ObserveFileCount", ActionCreated, actionSourceMSTeams, discardedReasonEmptyFileID, false).Times(1)
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {
mockmetrics.On("ObserveFileCount", metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonEmptyFileID, false).Times(1)
},
attachments: []msteams.Attachment{
{
Expand All @@ -350,11 +351,11 @@ func TestHandleAttachments(t *testing.T) {
},
{
description: "Number of attachments are greater than 10",
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics) {
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
p.On("GetClientForApp").Return(client).Once()
p.On("GetAPI").Return(mockAPI)
p.On("GetMaxSizeForCompleteDownload").Return(1).Times(10)
p.On("GetMetrics").Return(metrics).Times(11)
p.On("GetMetrics").Return(mockmetrics).Times(11)
},
setupAPI: func(mockAPI *plugintest.API) {
mockAPI.On("GetConfig").Return(&model.Config{
Expand All @@ -369,9 +370,9 @@ func TestHandleAttachments(t *testing.T) {
client.On("GetFileSizeAndDownloadURL", "").Return(int64(5), "mockDownloadURL", nil).Times(10)
client.On("GetFileContent", "mockDownloadURL").Return([]byte{}, nil).Times(10)
},
setupMetrics: func(metrics *mocksMetrics.Metrics) {
metrics.On("ObserveFileCount", ActionCreated, actionSourceMSTeams, "", false).Times(10)
metrics.On("ObserveFilesCount", ActionCreated, actionSourceMSTeams, discardedReasonFileLimitReached, false, int64(2)).Times(1)
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {
mockmetrics.On("ObserveFileCount", metrics.ActionCreated, metrics.ActionSourceMSTeams, "", false).Times(10)
mockmetrics.On("ObserveFilesCount", metrics.ActionCreated, metrics.ActionSourceMSTeams, discardedReasonFileLimitReached, false, int64(2)).Times(1)
},
attachments: []msteams.Attachment{
{}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {},
Expand All @@ -381,7 +382,7 @@ func TestHandleAttachments(t *testing.T) {
},
{
description: "Attachment type code snippet",
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics) {
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
p.On("GetClientForApp").Return(client)
},
setupAPI: func(mockAPI *plugintest.API) {
Expand All @@ -395,7 +396,7 @@ func TestHandleAttachments(t *testing.T) {
setupClient: func(client *mocksClient.Client) {
client.On("GetCodeSnippet", "https://example.com/version/chats/mock-chat-id/messages/mock-message-id/hostedContents/mock-content-id/$value").Return("snippet content", nil)
},
setupMetrics: func(metrics *mocksMetrics.Metrics) {},
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {},
attachments: []msteams.Attachment{
{
Name: "mock-name",
Expand All @@ -407,7 +408,7 @@ func TestHandleAttachments(t *testing.T) {
},
{
description: "Attachment type message reference",
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, metrics *mocksMetrics.Metrics) {
setupPlugin: func(p *mocksPlugin.PluginIface, mockAPI *plugintest.API, client *mocksClient.Client, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
p.On("GetClientForApp").Return(client)
p.On("GetStore").Return(store, nil)
p.On("GetAPI").Return(mockAPI)
Expand All @@ -427,7 +428,7 @@ func TestHandleAttachments(t *testing.T) {
}, nil)
},
setupClient: func(client *mocksClient.Client) {},
setupMetrics: func(metrics *mocksMetrics.Metrics) {},
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {},
attachments: []msteams.Attachment{{
Name: "mock-name",
ContentType: "messageReference",
Expand All @@ -443,14 +444,14 @@ func TestHandleAttachments(t *testing.T) {
mockAPI := &plugintest.API{}
client := mocksClient.NewClient(t)
store := mocksStore.NewStore(t)
metrics := mocksMetrics.NewMetrics(t)
mockmetrics := mocksMetrics.NewMetrics(t)

mockAPI.AssertExpectations(t)

testCase.setupPlugin(p, mockAPI, client, store, metrics)
testCase.setupPlugin(p, mockAPI, client, store, mockmetrics)
testCase.setupAPI(mockAPI)
testCase.setupClient(client)
testCase.setupMetrics(metrics)
testCase.setupMetrics(mockmetrics)

ah.plugin = p

Expand Down
18 changes: 6 additions & 12 deletions server/handlers/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,6 @@ const (
discardedReasonEmptyFileID = "empty_file_id"
discardedReasonMaxFileSizeExceeded = "max_file_size_exceeded"
discardedReasonExpiredSubscription = "expired_subscription"

actionSourceMSTeams = "msteams"
ActionCreated = "created"
ActionUpdated = "updated"
ActionDeleted = "deleted"
ReactionSetAction = "set"
ReactionUnsetAction = "unset"
)

type PluginIface interface {
Expand Down Expand Up @@ -218,6 +211,7 @@ func (ah *ActivityHandler) handleCreatedActivity(activityIds msteams.ActivityIds
if postInfo != nil {
ah.plugin.GetAPI().LogDebug("duplicate post")
ah.updateLastReceivedChangeDate(msg.LastUpdateAt)
ah.plugin.GetMetrics().ObserveMessagesConfirmedCount(metrics.ActionSourceMattermost, IsDirectMessage(activityIds.ChatID))
return discardedReasonDuplicatedPost
}

Expand Down Expand Up @@ -290,7 +284,7 @@ func (ah *ActivityHandler) handleCreatedActivity(activityIds msteams.ActivityIds
}

ah.plugin.GetAPI().LogDebug("Post created")
ah.plugin.GetMetrics().ObserveMessagesCount(ActionCreated, actionSourceMSTeams, IsDirectMessage(activityIds.ChatID))
ah.plugin.GetMetrics().ObserveMessagesCount(metrics.ActionCreated, metrics.ActionSourceMSTeams, IsDirectMessage(activityIds.ChatID))
if errorFound {
_ = ah.plugin.GetAPI().SendEphemeralPost(senderID, &model.Post{
ChannelId: channelID,
Expand Down Expand Up @@ -405,7 +399,7 @@ func (ah *ActivityHandler) handleUpdatedActivity(activityIds msteams.ActivityIds
}

isDirectMessage := IsDirectMessage(activityIds.ChatID)
ah.plugin.GetMetrics().ObserveMessagesCount(ActionUpdated, actionSourceMSTeams, isDirectMessage)
ah.plugin.GetMetrics().ObserveMessagesCount(metrics.ActionUpdated, metrics.ActionSourceMSTeams, isDirectMessage)
ah.updateLastReceivedChangeDate(msg.LastUpdateAt)
ah.handleReactions(postInfo.MattermostID, channelID, isDirectMessage, msg.Reactions)
return discardedReasonNone
Expand Down Expand Up @@ -449,7 +443,7 @@ func (ah *ActivityHandler) handleReactions(postID, channelID string, isDirectMes
if appErr = ah.plugin.GetAPI().RemoveReaction(r); appErr != nil {
ah.plugin.GetAPI().LogError("Unable to remove reaction", "error", appErr.Error())
}
ah.plugin.GetMetrics().ObserveReactionsCount(ReactionUnsetAction, actionSourceMSTeams, isDirectMessage)
ah.plugin.GetMetrics().ObserveReactionsCount(metrics.ReactionUnsetAction, metrics.ActionSourceMSTeams, isDirectMessage)
}
}

Expand Down Expand Up @@ -479,7 +473,7 @@ func (ah *ActivityHandler) handleReactions(postID, channelID string, isDirectMes
continue
}
ah.plugin.GetAPI().LogDebug("Added reaction", "reaction", r)
ah.plugin.GetMetrics().ObserveReactionsCount(ReactionSetAction, actionSourceMSTeams, isDirectMessage)
ah.plugin.GetMetrics().ObserveReactionsCount(metrics.ReactionSetAction, metrics.ActionSourceMSTeams, isDirectMessage)
}
}
}
Expand All @@ -500,7 +494,7 @@ func (ah *ActivityHandler) handleDeletedActivity(activityIds msteams.ActivityIds
return discardedReasonOther
}

ah.plugin.GetMetrics().ObserveMessagesCount(ActionDeleted, actionSourceMSTeams, IsDirectMessage(activityIds.ChatID))
ah.plugin.GetMetrics().ObserveMessagesCount(metrics.ActionDeleted, metrics.ActionSourceMSTeams, IsDirectMessage(activityIds.ChatID))
return discardedReasonNone
}

Expand Down
Loading

0 comments on commit 17373d2

Please sign in to comment.