-
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
MM-56188: Add presence indicator to load test #678
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,7 @@ start: | |
connectionFailCount++ | ||
select { | ||
// Draining the channel to avoid blocking the sender. | ||
case <-ue.wsTyping: | ||
case <-ue.dataChan: | ||
case <-ue.wsClosing: | ||
// Explicit disconnect. Return. | ||
close(ue.wsClosed) | ||
|
@@ -189,13 +189,28 @@ start: | |
// Explicit disconnect. Return. | ||
close(ue.wsClosed) | ||
return | ||
case msg, ok := <-ue.wsTyping: | ||
case msg, ok := <-ue.dataChan: | ||
if !ok { | ||
chanClosed = true | ||
break | ||
} | ||
if err := client.UserTyping(msg.channelId, msg.parentId); err != nil { | ||
errChan <- fmt.Errorf("userentity: error in client.UserTyping: %w", err) | ||
switch v := msg.(type) { | ||
case userTypingMsg: | ||
if err := client.UserTyping(v.channelId, v.parentId); err != nil { | ||
errChan <- fmt.Errorf("userentity: error in client.UserTyping: %w", err) | ||
} | ||
case threadPresenceMsg: | ||
if err := client.UpdateActiveThread(v.channelId, v.threadView); err != nil { | ||
errChan <- fmt.Errorf("userentity: error in client.UpdateActiveThread: %w", err) | ||
} | ||
case channelPresenceMsg: | ||
if err := client.UpdateActiveChannel(v.channelId); err != nil { | ||
errChan <- fmt.Errorf("userentity: error in client.UpdateActiveChannel: %w", err) | ||
} | ||
case teamPresenceMsg: | ||
if err := client.UpdateActiveTeam(v.teamId); err != nil { | ||
errChan <- fmt.Errorf("userentity: error in client.UpdateActiveTeam: %w", err) | ||
} | ||
} | ||
} | ||
if chanClosed { | ||
|
@@ -209,7 +224,7 @@ start: | |
connectionFailCount++ | ||
select { | ||
// Draining the channel to avoid blocking the sender. | ||
case <-ue.wsTyping: | ||
case <-ue.dataChan: | ||
case <-ue.wsClosing: | ||
// Explicit disconnect. Return. | ||
close(ue.wsClosed) | ||
|
@@ -239,9 +254,47 @@ func (ue *UserEntity) SendTypingEvent(channelId, parentId string) error { | |
if !ue.connected { | ||
return errors.New("user is not connected") | ||
} | ||
ue.wsTyping <- userTypingMsg{ | ||
channelId, | ||
parentId, | ||
ue.dataChan <- userTypingMsg{ | ||
channelId: channelId, | ||
parentId: parentId, | ||
} | ||
return nil | ||
} | ||
|
||
func (ue *UserEntity) UpdateActiveChannel(channelId string) error { | ||
if !ue.connected { | ||
return errors.New("user is not connected") | ||
} | ||
ue.dataChan <- channelPresenceMsg{ | ||
channelId: channelId, | ||
} | ||
return nil | ||
} | ||
|
||
func (ue *UserEntity) UpdateActiveThread(channelId string) error { | ||
if !ue.connected { | ||
return errors.New("user is not connected") | ||
} | ||
// 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, | ||
} | ||
Comment on lines
+278
to
+288
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this. What does the webapp do with these two messages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I get it now, we're simply opening both threads, right? That makes sense, thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return nil | ||
} | ||
|
||
func (ue *UserEntity) UpdateActiveTeam(teamId string) error { | ||
if !ue.connected { | ||
return errors.New("user is not connected") | ||
} | ||
ue.dataChan <- teamPresenceMsg{ | ||
teamId: teamId, | ||
} | ||
return 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.
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.
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'm also a bit worried that we deviate from what the webapp does. Should we just 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.
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.
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.
But are we making things more performant here with this piece of code? Or am I misunderstanding its goal and side effects?
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.
Correct, we are making it more performant here.
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.
That's what worries me (and what I think concerned Claudio as well). Is the performance improvement in here really needed?
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 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.
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.
Ok, let's go with it then! Thank you for the patience :)