Scheduled post #848

harshilsharma63 · 2024-11-14T06:29:09Z

Summary

Adding scheduled posts support to load test tool.

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-61486

agnivade

It looks like some debug fmt.Println statements are still left over from debugging. Let's clean them up.

agarciamontoro

Thank you so much for all your work on this, great job! I left some comments below, and I also agree with Agniva: we should remove all the fmt.Println lines.

loadtest/control/simulcontroller/controller.go

loadtest/control/simulcontroller/scheduled_posts.go

loadtest/store/memstore/random.go

loadtest/user/userentity/scheduled_posts.go

loadtest/control/simulcontroller/scheduled_posts.go

loadtest/utils.go

loadtest/store/memstore/store.go

loadtest/store/memstore/random.go

harshilsharma63 · 2024-12-02T05:10:41Z

@agnivade @agarciamontoro can you please re-review this? I've made the fixes.

agnivade

I will leave the store logic to Alejandro.

loadtest/store/store.go

loadtest/user/userentity/scheduled_posts.go

loadtest/utils.go

agnivade

Looking good!

agarciamontoro

Thanks, @harshilsharma63, and sorry for the delay in the review! Some more comments below.

loadtest/control/simulcontroller/scheduled_posts.go

loadtest/store/memstore/store.go

loadtest/user/userentity/scheduled_posts.go

loadtest/store/memstore/store.go

agarciamontoro · 2024-12-10T15:06:03Z

loadtest/user/userentity/scheduled_posts.go

+	ue.Store().DeleteScheduledPost(scheduledPostId)
+	return nil


Right now DeleteScheduledPost doesn't return an error, but maybe it should (I left a comment for that). If we decide that's a good decision, then we need to return its error.

I don't think its of any use returning error from delete method, it doesn't matter

loadtest/user/userentity/scheduled_posts.go

loadtest/utils.go

agarciamontoro

Thanks, @harshilsharma63! I've left a comment in the RandomFutureTime function, and I have an additional ask: I do think we should error out in MemStore's DeleteScheduledPost and UpdateScheduledPost if the post is not found, and log an error if that's the case. It shouldn't happen and may point to other problems in the load-test.

loadtest/utils.go

agnivade · 2024-12-23T07:48:55Z

Both Alejandro and Claudio are out. I will take another quick look and ask for a stamp approval.

agnivade · 2024-12-23T09:32:27Z

I gave it a test run and found some minor issues. We should be good to go after that.

1. Incorrect error/info logging when there are no scheduled posts.

Log line is:

error [2024-12-23 14:51:17.856 +05:30] control/simulcontroller.(*SimulController).sendScheduledPostNow loadtest/control/simulcontroller/scheduled_posts.go:135 no scheduled posts available caller="loadtest/loadte
st.go:60" controller_id=4 user_id=anoxyyicofbozjrig7og95i6py
...
error [2024-12-23 14:51:34.667 +05:30] control/simulcontroller.(*SimulController).deleteScheduledPost loadtest/control/simulcontroller/scheduled_posts.go:113 no scheduled posts available caller="loadtest/loadtes
t.go:60" controller_id=10 user_id=bpjjn1f67jg8xnya68m41ki6dc

Note that the log level is error, but it's just a case of scheduled posts not being available. And we are correctly doing it here:

scheduledPost, err := u.Store().GetRandomScheduledPost()
if err != nil {
return control.UserActionResponse{Err: control.NewUserError(err)}
}
if scheduledPost == nil {
return control.UserActionResponse{Info: "no scheduled posts found"}
}

But the issue is that GetRandomScheduledPost() already returns an error if no scheduled posts are found.

The right way is to return an error variable and check for it. See for example RandomTeam:

	if len(teams) == 0 {
		return model.Team{}, ErrTeamStoreEmpty
	}

and then we check for the error at call site:

team, err := u.Store().RandomTeam(store.SelectMemberOf | store.SelectNotCurrent)
if errors.Is(err, memstore.ErrTeamStoreEmpty) {
return control.UserActionResponse{Info: "no other team to switch to"}
} else if err != nil {
return control.UserActionResponse{Err: control.NewUserError(err)}
}

We should apply the same logic while calling GetRandomScheduledPost as well.

2. Invalid scheduled at time:

