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

@@ -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 {

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)
 	}
 }

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

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")

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) {

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)
 }

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()

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()
 }

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"