MM-56188: Add presence indicator to load test #678

agnivade · 2023-12-13T05:32:39Z

We send the message via the websocket whenever
the client state changes.

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

loadtest/user/userentity/websocket.go

agarciamontoro

I looked at the related server PR (this one, right?) to confirm that UpdateActiveThread does indeed need the channel ID. Why is that? Why don't we need the root post ID? It was quite confusing to see the header of the function being UpdateActiveThread(channelId string) and thought something was wrong.

agnivade · 2023-12-13T13:47:46Z

I am trying to keep the scope to the high level objects like - teamID, channelID. The rootID is embedded as a metadata to the typing event and is not a property in the WebSocketBroadcast struct. So yes it'll be slightly more wasteful but it keeps the overall logic simple.

We can move to tracking the actual thread maybe as a future perf improvement.

agarciamontoro

Thanks! The only ask I have would be to clarify with a comment in the code why `UpdateActiveThread receives a channel ID instead of a thread ID.

streamer45 · 2023-12-26T14:57:22Z

@agnivade In case you are wondering, I am mostly waiting for the server and client logic to be finalized before reviewing this.

We send the message via the websocket whenever the client state changes. https://mattermost.atlassian.net/browse/MM-56188

agnivade · 2024-01-17T06:24:16Z

@streamer45 @agarciamontoro - This can now be reviewed.

streamer45 · 2024-01-17T16:34:17Z

loadtest/control/simulcontroller/actions.go

+		// Setting the active thread to empty to allow the optimization to kick in early.
+		// We don't do this in the webapp to keep the code simple, but it's okay to do this in load-test
+		// to magnify the boost.


When in doubt I'd probably want the load-test simulation to perform worse than webapp to be on the safe side. I'd be okay with keeping this to prove the ideal improvements though.

I'm also a bit worried that we deviate from what the webapp does. Should we just remove this?

The problem is our simulation is not close enough to the webapp. We don't differentiate between loading a thread from RHS or from global thread view. It's all just a loadThread action. Until we differentiate that, we'll have to approximate somehow.

But are we making things more performant here with this piece of code? Or am I misunderstanding its goal and side effects?

Correct, we are making it more performant here.

That's what worries me (and what I think concerned Claudio as well). Is the performance improvement in here really needed?

I talked about this earlier in this comment: #678 (comment)

Until we do that, we have to approximate somehow. And if we choose not to set the active thread at all, then there will be no improvements at all, so it will be impossible to measure the impact of the change.

Ok, let's go with it then! Thank you for the patience :)

loadtest/user/userentity/websocket.go

streamer45

Looks good 👍

agarciamontoro

Looks great! Just blocking because I'm confused with a couple of things I prefer to discuss before merging.

agarciamontoro · 2024-01-19T16:08:49Z

loadtest/control/simulcontroller/actions.go

+		// Setting the active thread to empty to allow the optimization to kick in early.
+		// We don't do this in the webapp to keep the code simple, but it's okay to do this in load-test
+		// to magnify the boost.


I'm also a bit worried that we deviate from what the webapp does. Should we just remove this?

agarciamontoro · 2024-01-19T16:10:39Z

loadtest/user/userentity/websocket.go

+	// We don't really have a notion of RHS thread vs global thread in the load test.
+	// We either load a channel or load a thread. For now, we just set `is_thread_view`
+	// as both true and false to set the scope.
+	ue.dataChan <- threadPresenceMsg{
+		channelId:  channelId,
+		threadView: true,
+	}
+	ue.dataChan <- threadPresenceMsg{
+		channelId:  channelId,
+		threadView: false,
+	}


I don't understand this. What does the webapp do with these two messages?

The webapp has 2 different ways of loading a thread. One from RHS and one from global thread viewer. That means one can have 2 threads open at the same time. Since we don't simulate that, we are approximating it here.

See mattermost/mattermost#25794 for more details. And also the comment at the end of this section: https://mattermost.atlassian.net/wiki/spaces/~5de4a29d752c1b0d114f23c9/pages/2556788794/Rearchitecting+WebSocket+Events#Add-concept-of-%E2%80%9Cactive_channel%E2%80%9C-and-%E2%80%9Cactive_team%E2%80%9C-to-a-connection

Ah, I get it now, we're simply opening both threads, right? That makes sense, thanks!

Basically. Actually we are making the API call to open just a single thread. But we are setting the scope such that both threads are open.

agarciamontoro

🚀

agnivade · 2024-02-23T04:38:36Z

The feature PR is merged. Time to get this in!

agnivade added the 2: Dev Review Requires review by a core committer label Dec 13, 2023

agnivade requested review from agarciamontoro and streamer45 December 13, 2023 05:32

agnivade commented Dec 13, 2023

View reviewed changes

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

agnivade added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Dec 13, 2023

agarciamontoro reviewed Dec 13, 2023

View reviewed changes

agnivade requested a review from agarciamontoro December 18, 2023 05:46

agarciamontoro approved these changes Dec 22, 2023

View reviewed changes

agnivade added 2 commits January 17, 2024 10:37

MM-56188: Add presence indicator to load test

07c87b2

We send the message via the websocket whenever the client state changes. https://mattermost.atlassian.net/browse/MM-56188

using FF and also handling both RHS and global thread

dd969bd

agnivade force-pushed the addPresence branch from 5a09a72 to dd969bd Compare January 17, 2024 06:23

agnivade requested a review from agarciamontoro January 17, 2024 06:24

streamer45 reviewed Jan 17, 2024

View reviewed changes

use channel to pass messages

48a796b

agnivade requested a review from streamer45 January 18, 2024 05:28

streamer45 approved these changes Jan 18, 2024

View reviewed changes

agarciamontoro requested changes Jan 19, 2024

View reviewed changes

agarciamontoro self-requested a review January 31, 2024 12:36

agarciamontoro approved these changes Jan 31, 2024

View reviewed changes

agnivade added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Feb 23, 2024

agnivade merged commit 51e020f into master Feb 23, 2024
1 check passed

agnivade deleted the addPresence branch February 23, 2024 04:38

agnivade mentioned this pull request Sep 18, 2024

MM-60439: Set websocket scope early #796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-56188: Add presence indicator to load test #678

MM-56188: Add presence indicator to load test #678

agnivade commented Dec 13, 2023

agarciamontoro left a comment

agnivade commented Dec 13, 2023

agarciamontoro left a comment

streamer45 commented Dec 26, 2023

agnivade commented Jan 17, 2024

streamer45 Jan 17, 2024

agarciamontoro Jan 19, 2024

agnivade Jan 19, 2024

agarciamontoro Jan 19, 2024 •

edited

Loading

agnivade Jan 20, 2024

agarciamontoro Jan 29, 2024

agnivade Jan 30, 2024

agarciamontoro Jan 31, 2024

streamer45 left a comment

agarciamontoro left a comment

agarciamontoro Jan 19, 2024

agarciamontoro Jan 19, 2024

agnivade Jan 19, 2024

agnivade Jan 19, 2024

agarciamontoro Jan 19, 2024

agnivade Jan 20, 2024

agarciamontoro left a comment

agnivade commented Feb 23, 2024

MM-56188: Add presence indicator to load test #678

MM-56188: Add presence indicator to load test #678

Conversation

agnivade commented Dec 13, 2023

agarciamontoro left a comment

Choose a reason for hiding this comment

agnivade commented Dec 13, 2023

agarciamontoro left a comment

Choose a reason for hiding this comment

streamer45 commented Dec 26, 2023

agnivade commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro Jan 19, 2024 • edited Loading

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

streamer45 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

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

agarciamontoro left a comment

Choose a reason for hiding this comment

agnivade commented Feb 23, 2024

agarciamontoro Jan 19, 2024 •

edited

Loading