error [2024-12-23 14:52:18.420 +05:30] control/simulcontroller.(*SimulController).createScheduledPost loadtest/control/simulcontroller/scheduled_posts.go:63 Invalid scheduled at time., id=m9txr5p37bf63kg4xwq7upjxcy caller="loadtest/loadtest.go:60" controller_id=9 user_id=b1ysdcjc5prodfef1386jdjawy

This is what I see in the server logs:

debug [2024-12-23 15:01:16.543 +05:30] Invalid scheduled at time.                    caller="web/context.go:120" path=/api/v4/posts/schedule request_id=xzx84obdgjgdugma4setb8a1ye ip_addr=127.0.0.1 user_id=eh1k7cuhmtntfx8y9ziihatmkr method=POST err_where=ScheduledPost.IsValid http_code=400 error="ScheduledPost.IsValid: Invalid scheduled at time., id=qoafc3rp3tf5zjsqpcck9ew55w"

If we fix the above 2 issues, we should be good to go.

agarciamontoro · 2025-01-08T10:11:25Z

Thanks for taking a look, @agnivade!

harshilsharma63 · 2025-01-08T10:15:31Z

I'll address the change next week and I'm getting the edit file functionality ready for feature complete by Monday

agarciamontoro · 2025-01-08T16:11:05Z

Got it, @harshilsharma63, and thank you for your work here!

agnivade · 2025-01-27T04:30:10Z

@harshilsharma63 - Just checking to see the status on this. I think there are just a few minor things pending, and then we should be good to merge this. So hopefully not a lot of effort required.

harshilsharma63 · 2025-01-27T04:43:21Z

@harshilsharma63 - Just checking to see the status on this. I think there are just a few minor things pending, and then we should be good to merge this. So hopefully not a lot of effort required.

I'm working to finish scheduled posts on mobile before the feature complete date so have deferred this until that. Should be done in Feb first half.

agnivade · 2025-01-27T06:18:55Z

Thanks. Ideally, we should not be releasing features which are not load tested. @streamer45 - wondering if you can help bump up the priority on this so that Harshil is free to wrap up the work?

streamer45

Looks good overhaul. Left mostly minor suggestions.

One additional comment is that I don't see any websocket addition here, and wondering how we are keeping the state in sync across multiple clients (e.g. deleting a draft).

streamer45 · 2025-01-27T15:29:49Z

loadtest/control/actions.go

+func ScheduledPostsEnabled(u user.User) (bool, UserActionResponse) {
+	allow, err := strconv.ParseBool(u.Store().ClientConfig()["ScheduledPosts"])
+	if err != nil {
+		fmt.Println("Error parsing ScheduledPosts config", err)


Unnecessary log?

yeah, removed.

streamer45 · 2025-01-27T15:32:00Z

loadtest/control/simulcontroller/scheduled_posts.go

+		post, err := u.Store().RandomPostForChannel(channel.Id)
+		if err == nil {


We should handle the error case.

streamer45 · 2025-01-27T15:36:37Z

loadtest/control/simulcontroller/scheduled_posts.go

+		return control.UserActionResponse{Err: control.NewUserError(err)}
+	}
+
+	message, err := createMessage(u, channel, false)


I assume we can schedule replies as well. A case to consider adding.

Done, but this doesn't matter as all thats happening from isReply param is changing the avg word length, nothing else The part that send scheduled post as a reply is above where I set the rootId. But fixed this anyways as it is now semantically correct.

streamer45 · 2025-01-27T15:40:33Z

loadtest/utils_test.go

+	}
+}
+
+func TestRandomFutureTimeZeroDuration(t *testing.T) {


nit: I'd expect these to be subcases of TestRandomFutureTime rather than top-level tests.

streamer45 · 2025-01-27T15:41:40Z

loadtest/utils_test.go

+	if randomTime < start.Unix() || randomTime > end.Unix() {
+		t.Errorf("RandomFutureTime() = %v, want between %v and %v", randomTime, start.Unix(), end.Unix())
+	}


We may want to use the require package for these checks.

streamer45 · 2025-01-27T15:45:43Z

loadtest/utils.go

+// RandomFutureTime returns a random Unix timestamp, in milliseconds, in the interval
+// [now+deltaStart, now+deltaStart+maxUntil]
+func RandomFutureTime(deltaStart, maxUntil time.Duration) int64 {


I'd find it more versatile if we returned a time.Time and then let the caller format it as needed. Otherwise, if we can only foresee a timestamp usage, we could rename this to reflect its behavior better.

There's just one use case of this function right now and I don't want to spend excessive time refactoring this to accommodate all possible cases. This is fine for now can can be updated later as needed. I've already refactored this twice.

streamer45 · 2025-01-27T15:46:38Z

loadtest/control/simulcontroller/scheduled_posts.go

+			RootId:    rootId,
+			CreateAt:  model.GetMillis(),
+		},
+		ScheduledAt: loadtest.RandomFutureTime(time.Hour*24*2, time.Hour*24*10),


I'd really welcome named constants to make it straightforward to read, e.g. TwoDays, TenDays :)

We'd end up creating constants for every day. Skipping this for now.

streamer45 · 2025-01-27T15:48:55Z

loadtest/control/simulcontroller/scheduled_posts.go

+		return control.UserActionResponse{Info: "scheduled posts not enabled"}
+	}
+
+	scheduledPost, err := u.Store().GetRandomScheduledPost()


