gundeck: Remove bulkPush config option (#4290)

It has been set to `true` for a few years now and seems to work well. It is
being removed to reduce work for RabbitMq based notifications.

Related: #4272
Also: https://wearezeta.atlassian.net/browse/WPB-10308

changelog.d/0-release-notes/gundeck-bulk-push

@@ -0,0 +1,3 @@
+Config value `gundeck.config.bulkPush` has been removed. This is purely an
+internal change, in case the value was overriden to `false`, operators might see
+more spiky usage of CPU and memory from gundeck due to bulk processing.

charts/gundeck/templates/configmap.yaml

@@ -68,7 +68,6 @@ data:
     settings:
       httpPoolSize: 1024
       notificationTTL: {{ required "config.notificationTTL" .notificationTTL }}
-      bulkPush: {{ .bulkPush }}
       {{- if hasKey . "perNativePushConcurrency" }}
       perNativePushConcurrency: {{ .perNativePushConcurrency }}
       {{- end }}

charts/gundeck/values.yaml

@@ -56,7 +56,6 @@ config:
   # # tlsCaSecretRef:
   # #   name: <secret-name>
   # #   key: <ca-attribute>
-  bulkPush: true
   aws:
     region: "eu-west-1"
   proxy: {}

hack/helm_vars/wire-server/values.yaml.gotmpl

@@ -373,7 +373,6 @@ gundeck:
       sqsEndpoint: http://fake-aws-sqs:4568
       snsEndpoint: http://fake-aws-sns:4575
     disabledAPIVersions: []
-    bulkPush: true
     setMaxConcurrentNativePushes:
       hard: 30
       soft: 10

services/gundeck/gundeck.integration.yaml

@@ -37,7 +37,6 @@ aws:
 settings:
   httpPoolSize: 1024
   notificationTTL: 24192200
-  bulkPush: true
   perNativePushConcurrency: 32
   sqsThrottleMillis: 1000
   maxConcurrentNativePushes:

services/gundeck/src/Gundeck/Options.hs

@@ -57,9 +57,6 @@ data Settings = Settings
     _httpPoolSize :: !Int,
     -- | TTL (seconds) of stored notifications
     _notificationTTL :: !NotificationTTL,
-    -- | Use this option to group push notifications and send them in bulk to Cannon, instead
-    -- of in individual requests
-    _bulkPush :: !Bool,
     -- | Maximum number of concurrent threads calling SNS.
     _maxConcurrentNativePushes :: !(Maybe MaxConcurrentNativePushes),
     -- | Maximum number of parallel requests to SNS and cassandra

services/gundeck/src/Gundeck/Push.hs

@@ -23,15 +23,12 @@ module Gundeck.Push
     deleteToken,
     -- (for testing)
     pushAll,
-    pushAny,
     MonadPushAll (..),
     MonadNativeTargets (..),
     MonadMapAsync (..),
-    MonadPushAny (..),
   )
 where
 
-import Control.Arrow ((&&&))
 import Control.Error
 import Control.Exception (ErrorCall (ErrorCall))
 import Control.Lens (to, view, (.~), (^.))
@@ -43,7 +40,6 @@ import Data.List.Extra qualified as List
 import Data.List1 (List1, list1)
 import Data.Map qualified as Map
 import Data.Range
-import Data.Sequence qualified as Seq
 import Data.Set qualified as Set
 import Data.Text qualified as Text
 import Data.UUID qualified as UUID
@@ -74,16 +70,9 @@ import Wire.API.Push.V2
 
 push :: [Push] -> Gundeck ()
 push ps = do
-  bulk :: Bool <- view (options . settings . bulkPush)
-  rs <-
-    if bulk
-      then (Right <$> pushAll ps) `catch` (pure . Left . Seq.singleton)
-      else pushAny ps
-  case rs of
-    Right () -> pure ()
-    Left exs -> do
-      forM_ exs $ Log.err . msg . (val "Push failed: " +++) . show
-      throwM (mkError status500 "server-error" "Server Error")
+  pushAll ps `catch` \(ex :: SomeException) -> do
+    Log.err $ msg (val "Push failed") . Log.field "error" (displayException ex)
+    throwM (mkError status500 "server-error" "Server Error")
 
 -- | Abstract over all effects in 'pushAll' (for unit testing).
 class (MonadThrow m) => MonadPushAll m where
@@ -134,59 +123,6 @@ instance MonadMapAsync Gundeck where
       Nothing -> mapAsync f l
       Just chunkSize -> concat <$> mapM (mapAsync f) (List.chunksOf chunkSize l)
 
