Add Channel bookmarks to loadtest #828

enahum · 2024-10-14T13:08:20Z

Summary

This PR adds some actions for the Channel Bookmarks feature

Ticket Link

https://mattermost.atlassian.net/browse/MM-59997

agnivade

Nice work on this Elias. Really appreciate this PR.

I've left a few comments to make them more consistent with how we add feature coverage.

Totally understand this is not where you usually contribute code, so things might not be very familiar. Me or Alejandro are happy to fix things ourselves if it gets too difficult. I think most of the stuff is nearly done, just some small things to fix here and there.

agnivade · 2024-10-16T06:52:07Z

api/agent_client_test.go

+			maxUsers := 10000
+			mmCfg.TeamSettings.MaxUsersPerTeam = &maxUsers


Ohhh my dear ..

https://hub.mattermost.com/private-core/pl/hih74enmn7bwunmy6eamgct67r

Let's fix it everywhere else as well :)

I also mentioned this additionally in the dev meeting, but probably I could have done a better job.

agnivade · 2024-10-16T06:55:33Z

cmd/ltctl/collect.go

@@ -220,7 +220,7 @@ func collect(config deployment.Config, deploymentId string, outputName string) e
 				if err := json.Unmarshal(input, &cfg); err != nil {
 					return nil, fmt.Errorf("failed to unmarshal MM configuration: %w", err)
 				}
-				cfg.Sanitize()
+				cfg.Sanitize([]*model.Manifest{})


Can just be nil

loadtest/control/simulcontroller/bookmarks.go

agnivade · 2024-10-16T07:00:36Z

loadtest/control/simulcontroller/controller.go

@@ -264,6 +264,30 @@ func getActionList(c *SimulController) []userAction {
 			frequency:        1.41,
 			minServerVersion: control.MinSupportedVersion, // 7.7.0
 		},