Are we ensuring GetRandomScheduledPost() always returns non-deleted posts, if any?

While there is no explicit check for it, just like in other random functions, I am removing scheduled posts from store when deleting or posting them, so it will not return a deleted scheduled post.

agnivade · 2025-02-19T15:03:39Z

@harshilsharma63 - Just wanted to check on this. Would you be able to find some time to address the review comments?

harshilsharma63 · 2025-02-19T15:06:22Z

@agnivade will do this tomorrow. Noted.

harshilsharma63 · 2025-02-20T05:32:35Z

@agnivade I've fixed the comments and invalid time error.

agnivade · 2025-02-20T16:14:18Z

Thanks!

streamer45 · 2025-02-20T16:27:04Z

@harshilsharma63 Thoughts on my top comment above? On the websocket part.

harshilsharma63 · 2025-02-21T09:03:28Z

One additional comment is that I don't see any websocket addition here, and wondering how we are keeping the state in sync across multiple clients (e.g. deleting a draft).

@agnivade @streamer45 I didn't know we had to do it. Can you share an example where this is being done and I'll refer it? Can you also explain a bit more about this? This is the first time I'm adding something to load test so I don't know the conventions and what all stuff needs to be done. I followed a channel bookmark example.

agnivade · 2025-02-21T09:29:55Z

Basically look at the simulcontroller/websocket.go file. We need to add event handlers for scheduled post events and keep the load-test agent state up to date. Although we don't do it for a lot of other events. But I agree that we should start doing it.

streamer45 · 2025-02-24T17:29:23Z

Yeah, the idea is to mimic a real client's behavior as much as possible including any synchronization logic that may happen as a result of receiving a websocket event. Usually, I'd expect a Redux handler to be in place on the web implementation, which on this side can be converted to use the local store. See how we handle the posted event as a practical example:

mattermost-load-test-ng/loadtest/user/userentity/websocket.go

Lines 70 to 96 in 4954198

    
           func (ue *UserEntity) handlePostEvent(ev *model.WebSocketEvent) error { 
        
           	var data string 
        
           	if el, ok := ev.GetData()["post"]; !ok { 
        
           		return errors.New("post data is missing") 
        
           	} else if data, ok = el.(string); !ok { 
        
           		return fmt.Errorf("type of the post data should be a string, but it is %T", el) 
        
           	} 
        
           	var post *model.Post 
        
           	if err := json.Unmarshal([]byte(data), &post); err != nil { 
        
           		return err 
        
           	} 
        
           	switch ev.EventType() { 
        
           	case model.WebsocketEventPosted, model.WebsocketEventPostEdited: 
        
           		currentChannel, err := ue.store.CurrentChannel() 
        
           		if err == nil && currentChannel.Id == post.ChannelId { 
        
           			return ue.store.SetPost(post) 
        
           		} else if err != nil && !errors.Is(err, memstore.ErrChannelNotFound) { 
        
           			return fmt.Errorf("failed to get current channel from store: %w", err) 
        
           		} 
        
           	case model.WebsocketEventPostDeleted: 
        
           		return ue.store.DeletePost(post.Id) 
        
           	} 
        
           	return nil 
        
           }

