From 9cf5a4bccbbdbfdf748f2ef51c7ec5f38a14be09 Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Tue, 23 Apr 2024 17:31:17 -0400 Subject: [PATCH] Notification metrics fixes (#26854) * Ensure your own posts are never ACKed * Don't ACK mobile websocket notifications * Add counter for the unsupported Desktop Apps * Count only push messages when checking for acks * Fix generated * Add tests, fix comment * Fix help string * Check for nil session --- server/channels/api4/system.go | 4 ++- server/channels/app/notification.go | 2 ++ server/channels/app/notification_push.go | 4 +++ server/channels/app/web_broadcast_hooks.go | 19 ++++++++----- .../channels/app/web_broadcast_hooks_test.go | 18 +++++++++++++ server/einterfaces/metrics.go | 1 + server/einterfaces/mocks/MetricsInterface.go | 5 ++++ server/enterprise/metrics/metrics.go | 27 +++++++++++++++---- server/public/model/notification.go | 7 ++--- 9 files changed, 72 insertions(+), 15 deletions(-) diff --git a/server/channels/api4/system.go b/server/channels/api4/system.go index 0d8b53e88b..6f64d0717d 100644 --- a/server/channels/api4/system.go +++ b/server/channels/api4/system.go @@ -641,7 +641,9 @@ func pushNotificationAck(c *Context, w http.ResponseWriter, r *http.Request) { return } - c.App.CountNotificationAck(model.NotificationTypePush) + if ack.NotificationType == model.PushTypeMessage { + c.App.CountNotificationAck(model.NotificationTypePush) + } err := c.App.SendAckToPushProxy(&ack) if ack.IsIdLoaded { diff --git a/server/channels/app/notification.go b/server/channels/app/notification.go index 79580c7e7c..5d8b989a5b 100644 --- a/server/channels/app/notification.go +++ b/server/channels/app/notification.go @@ -1775,5 +1775,7 @@ func (a *App) CountNotificationReason( a.Metrics().IncrementNotificationErrorCounter(notificationType, notificationReason) case model.NotificationStatusNotSent: a.Metrics().IncrementNotificationNotSentCounter(notificationType, notificationReason) + case model.NotificationStatusUnsupported: + a.Metrics().IncrementNotificationUnsupportedCounter(notificationType, notificationReason) } } diff --git a/server/channels/app/notification_push.go b/server/channels/app/notification_push.go index e77af5618a..30cede7687 100644 --- a/server/channels/app/notification_push.go +++ b/server/channels/app/notification_push.go @@ -185,6 +185,10 @@ func (a *App) sendPushNotificationToAllSessions(msg *model.PushNotification, use if a.Metrics() != nil { a.Metrics().IncrementPostSentPush() } + + if msg.Type == model.PushTypeMessage { + a.CountNotification(model.NotificationTypePush) + } } return nil diff --git a/server/channels/app/web_broadcast_hooks.go b/server/channels/app/web_broadcast_hooks.go index d13ffce823..6b08942586 100644 --- a/server/channels/app/web_broadcast_hooks.go +++ b/server/channels/app/web_broadcast_hooks.go @@ -82,12 +82,11 @@ func usePostedAckHook(message *model.WebSocketEvent, postedUserId string, channe } func (h *postedAckBroadcastHook) Process(msg *platform.HookedWebSocketEvent, webConn *platform.WebConn, args map[string]any) error { - // Add if we have mentions or followers - // This works since we currently do have an order for broadcast hooks, but this probably should be reworked going forward - if msg.Get("followers") != nil || msg.Get("mentions") != nil { - msg.Add("should_ack", true) - incrementWebsocketCounter(webConn) - return nil + // 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 + } } postedUserId, err := getTypedArg[string](args, "posted_user_id") @@ -100,6 +99,14 @@ func (h *postedAckBroadcastHook) Process(msg *platform.HookedWebSocketEvent, web return nil } + // Add if we have mentions or followers + // This works since we currently do have an order for broadcast hooks, but this probably should be reworked going forward + if msg.Get("followers") != nil || msg.Get("mentions") != nil { + msg.Add("should_ack", true) + incrementWebsocketCounter(webConn) + return nil + } + channelType, err := getTypedArg[model.ChannelType](args, "channel_type") if err != nil { return errors.Wrap(err, "Invalid channel_type value passed to postedAckBroadcastHook") diff --git a/server/channels/app/web_broadcast_hooks_test.go b/server/channels/app/web_broadcast_hooks_test.go index 305bbb5cad..e199c3ca2c 100644 --- a/server/channels/app/web_broadcast_hooks_test.go +++ b/server/channels/app/web_broadcast_hooks_test.go @@ -90,6 +90,7 @@ func TestPostedAckHook_Process(t *testing.T) { UserId: userID, Platform: &platform.PlatformService{}, } + webConn.SetSession(&model.Session{}) t.Run("should ack if user is in the list of users to notify", func(t *testing.T) { msg := platform.MakeHookedWebSocketEvent(model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")) @@ -138,6 +139,23 @@ 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) { + mobileWebConn := &platform.WebConn{ + UserId: userID, + Platform: &platform.PlatformService{}, + } + 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{ + "posted_user_id": model.NewId(), + "channel_type": model.ChannelTypeDirect, + "users": []string{}, + }) + + assert.Nil(t, msg.Event().GetData()["should_ack"]) + }) } func TestAddMentionsAndAddFollowersHooks(t *testing.T) { diff --git a/server/einterfaces/metrics.go b/server/einterfaces/metrics.go index 2483451461..50cf797a0d 100644 --- a/server/einterfaces/metrics.go +++ b/server/einterfaces/metrics.go @@ -101,4 +101,5 @@ type MetricsInterface interface { IncrementNotificationSuccessCounter(notificationType model.NotificationType) IncrementNotificationErrorCounter(notificationType model.NotificationType, errorReason model.NotificationReason) IncrementNotificationNotSentCounter(notificationType model.NotificationType, notSentReason model.NotificationReason) + IncrementNotificationUnsupportedCounter(notificationType model.NotificationType, notSentReason model.NotificationReason) } diff --git a/server/einterfaces/mocks/MetricsInterface.go b/server/einterfaces/mocks/MetricsInterface.go index f7440510fd..c8cbdd0152 100644 --- a/server/einterfaces/mocks/MetricsInterface.go +++ b/server/einterfaces/mocks/MetricsInterface.go @@ -188,6 +188,11 @@ func (_m *MetricsInterface) IncrementNotificationSuccessCounter(notificationType _m.Called(notificationType) } +// IncrementNotificationUnsupportedCounter provides a mock function with given fields: notificationType, notSentReason +func (_m *MetricsInterface) IncrementNotificationUnsupportedCounter(notificationType model.NotificationType, notSentReason model.NotificationReason) { + _m.Called(notificationType, notSentReason) +} + // IncrementPostBroadcast provides a mock function with given fields: func (_m *MetricsInterface) IncrementPostBroadcast() { _m.Called() diff --git a/server/enterprise/metrics/metrics.go b/server/enterprise/metrics/metrics.go index 06d0a5e9f3..8c14651dec 100644 --- a/server/enterprise/metrics/metrics.go +++ b/server/enterprise/metrics/metrics.go @@ -203,11 +203,12 @@ type MetricsInterfaceImpl struct { JobsActive *prometheus.GaugeVec - NotificationTotalCounters *prometheus.CounterVec - NotificationAckCounters *prometheus.CounterVec - NotificationSuccessCounters *prometheus.CounterVec - NotificationErrorCounters *prometheus.CounterVec - NotificationNotSentCounters *prometheus.CounterVec + NotificationTotalCounters *prometheus.CounterVec + NotificationAckCounters *prometheus.CounterVec + NotificationSuccessCounters *prometheus.CounterVec + NotificationErrorCounters *prometheus.CounterVec + NotificationNotSentCounters *prometheus.CounterVec + NotificationUnsupportedCounters *prometheus.CounterVec } func init() { @@ -1109,6 +1110,18 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf ) m.Registry.MustRegister(m.NotificationNotSentCounters) + m.NotificationUnsupportedCounters = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: MetricsNamespace, + Subsystem: MetricsSubsystemNotifications, + Name: "unsupported", + Help: "Total number of untrackable notifications due to an unsupported app version", + ConstLabels: additionalLabels, + }, + []string{"type", "reason"}, + ) + m.Registry.MustRegister(m.NotificationUnsupportedCounters) + return m } @@ -1555,6 +1568,10 @@ func (mi *MetricsInterfaceImpl) IncrementNotificationNotSentCounter(notification mi.NotificationNotSentCounters.With(prometheus.Labels{"type": string(notificationType), "reason": string(notSentReason)}).Inc() } +func (mi *MetricsInterfaceImpl) IncrementNotificationUnsupportedCounter(notificationType model.NotificationType, notSentReason model.NotificationReason) { + mi.NotificationUnsupportedCounters.With(prometheus.Labels{"type": string(notificationType), "reason": string(notSentReason)}).Inc() +} + func (mi *MetricsInterfaceImpl) IncrementHTTPWebSockets(originClient string) { mi.HTTPWebsocketsGauge.With(prometheus.Labels{"origin_client": originClient}).Inc() } diff --git a/server/public/model/notification.go b/server/public/model/notification.go index eb511fcac5..c959aa5fda 100644 --- a/server/public/model/notification.go +++ b/server/public/model/notification.go @@ -8,9 +8,10 @@ type NotificationType string type NotificationReason string const ( - NotificationStatusSuccess NotificationStatus = "success" - NotificationStatusError NotificationStatus = "error" - NotificationStatusNotSent NotificationStatus = "not_sent" + NotificationStatusSuccess NotificationStatus = "success" + NotificationStatusError NotificationStatus = "error" + NotificationStatusNotSent NotificationStatus = "not_sent" + NotificationStatusUnsupported NotificationStatus = "unsupported" NotificationTypeAll NotificationType = "all" NotificationTypeEmail NotificationType = "email"