Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MI-3624] Add some additional metrics #354

Merged
merged 13 commits into from
Oct 24, 2023
Merged

[MI-3624] Add some additional metrics #354

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8788616
[MI-3624] Add some additional metrics
ayusht2810 Oct 18, 2023
9e4298d
[MI-3624] Remove extra comments
ayusht2810 Oct 18, 2023
f57a0f6
Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…
ayusht2810 Oct 18, 2023
6406197
[MI-3624] Fix testcases
ayusht2810 Oct 18, 2023
939a058
[MI-3624] Update variable names and add new
ayusht2810 Oct 18, 2023
8388f5c
Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…
ayusht2810 Oct 23, 2023
9a4f727
[MI-3624] Update function, added util and minor review fixes
ayusht2810 Oct 23, 2023
6628939
[MI-3624] Reuse constants from other packages
ayusht2810 Oct 23, 2023
bf17c3d
[MI-3624] Update function arguments type
ayusht2810 Oct 24, 2023
1bc0839
Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…
ayusht2810 Oct 24, 2023
da34aba
[MI-3624] Fix lint
ayusht2810 Oct 24, 2023
2d60c70
Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…
ayusht2810 Oct 24, 2023
2e0bfec
[MI-3624] Review fixes: Added another metric function for observing f…
manojmalik20 Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 52 additions & 10 deletions server/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"testing"
"time"

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"
storemocks "github.com/mattermost/mattermost-plugin-msteams-sync/server/store/mocks"
Expand Down Expand Up @@ -47,6 +48,8 @@ func TestSubscriptionValidation(t *testing.T) {

func TestSubscriptionInvalidRequest(t *testing.T) {
plugin := newTestPlugin(t)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveAPIEndpointDuration", "/changes", http.MethodPost, fmt.Sprint(http.StatusBadRequest), mock.AnythingOfType("float64")).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)

w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodPost, "/changes", strings.NewReader(""))
Expand Down Expand Up @@ -86,8 +89,9 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
},
func() {
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", actionCreated).Times(1)
},
202,
http.StatusAccepted,
"",
},
{
Expand All @@ -103,9 +107,10 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil)
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", actionCreated).Times(1)
},
202,
http.StatusAccepted,
"",
},
{
Expand All @@ -121,9 +126,10 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil)
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", actionCreated).Times(1)
},
202,
http.StatusAccepted,
"",
},
{
Expand All @@ -139,9 +145,10 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil)
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", actionCreated).Times(1)
},
202,
http.StatusAccepted,
"",
},
{
Expand All @@ -157,10 +164,10 @@ func TestSubscriptionNewMesage(t *testing.T) {
},
},
func() {
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil)
plugin.API.(*plugintest.API).On("LogError", "Unable to process created activity", "activity", mock.Anything, "error", "Invalid webhook secret").Return(nil)
plugin.store.(*storemocks.Store).On("GetTokenForMattermostUser", "bot-user-id").Return(&oauth2.Token{}, nil).Times(1)
plugin.API.(*plugintest.API).On("LogError", "Unable to process created activity", "activity", mock.Anything, "error", "Invalid webhook secret").Return(nil).Times(1)
},
400,
http.StatusBadRequest,
"Invalid webhook secret\n",
},
}
Expand All @@ -170,6 +177,11 @@ func TestSubscriptionNewMesage(t *testing.T) {
require.NoError(t, err)

tc.PopulateMocks()
if tc.ExpectedBody != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
} else {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementChangeEventQueueLength", actionCreated).Times(1)
}

w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodPost, "/changes", bytes.NewReader(data))
Expand Down Expand Up @@ -239,6 +251,7 @@ func TestGetAvatarNotFound(t *testing.T) {
plugin.store.(*storemocks.Store).On("GetAvatarCache", "user-id").Return(nil, &model.AppError{Message: "not-found"}).Times(1)
plugin.msteamsAppClient.(*clientmocks.Client).On("GetUserAvatar", "user-id").Return(nil, errors.New("not-found")).Times(1)
plugin.API.(*plugintest.API).On("LogError", "Unable to get user avatar", "msteamsUserID", "user-id", "error", "not-found").Return(nil)
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)