+		{
+			name:             "AddChannelBookmark",
+			run:              c.AddChannelBookmark,


Just a small thing: we don't export the method names for the actions. Let's unexport them.

agnivade · 2024-10-16T07:01:17Z

loadtest/control/simulcontroller/controller.go

+		{
+			name:             "AddChannelBookmark",
+			run:              c.AddChannelBookmark,
+			frequency:        0.0003,


I assume this is taken following: https://github.com/mattermost/mattermost-load-test-ng/blob/master/docs/coverage-frequency.md?

Yes and no.. basically following the frequency we get 0 as the value, now the reality is we have customers that based on their feedback they will rely on this feature way more than what we do on community and/or hub, thus giving it a bit of a bump.

We have typically taken data from our large cloud customer, not community/hub. I realize that it's not enabled in cloud yet, but that is the process we follow. It is fine to work with estimates for now, but we should track this future update in the ticket and paste the link in a comment.

I guess in this case once deployed we can actually ask p1 instead out largest cloud customer as they will be the one using the feature the most, at least that is my understanding

agnivade · 2024-10-16T07:04:53Z

loadtest/control/simulcontroller/bookmarks.go

+	currentBookmarks := u.Store().ChannelBookmarks(channel.Id)
+	if len(currentBookmarks) > 0 {
+		bookmark := currentBookmarks[rand.Intn(len(currentBookmarks))]
+		if bookmark != nil {


Same comment as above

agnivade · 2024-10-16T07:05:21Z

loadtest/control/simulcontroller/bookmarks.go

+		}
+	}
+
+	return control.UserActionResponse{Info: "no channel bookmarks deleted"}


Maybe a better string would be "no channel bookmarks found"?

agnivade · 2024-10-16T07:12:30Z

loadtest/control/utils.go

+	filenames := []string{"test_upload.png", "test_upload.jpg", "test_upload.mp4", "test_upload.txt"}
+	file := filenames[rand.Intn(len(filenames))]
+	data := MustAsset(file)
+	resp, err := u.UploadFile(data, bookmark.ChannelId, file)
+	if err != nil {
+		return err
+	}


I think this logic can be refactored into _attachFilesToObj with a little bit of work to simply upload a single file. And then we can just call _attachFilesToObj(u, bookmark.ChannelId, 1).

agnivade · 2024-10-16T07:22:04Z

loadtest/user/userentity/bookmarks.go

+	_, err := ue.getUserFromStore()
+	if err != nil {
+		return err
+	}


I don't think this is necessary here, or in any of the methods. We can remove this.

loadtest/store/memstore/store.go

agnivade

Nicely done. Left a small comment, but non-blocking.

agnivade · 2024-10-17T14:48:34Z

loadtest/control/utils.go

+		upload := rand.Intn(2) == 0 && qtyToUpload > count
+		if upload {
+			count += 1
+		}


Wondering if it would be simpler to break it into two?

upload := rand.Intn(2) == 0 if upload { count += 1 } files[filename] = &file{ data: MustAsset(filename), upload: upload, } if count == qtyToUpload { break }

agnivade · 2024-10-22T08:37:13Z

@agarciamontoro - please take a look whenever.

agarciamontoro

Thank you so much for this PR, @enahum, really appreciate all the work! I've left a bunch of comments, mostly nits, but this is fantastic!

go.mod

loadtest/control/actions.go

loadtest/control/simulcontroller/bookmarks.go

loadtest/control/utils.go

loadtest/store/memstore/store.go

loadtest/user/userentity/actions.go

loadtest/user/userentity/bookmarks.go

agarciamontoro

Thank you for addressing all my nits, @enahum! Something that was missed and I think we're good to go :)

loadtest/control/utils.go

agarciamontoro

🚀 Thank you!

agarciamontoro · 2024-10-24T13:59:03Z

@DHaussermann, do you have some spare cycles to test a simple deployment with this version to double-check all is good?

DHaussermann · 2024-10-25T16:13:15Z

@agarciamontoro Sorry, I only intened to add myself as a reviewer to get this visible in my GitHub work queue. I accidentally requested yours too 🤦

agarciamontoro

Haha, no worries! Approved again 😂

Add Channel bookmarks to loadtest

1770f69

enahum added the 2: Dev Review Requires review by a core committer label Oct 14, 2024

enahum requested review from agnivade and streamer45 October 14, 2024 13:08

agnivade requested review from agarciamontoro and removed request for streamer45 October 14, 2024 14:06

enahum added 4 commits October 16, 2024 07:23

update dependency to reflect changes in client4.go

93ce19d

fixes after model.NewString, model.NewBool and others were removed?

e18fd55

missing replacement of model.New...

319aaf0

add parameter to cfg.Sanitize

6d2cb32

agnivade reviewed Oct 16, 2024

View reviewed changes

enahum added 2 commits October 16, 2024 15:53

use model.NewPointer

3f2c74f

feedback review

b78d2d9

enahum requested a review from agnivade October 16, 2024 09:03

agnivade approved these changes Oct 17, 2024

View reviewed changes

feedback review 2

8255edb

agarciamontoro requested changes Oct 23, 2024

View reviewed changes

feedback review 3

5c455d8

enahum requested a review from agarciamontoro October 24, 2024 00:02

agarciamontoro requested changes Oct 24, 2024

View reviewed changes

loadtest/control/utils.go Outdated Show resolved Hide resolved

loadtest/control/utils.go Outdated Show resolved Hide resolved

upload 4 files for posts and drafts

6509a99

enahum requested a review from agarciamontoro October 24, 2024 13:55

agarciamontoro approved these changes Oct 24, 2024

View reviewed changes

agarciamontoro added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Oct 24, 2024

DHaussermann requested review from agarciamontoro and DHaussermann October 25, 2024 16:11

agarciamontoro approved these changes Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Channel bookmarks to loadtest #828

Add Channel bookmarks to loadtest #828

enahum commented Oct 14, 2024

agnivade left a comment

agnivade Oct 16, 2024

agnivade Oct 16, 2024

agnivade Oct 16, 2024

agnivade Oct 16, 2024

enahum Oct 16, 2024

agnivade Oct 16, 2024

enahum Oct 16, 2024

agnivade Oct 16, 2024

agnivade Oct 16, 2024

agnivade Oct 16, 2024

agnivade Oct 16, 2024

agnivade left a comment

agnivade Oct 17, 2024

agnivade commented Oct 22, 2024

agarciamontoro left a comment

agarciamontoro left a comment

agarciamontoro left a comment

agarciamontoro commented Oct 24, 2024

DHaussermann commented Oct 25, 2024 •

edited

Loading

agarciamontoro left a comment •

edited

Loading

		maxUsers := 10000
		mmCfg.TeamSettings.MaxUsersPerTeam = &maxUsers

Add Channel bookmarks to loadtest #828

Are you sure you want to change the base?

Add Channel bookmarks to loadtest #828

Conversation

enahum commented Oct 14, 2024

Summary

Ticket Link

agnivade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade commented Oct 22, 2024

agarciamontoro left a comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

agarciamontoro commented Oct 24, 2024

DHaussermann commented Oct 25, 2024 • edited Loading

agarciamontoro left a comment • edited Loading

Choose a reason for hiding this comment

DHaussermann commented Oct 25, 2024 •

edited

Loading

agarciamontoro left a comment •

edited

Loading