mattermost-load-test-ng/loadtest/control/simulcontroller/websocket.go

Lines 32 to 62 in 4954198

    
           case model.WebsocketEventPosted: 
        
           	post, err := getPostFromEvent(ev) 
        
           	if err != nil { 
        
           		c.status <- c.newErrorStatus(fmt.Errorf("failed to get post from event: %w", err)) 
        
           		break 
        
           	} 
        
           	if ack, ok := ev.GetData()["should_ack"]; ok && ack.(bool) { 
        
           		if err := c.user.PostedAck(post.Id, "success", "", ""); err != nil { 
        
           			c.status <- c.newErrorStatus(err) 
        
           			break 
        
           		} 
        
           	} 
        
           	cm, _ := c.user.Store().ChannelMember(post.ChannelId, c.user.Store().Id()) 
        
           	if cm.UserId != "" { 
        
           		break 
        
           	} 
        
           	c.status <- c.newInfoStatus(fmt.Sprintf("channel member for post's channel missing from store, fetching: %q", post.ChannelId)) 
        
           	if err := c.user.GetChannelMember(post.ChannelId, c.user.Store().Id()); err != nil { 
        
           		c.status <- c.newErrorStatus(fmt.Errorf("GetChannelMember failed: %w", err)) 
        
           		break 
        
           	} 
        
           	// If we were to follow webapp literally we'd have to check against user's 
        
           	// preferences and possibly reload direct/group channels (and users) if 
        
           	// necessary. However this only happens if the preferences for these 
        
           	// channels are set not to show them, something we don't support yet as we default to 
        
           	// showing them.

You can notice we have two main places to handle ws events, based on what needs to happen. If we just need to save the data to the store, then we modify the internal userentity/websocket.go file. If we need to execute some actions as a result of receiving the event, then we'd be touching the simulcontroller/websocket.go.

agnivade · 2025-02-25T03:51:13Z

@streamer45 - what are your thoughts on splitting the ws event handler to another PR? This PR has been up for 4 months and quite big already. I am thinking if we could merge this and send another PR after this for the ws handler. That way atleast we start getting some feature coverage for scheduled posts.

streamer45 · 2025-02-25T14:30:34Z

@streamer45 - what are your thoughts on splitting the ws event handler to another PR? I am thinking if we could merge this and send another PR after this for the ws handler. That way atleast we start getting some feature coverage for scheduled posts.

Not opposed. Mostly I'd like to understand (from someone who developed the functionality) what ws events we need to handle and what the handling looks like. Do we make additional API requests in response? Is it just about saving data to the store?

These questions help me understand what the performance impact of leaving these out could be.

This PR has been up for 4 months and quite big already.

I'd encourage us to understand more about that (I have a few thoughts, e.g. the choice of 3 reviewers). Maybe a quick retro would be beneficial since we want to make this process as seamless as possible.

I'll remove myself from review to unblock and let @agarciamontoro do a final pass. Thanks for your patience :)

harshilsharma63 · 2025-02-26T04:11:53Z

@streamer45

Mostly I'd like to understand (from someone who developed the functionality) what ws events we need to handle and what the handling looks like. Do we make additional API requests in response? Is it just about saving data to the store?

just to keep state updated, thats the only purpose of WS handling.

I'd encourage us to understand more about that (I have a few thoughts, e.g. the choice of 3 reviewers). Maybe a quick retro would be beneficial since we want to make this process as seamless as possible.

The PR has been up for 4 months because I started working on another task and couldn't stop working on it to get back to this.

agnivade · 2025-02-26T06:16:58Z

I'd encourage us to understand more about that (I have a few thoughts, e.g. the choice of 3 reviewers).

To be clear, I wasn't explicitly looking for your review when I said the following 😛

@streamer45 - wondering if you can help bump up the priority on this so that Harshil is free to wrap up the work?

I was hoping you could have a chat with the leads so that Harshil gets enough bandwidth to prioritise this load-test work. Otherwise, he was constantly getting pulled into developing other features. But you did a review, so no harm done 😄

@agarciamontoro - could you give this a final pass? Thanks.

