-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
Add Channel bookmarks to loadtest #828
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
api/agent_client_test.go
Outdated
maxUsers := 10000 | ||
mmCfg.TeamSettings.MaxUsersPerTeam = &maxUsers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cmd/ltctl/collect.go
Outdated
@@ -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{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just be nil
@@ -264,6 +264,30 @@ func getActionList(c *SimulController) []userAction { | |||
frequency: 1.41, | |||
minServerVersion: control.MinSupportedVersion, // 7.7.0 | |||
}, | |||
{ | |||
name: "AddChannelBookmark", | |||
run: c.AddChannelBookmark, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small thing: we don't export the method names for the actions. Let's unexport them.
{ | ||
name: "AddChannelBookmark", | ||
run: c.AddChannelBookmark, | ||
frequency: 0.0003, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is taken following: https://github.com/mattermost/mattermost-load-test-ng/blob/master/docs/coverage-frequency.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
currentBookmarks := u.Store().ChannelBookmarks(channel.Id) | ||
if len(currentBookmarks) > 0 { | ||
bookmark := currentBookmarks[rand.Intn(len(currentBookmarks))] | ||
if bookmark != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
} | ||
} | ||
|
||
return control.UserActionResponse{Info: "no channel bookmarks deleted"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a better string would be "no channel bookmarks found"?
loadtest/control/utils.go
Outdated
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
.
_, err := ue.getUserFromStore() | ||
if err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary here, or in any of the methods. We can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done. Left a small comment, but non-blocking.
loadtest/control/utils.go
Outdated
upload := rand.Intn(2) == 0 && qtyToUpload > count | ||
if upload { | ||
count += 1 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
@agarciamontoro - please take a look whenever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing all my nits, @enahum! Something that was missed and I think we're good to go :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Thank you!
@DHaussermann, do you have some spare cycles to test a simple deployment with this version to double-check all is good? |
@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 🤦 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, no worries! Approved again 😂
Summary
This PR adds some actions for the Channel Bookmarks feature
Ticket Link
https://mattermost.atlassian.net/browse/MM-59997