IntenseWebs/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2025-02-25 18:55:24 -06:00
Code Issues Packages Projects Releases Wiki Activity

More notification metrics fixes (#26889)

Browse Source 
* Explicitly have the client tell the server when it should expect an ACK

* Don't count missing profile errors for your own posts, added comment

* Fix test

* Make postedAck a parameter in WebSocketClient

* Snapshot fixes

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Devin Binnie 2024-04-25 15:07:12 -04:00 committed by GitHub
parent 5f1a357845
commit 60c15c821f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 44 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
server/channels/api4/websocket.go
View File

@ -17,6 +17,7 @@ import (
const (
connectionIDParam = "connection_id"
sequenceNumberParam = "sequence_number"
postedAckParam = "posted_ack"
)
func (api *API) InitWebSocket() {
@ -48,6 +49,7 @@ func connectWebSocket(c *Context, w http.ResponseWriter, r *http.Request) {
TFunc: c.AppContext.T,
Locale: "",
Active: true,
PostedAck: r.URL.Query().Get(postedAckParam) == "true",
}
// The WebSocket upgrade request coming from mobile is missing the
// user agent so we need to fallback on the session's metadata.

22
server/channels/app/notification.go
View File

@ -740,15 +740,19 @@ func (a *App) SendNotifications(c request.CTX, post *model.Post, team *model.Tea
// A user following a thread but had left the channel won't get a notification
// https://mattermost.atlassian.net/browse/MM-36769
if profileMap[uid] == nil {
a.CountNotificationReason(model.NotificationStatusError, model.NotificationTypeWebsocket, model.NotificationReasonMissingProfile)
a.NotificationsLog().Error("Missing profile",
mlog.String("type", model.NotificationTypeWebsocket),
mlog.String("post_id", post.Id),
mlog.String("status", model.NotificationStatusError),
mlog.String("reason", model.NotificationReasonMissingProfile),
mlog.String("sender_id", sender.Id),
mlog.String("receiver_id", uid),
)
// This also sometimes happens when bots, which will never show up in the map, reply to threads
// Their own post goes through this and they get "notified", which we don't need to count as an error if they can't
if uid != post.UserId {
a.CountNotificationReason(model.NotificationStatusError, model.NotificationTypeWebsocket, model.NotificationReasonMissingProfile)
a.NotificationsLog().Error("Missing profile",
mlog.String("type", model.NotificationTypeWebsocket),
mlog.String("post_id", post.Id),
mlog.String("status", model.NotificationStatusError),
mlog.String("reason", model.NotificationReasonMissingProfile),
mlog.String("sender_id", sender.Id),
mlog.String("receiver_id", uid),
)
}
continue
}
if a.IsCRTEnabledForUser(c, uid) {

3
server/channels/app/platform/web_conn.go
View File

@ -68,6 +68,7 @@ type WebConnConfig struct {
Active bool
ReuseCount int
OriginClient string
PostedAck bool
// These aren't necessary to be exported to api layer.
sequence int
@ -89,6 +90,7 @@ type WebConn struct {
Locale string
Sequence int64
UserId string
PostedAck bool
allChannelMembers map[string]string
lastAllChannelMembersTime int64
@ -234,6 +236,7 @@ func (ps *PlatformService) NewWebConn(cfg *WebConnConfig, suite SuiteIFace, runn
UserId: cfg.Session.UserId,
T: cfg.TFunc,
Locale: cfg.Locale,
PostedAck: cfg.PostedAck,
reuseCount: cfg.ReuseCount,
endWritePump: make(chan struct{}),
pumpFinished: make(chan struct{}),

8
server/channels/app/web_broadcast_hooks.go
View File

@ -82,11 +82,9 @@ func usePostedAckHook(message *model.WebSocketEvent, postedUserId string, channe
}
func (h *postedAckBroadcastHook) Process(msg *platform.HookedWebSocketEvent, webConn *platform.WebConn, args map[string]any) error {
// Don't ACK mobile app websocket events at this time, we may revisit this when we can add this to mobile app
if session := webConn.GetSession(); session != nil {
if session.IsMobile() {
return nil
}
// Don't ACK unless we say to explicitly
if !webConn.PostedAck {
return nil
}
postedUserId, err := getTypedArg[string](args, "posted_user_id")

13
server/channels/app/web_broadcast_hooks_test.go
View File

@ -87,8 +87,9 @@ func TestPostedAckHook_Process(t *testing.T) {
hook := &postedAckBroadcastHook{}
userID := model.NewId()
webConn := &platform.WebConn{
UserId: userID,
Platform: &platform.PlatformService{},
UserId: userID,
Platform: &platform.PlatformService{},
PostedAck: true,
}
webConn.SetSession(&model.Session{})
@ -140,12 +141,12 @@ func TestPostedAckHook_Process(t *testing.T) {
assert.True(t, msg.Event().GetData()["should_ack"].(bool))
})
t.Run("should not ack for mobile app", func(t *testing.T) {
t.Run("should not ack if posted ack is false", func(t *testing.T) {
mobileWebConn := &platform.WebConn{
UserId: userID,
Platform: &platform.PlatformService{},
UserId: userID,
Platform: &platform.PlatformService{},
PostedAck: false,
}
mobileWebConn.SetSession(&model.Session{Props: map[string]string{"isMobile": "true"}})
msg := platform.MakeHookedWebSocketEvent(model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, ""))
hook.Process(msg, mobileWebConn, map[string]any{

2
webapp/channels/src/actions/websocket_actions.jsx
View File

@ -179,7 +179,7 @@ export function initialize() {
WebSocketClient.addMissedMessageListener(restart);
WebSocketClient.addCloseListener(handleClose);
WebSocketClient.initialize(connUrl);
WebSocketClient.initialize(connUrl, undefined, true);
}
export function close() {

2
webapp/channels/src/components/post_view/post_body_additional_content/__snapshots__/post_body_additional_content.test.tsx.snap
View File

@ -111,6 +111,7 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c
"messageListeners": Set {},
"missedEventCallback": null,
"missedMessageListeners": Set {},
"postedAck": false,
"reconnectCallback": null,
"reconnectListeners": Set {},
"responseCallbacks": Object {},
@ -157,6 +158,7 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c
"messageListeners": Set {},
"missedEventCallback": null,
"missedMessageListeners": Set {},
"postedAck": false,
"reconnectCallback": null,
"reconnectListeners": Set {},
"responseCallbacks": Object {},

4
webapp/channels/src/plugins/pluggable/__snapshots__/pluggable.test.tsx.snap
View File

@ -115,6 +115,7 @@ exports[`plugins/Pluggable should match snapshot with extended component 1`] = `
"messageListeners": Set {},
"missedEventCallback": null,
"missedMessageListeners": Set {},
"postedAck": false,
"reconnectCallback": null,
"reconnectListeners": Set {},
"responseCallbacks": Object {},
@ -248,6 +249,7 @@ exports[`plugins/Pluggable should match snapshot with extended component with pl
"messageListeners": Set {},
"missedEventCallback": null,
"missedMessageListeners": Set {},
"postedAck": false,
"reconnectCallback": null,
"reconnectListeners": Set {},
"responseCallbacks": Object {},
@ -507,6 +509,7 @@ exports[`plugins/Pluggable should match snapshot with null pluggableId 1`] = `
"messageListeners": Set {},
"missedEventCallback": null,
"missedMessageListeners": Set {},
"postedAck": false,
"reconnectCallback": null,
"reconnectListeners": Set {},
"responseCallbacks": Object {},
@ -641,6 +644,7 @@ exports[`plugins/Pluggable should match snapshot with valid pluggableId 1`] = `
"messageListeners": Set {},
"missedEventCallback": null,
"missedMessageListeners": Set {},
"postedAck": false,
"reconnectCallback": null,
"reconnectListeners": Set {},
"responseCallbacks": Object {},

12
webapp/platform/client/src/websocket.ts
View File

@ -68,6 +68,7 @@ export default class WebSocketClient {
private closeListeners = new Set<CloseListener>();
private connectionId: string | null;
private postedAck: boolean;
constructor() {
this.conn = null;
@ -77,12 +78,13 @@ export default class WebSocketClient {
this.connectFailCount = 0;
this.responseCallbacks = {};
this.connectionId = '';
this.postedAck = false;
}
// on connect, only send auth cookie and blank state.
// on hello, get the connectionID and store it.
// on reconnect, send cookie, connectionID, sequence number.
initialize(connectionUrl = this.connectionUrl, token?: string) {
initialize(connectionUrl = this.connectionUrl, token?: string, postedAck?: boolean) {
if (this.conn) {
return;
}
@ -96,10 +98,14 @@ export default class WebSocketClient {
console.log('websocket connecting to ' + connectionUrl); //eslint-disable-line no-console
}
if (typeof postedAck != 'undefined') {
this.postedAck = postedAck;
}
// Add connection id, and last_sequence_number to the query param.
// We cannot use a cookie because it will bleed across tabs.
// We cannot also send it as part of the auth_challenge, because the session cookie is already sent with the request.
this.conn = new WebSocket(`${connectionUrl}?connection_id=${this.connectionId}&sequence_number=${this.serverSequence}`);
this.conn = new WebSocket(`${connectionUrl}?connection_id=${this.connectionId}&sequence_number=${this.serverSequence}${this.postedAck ? '&posted_ack=true' : ''}`);
this.connectionUrl = connectionUrl;
this.conn.onopen = () => {
@ -148,7 +154,7 @@ export default class WebSocketClient {
setTimeout(
() => {
this.initialize(connectionUrl, token);
this.initialize(connectionUrl, token, postedAck);
},
retryTime,
);