@harshilsharma63 - As Claudio outlined here: #848 (comment), we can set up event handlers for those ws events and update the store of the load-test agent to keep the state in sync. Let's schedule that work as a separate PR.

harshilsharma63 added 11 commits October 28, 2024 15:40

Added create scheduled posts

22cdf28

Addded UpdateScheduledPost

4341570

DeleteScheduledPost

f51ece6

SendScheduledPost

28a8187

Updated loadTeam

18fd076

WIP

1e816df

WIP

a89e45f

WIP

532675b

Merge branch 'master' into scheduled_post

4f500e3

Cleanup

6394486

Comment cleanup

d37d54e

harshilsharma63 marked this pull request as ready for review November 14, 2024 07:01

harshilsharma63 requested review from agarciamontoro and streamer45 November 14, 2024 07:01

agnivade reviewed Nov 16, 2024

View reviewed changes

agarciamontoro requested changes Nov 18, 2024

View reviewed changes

agnivade requested changes Nov 20, 2024

View reviewed changes

loadtest/store/memstore/store.go Outdated Show resolved Hide resolved

loadtest/store/memstore/random.go Outdated Show resolved Hide resolved

harshilsharma63 added 3 commits November 21, 2024 14:08

Merge branch 'master' into scheduled_post

b862d79

Merge branch 'master' into scheduled_post

3e0bf55

review fixes

2e439ab

harshilsharma63 requested review from agnivade and agarciamontoro November 27, 2024 04:44

agnivade reviewed Dec 2, 2024

View reviewed changes

loadtest/store/store.go Outdated Show resolved Hide resolved

loadtest/user/userentity/scheduled_posts.go Outdated Show resolved Hide resolved

loadtest/utils.go Outdated Show resolved Hide resolved

harshilsharma63 added 3 commits December 9, 2024 06:34

Merge branch 'master' into scheduled_post

a48e09a

Cleanup

1e2606e

Cleanup

8096d4a

harshilsharma63 requested a review from agnivade December 9, 2024 02:57

agnivade approved these changes Dec 9, 2024

View reviewed changes

agarciamontoro requested changes Dec 10, 2024

View reviewed changes

agarciamontoro requested changes Dec 19, 2024

View reviewed changes

loadtest/utils.go Outdated Show resolved Hide resolved

harshilsharma63 added 2 commits December 23, 2024 11:17

Merge branch 'master' into scheduled_post

f249da9

Updated RandomFutureTime

fe0df4b

harshilsharma63 requested a review from agarciamontoro December 23, 2024 06:07

streamer45 reviewed Jan 27, 2025

View reviewed changes

harshilsharma63 added 3 commits February 20, 2025 09:31

Merge branch 'master' into scheduled_post

80c0b97

review fixes

a0eca8f

fix time util to return milliseconds, not seconds

6a4e141

agnivade requested a review from streamer45 February 20, 2025 16:14

streamer45 removed their request for review February 25, 2025 14:30

		post, err := u.Store().RandomPostForChannel(channel.Id)
		if err == nil {

Scheduled post #848

Are you sure you want to change the base?

Scheduled post #848

Conversation

harshilsharma63 commented Nov 14, 2024

Summary

Ticket Link

agnivade left a comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

harshilsharma63 commented Dec 2, 2024

agnivade left a comment

Choose a reason for hiding this comment

agnivade left a comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

agnivade commented Dec 23, 2024

agnivade commented Dec 23, 2024

agarciamontoro commented Jan 8, 2025

harshilsharma63 commented Jan 8, 2025

agarciamontoro commented Jan 8, 2025

agnivade commented Jan 27, 2025

harshilsharma63 commented Jan 27, 2025

agnivade commented Jan 27, 2025

streamer45 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

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 commented Feb 19, 2025

harshilsharma63 commented Feb 19, 2025

harshilsharma63 commented Feb 20, 2025

agnivade commented Feb 20, 2025

streamer45 commented Feb 20, 2025

harshilsharma63 commented Feb 21, 2025 • edited Loading

agnivade commented Feb 21, 2025

streamer45 commented Feb 24, 2025

agnivade commented Feb 25, 2025

streamer45 commented Feb 25, 2025

harshilsharma63 commented Feb 26, 2025

agnivade commented Feb 26, 2025

harshilsharma63 commented Feb 21, 2025 •

edited

Loading