w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, "/avatar/user-id", nil)
Expand Down Expand Up @@ -326,6 +339,15 @@ func TestProcessActivity(t *testing.T) {
t.Run(test.Name, func(t *testing.T) {
assert := assert.New(t)
plugin := newTestPlugin(t)
if test.ExpectedResult != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
}

if test.ExpectedStatusCode == http.StatusAccepted {
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveChangeEventTotal", "mockChangeType").Times(1)
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementChangeEventQueueLength", "mockChangeType").Times(1)
}

test.SetupStore(plugin.store.(*storemocks.Store))
test.SetupAPI(plugin.API.(*plugintest.API))
test.SetupClient(plugin.msteamsAppClient.(*clientmocks.Client), plugin.clientBuilderWithToken("", "", "", "", nil, nil).(*clientmocks.Client))
Expand Down Expand Up @@ -451,6 +473,14 @@ func TestProcessLifecycle(t *testing.T) {
t.Run(test.Name, func(t *testing.T) {
assert := assert.New(t)
plugin := newTestPlugin(t)
if test.ExpectedResult != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
}

if test.ExpectedStatusCode == http.StatusOK {
plugin.metricsService.(*metricsmocks.Metrics).On("ObserveLifecycleEventTotal", mock.AnythingOfType("string")).Times(1)
}

test.SetupStore(plugin.store.(*storemocks.Store))
test.SetupAPI(plugin.API.(*plugintest.API))
test.SetupClient(plugin.msteamsAppClient.(*clientmocks.Client), plugin.clientBuilderWithToken("", "", "", "", nil, nil).(*clientmocks.Client))
Expand Down Expand Up @@ -801,6 +831,10 @@ func TestConnect(t *testing.T) {
t.Run(test.Name, func(t *testing.T) {
assert := assert.New(t)
plugin := newTestPlugin(t)
if test.ExpectedResult != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
}

mockAPI := &plugintest.API{}

plugin.SetAPI(mockAPI)
Expand Down Expand Up @@ -890,6 +924,10 @@ func TestGetConnectedUsers(t *testing.T) {
t.Run(test.Name, func(t *testing.T) {
assert := assert.New(t)
plugin := newTestPlugin(t)
if test.ExpectedResult != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
}

mockAPI := &plugintest.API{}

plugin.SetAPI(mockAPI)
Expand Down Expand Up @@ -979,6 +1017,10 @@ func TestGetConnectedUsersFile(t *testing.T) {
t.Run(test.Name, func(t *testing.T) {
assert := assert.New(t)
plugin := newTestPlugin(t)
if test.ExpectedResult != "" {
plugin.metricsService.(*metricsmocks.Metrics).On("IncrementHTTPErrors").Times(1)
}

mockAPI := &plugintest.API{}

plugin.SetAPI(mockAPI)
Expand Down
19 changes: 17 additions & 2 deletions server/handlers/attachments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
newText := text
parentID := ""
countAttachments := 0
manojmalik20 marked this conversation as resolved.
Show resolved Hide resolved
countFileAttachments := 0
var client msteams.Client
if chat == nil {
client = ah.plugin.GetClientForApp()
Expand All @@ -114,19 +115,26 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
return "", nil, "", errorFound
}

isDirect := isNotDirectMessage
if chat != nil {
isDirect = isDirectMessage
}

for _, a := range msg.Attachments {
// remove the attachment tags from the text
newText = attachRE.ReplaceAllString(newText, "")

// handle a code snippet (code block)
if a.ContentType == "application/vnd.microsoft.card.codesnippet" {
newText = ah.handleCodeSnippet(client, a, newText)
countAttachments++
continue
}

// handle a message reference (reply)
if a.ContentType == "messageReference" {
parentID, newText = ah.handleMessageReference(a, msg.ChatID+msg.ChannelID, newText)
countAttachments++
continue
}

Expand All @@ -143,19 +151,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().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, discardedReasonUnableToGetTeamsData, 1)
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().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, discardedReasonUnableToGetTeamsData, 1)
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().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, discardedReasonMaxFileSizeExceeded, 1)
continue
}

Expand All @@ -164,6 +175,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().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, discardedReasonUnableToGetTeamsData, 1)
continue
}
}
Expand All @@ -184,12 +196,15 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
}

if fileInfoID == "" {
ah.plugin.GetMetrics().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, discardedReasonEmptyFileID, 1)
continue
}
attachments = append(attachments, fileInfoID)
countAttachments++
if countAttachments == 10 {
ah.plugin.GetMetrics().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, "", 1)
countFileAttachments++
if countFileAttachments == 10 {
ayusht2810 marked this conversation as resolved.
Show resolved Hide resolved
ah.plugin.GetAPI().LogDebug("discarding the rest of the attachments as Mattermost supports only 10 attachments per post")
ah.plugin.GetMetrics().ObserveFilesCount(actionCreated, actionSourceMSTeams, isDirect, discardedReasonFileLimitReached, len(msg.Attachments)-countAttachments-countFileAttachments)
break
}
}
Expand Down
Loading
Loading