--- | Abstract over all effects in 'pushAny' (for unit testing).
-class (MonadPushAll m, MonadNativeTargets m, MonadMapAsync m) => MonadPushAny m where
-  mpyPush ::
-    Notification ->
-    List1 NotificationTarget ->
-    Maybe UserId ->
-    Maybe ConnId ->
-    Set ConnId ->
-    m [Presence]
-
-instance MonadPushAny Gundeck where
-  mpyPush = Web.push
-
--- | Send individual HTTP requests to cannon for every device and notification.
---
--- REFACTOR: This should go away in the future, once 'pushAll' has been proven to always do the same
--- thing.  also check what types this removal would make unnecessary.
-pushAny ::
-  forall m.
-  (MonadPushAny m) =>
-  [Push] ->
-  m (Either (Seq.Seq SomeException) ())
-pushAny ps = collectErrors <$> mntgtMapAsync pushAny' ps
-  where
-    collectErrors :: [Either SomeException ()] -> Either (Seq.Seq SomeException) ()
-    collectErrors = runAllE . foldMap (AllE . fmapL Seq.singleton)
-
-pushAny' ::
-  forall m.
-  (MonadPushAny m) =>
-  Push ->
-  m ()
-pushAny' p = do
-  i <- mpaMkNotificationId
-  let pload = p ^. pushPayload
-  let notif = Notification i (p ^. pushTransient) pload
-  let rcps = fromRange (p ^. pushRecipients)
-  let uniq = uncurry list1 $ head &&& tail $ toList rcps
-  let tgts = mkTarget <$> uniq
-  unless (p ^. pushTransient) $
-    mpaStreamAdd i tgts pload =<< mpaNotificationTTL
-  mpaForkIO $ do
-    alreadySent <- mpyPush notif tgts (p ^. pushOrigin) (p ^. pushOriginConnection) (p ^. pushConnections)
-    unless (p ^. pushTransient) $
-      mpaPushNative notif (p ^. pushNativePriority) =<< nativeTargets p (nativeTargetsRecipients p) alreadySent
-  where
-    mkTarget :: Recipient -> NotificationTarget
-    mkTarget r =
-      target (r ^. recipientId)
-        & targetClients .~ case r ^. recipientClients of
-          RecipientClientsAll -> []
-          RecipientClientsSome cs -> toList cs
-
 -- | Construct and send a single bulk push request to the client.  Write the 'Notification's from
 -- the request to C*.  Trigger native pushes for all delivery failures notifications.
 pushAll :: (MonadPushAll m, MonadNativeTargets m, MonadMapAsync m) => [Push] -> m ()

services/gundeck/test/integration/API.hs

@@ -50,7 +50,7 @@ import Data.Set qualified as Set
 import Data.Text.Encoding qualified as T
 import Data.UUID qualified as UUID
 import Data.UUID.V4
-import Gundeck.Options hiding (bulkPush)
+import Gundeck.Options
 import Gundeck.Options qualified as O
 import Imports
 import Network.HTTP.Client qualified as Http

services/gundeck/test/unit/MockGundeck.hs

@@ -439,9 +439,6 @@ instance MonadMapAsync MockGundeck where
   mntgtPerPushConcurrency = pure Nothing -- (unbounded)
   mntgtMapAsync f xs = Right <$$> mapM f xs -- (no concurrency)
 
-instance MonadPushAny MockGundeck where
-  mpyPush = mockOldSimpleWebPush
-
 instance MonadBulkPush MockGundeck where
   mbpBulkSend = mockBulkSend
   mbpDeleteAllPresences _ = pure () -- FUTUREWORK: test presence deletion logic
@@ -644,53 +641,6 @@ mockBulkSend uri notifs = do
     BulkPushResponse
       [(ntfId ntif, trgt, getstatus trgt) | (ntif, trgt) <- flat]
 
-mockOldSimpleWebPush ::
-  (HasCallStack, m ~ MockGundeck) =>
-  Notification ->
-  List1 NotificationTarget ->
-  Maybe UserId ->
-  Maybe ConnId ->
-  Set ConnId ->
-  m [Presence]
-mockOldSimpleWebPush notif tgts _senderid mconnid connWhitelist = do
-  env <- ask
-  getstatus <- mkWSStatus
-  let clients :: [(UserId, ClientId)]
-      clients =
-        -- reformat
-        fmap (\(PushTarget uid connid) -> (uid, clientIdFromConnId connid))
-          -- drop all broken web sockets
-          . filter ((== PushStatusOk) . getstatus)
-          -- do not push to sending device
-          . filter ((/= mconnid) . Just . ptConnId)
-          -- reformat
-          . mconcat
-          . fmap
-            ( ( \tgt ->
-                  PushTarget (tgt ^. targetUser)
-                    . fakeConnId
-                    <$> (tgt ^. targetClients)
-              )
-                -- apply filters
-                . connWhitelistSieve
-                . emptyMeansFullHack
-            )
-          $ toList tgts
-      connWhitelistSieve :: NotificationTarget -> NotificationTarget
-      connWhitelistSieve =
-        if null connWhitelist
-          then id
-          else targetClients %~ filter ((`elem` connWhitelist) . fakeConnId)
-      emptyMeansFullHack :: NotificationTarget -> NotificationTarget
-      emptyMeansFullHack tgt =
-        tgt
-          & targetClients %~ \case
-            [] -> clientIdsOfUser env (tgt ^. targetUser)
-            same@(_ : _) -> same
-  forM_ clients $ \(userid, clientid) -> do
-    msWSQueue %= deliver (userid, clientid) (ntfPayload notif)
-  pure $ uncurry fakePresence <$> clients
-
 ----------------------------------------------------------------------
 -- helpers
 

services/gundeck/test/unit/Push.hs

@@ -21,7 +21,7 @@
 module Push where
 
 import Data.Aeson qualified as Aeson
-import Gundeck.Push (pushAll, pushAny)
+import Gundeck.Push (pushAll)
 import Gundeck.Push.Websocket as Web (bulkPush)
 import Imports
 import MockGundeck
@@ -83,11 +83,8 @@ pushAllProp env (Pretty pushes) =
   where
     ((), realst) = runMockGundeck env (pushAll pushes)
     ((), mockst) = runMockGundeck env (mockPushAll pushes)
-    (errs, oldst) = runMockGundeck env (pushAny pushes)
     props =
       [ (Aeson.eitherDecode . Aeson.encode) pushes === Right pushes,
         (Aeson.eitherDecode . Aeson.encode) env === Right env,
-        counterexample "real vs. mock:" $ realst === mockst,
-        counterexample "real vs. old:" $ realst === oldst,
-        counterexample "old errors:" $ isRight errs === True
+        counterexample "real vs. mock:" $ realst === mockst
       ]