Skip to content
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

[WPB-1220] servantify proxy internal #4296

Merged
merged 8 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5-internal/WPB-1220-servantify-proxy-internal
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Servantify internal routing table for proxy.
2 changes: 2 additions & 0 deletions libs/metrics-wai/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
, servant
, servant-multipart
, text
, types-common
, utf8-string
, wai
, wai-middleware-prometheus
Expand All @@ -32,6 +33,7 @@ mkDerivation {
servant
servant-multipart
text
types-common
utf8-string
wai
wai-middleware-prometheus
Expand Down
1 change: 1 addition & 0 deletions libs/metrics-wai/metrics-wai.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ library
, servant
, servant-multipart
, text >=0.11
, types-common
, utf8-string
, wai >=3
, wai-middleware-prometheus
Expand Down
13 changes: 10 additions & 3 deletions libs/metrics-wai/src/Data/Metrics/Middleware/Prometheus.hs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@

module Data.Metrics.Middleware.Prometheus
( waiPrometheusMiddleware,
waiPrometheusMiddlewarePaths,
normalizeWaiRequestRoute,
)
where

import Data.Id
import Data.Metrics.Types (Paths, treeLookup)
import Data.Metrics.WaiRoute (treeToPaths)
import Data.Text.Encoding qualified as T
Expand All @@ -33,12 +35,17 @@ import Network.Wai.Routing.Route (Routes, prepare)
-- This middleware requires your servers 'Routes' because it does some normalization
-- (e.g. removing params from calls)
waiPrometheusMiddleware :: (Monad m) => Routes a m b -> Wai.Middleware
waiPrometheusMiddleware routes =
waiPrometheusMiddleware routes = waiPrometheusMiddlewarePaths $ treeToPaths $ prepare routes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised by our lack of test coverage for proxy 🙄

Wouldn't this ticket have been a chance to make the beast testable and ensure we're not breaking anything? 🤔 (I may miss some details something, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would argue "out of scope", this is only about internal end-points, and i only changed Run.hs because it seemed the most straight-forward way to do this at the time. it's tricky to test these without api keys for the resp. services. and the public routes really haven't changed in this PR, just wrapped a little.

but i agree to your point. hm, what to do?


-- | Helper function that should only be needed as long as we have wai-routing code left in
fisx marked this conversation as resolved.
Show resolved Hide resolved
-- proxy: run 'treeToPaths' on old routing tables and 'routeToPaths' on the servant ones, and
-- feed both to this function.
waiPrometheusMiddlewarePaths :: Paths -> Wai.Middleware
waiPrometheusMiddlewarePaths paths =
Promth.prometheus conf . instrument (normalizeWaiRequestRoute paths)
where
-- See Note [Raw Response]
instrument = Promth.instrumentHandlerValueWithFilter Promth.ignoreRawResponses
paths = treeToPaths $ prepare routes
conf =
Promth.def
{ Promth.prometheusEndPoint = ["i", "metrics"],
Expand All @@ -57,4 +64,4 @@ normalizeWaiRequestRoute paths req = pathInfo
-- Use the normalized path info if available; otherwise dump the raw path info for
-- debugging purposes
pathInfo :: Text
pathInfo = T.decodeUtf8 $ fromMaybe "N/A" mPathInfo
pathInfo = T.decodeUtf8 $ fromMaybe defRequestId mPathInfo
3 changes: 2 additions & 1 deletion libs/metrics-wai/src/Data/Metrics/Servant.hs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
module Data.Metrics.Servant where

import Data.ByteString.UTF8 qualified as UTF8
import Data.Id
import Data.Metrics.Types
import Data.Metrics.Types qualified as Metrics
import Data.Proxy
Expand All @@ -49,7 +50,7 @@ servantPrometheusMiddleware _ = Promth.prometheus conf . instrument promthNormal
promthNormalize req = pathInfo
where
mPathInfo = Metrics.treeLookup (routesToPaths @api) $ encodeUtf8 <$> Wai.pathInfo req
pathInfo = decodeUtf8With lenientDecode $ fromMaybe "N/A" mPathInfo
pathInfo = decodeUtf8With lenientDecode $ fromMaybe defRequestId mPathInfo

-- See Note [Raw Response]
instrument = Promth.instrumentHandlerValueWithFilter Promth.ignoreRawResponses
Expand Down
1 change: 1 addition & 0 deletions libs/metrics-wai/src/Data/Metrics/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ newtype PathTemplate = PathTemplate Text
-- (e.g. user id).
newtype Paths = Paths (Forest PathSegment)
deriving (Eq, Show)
deriving newtype (Semigroup)

type PathSegment = Either ByteString ByteString

Expand Down
4 changes: 4 additions & 0 deletions libs/types-common/src/Data/Id.hs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ module Data.Id
-- * Other IDs
ConnId (..),
RequestId (..),
defRequestId,
BotId (..),
NoId,
OAuthClientId,
Expand Down Expand Up @@ -418,6 +419,9 @@ newtype RequestId = RequestId
ToBytes
)

defRequestId :: (IsString s) => s
defRequestId = "N/A"

instance ToSchema RequestId where
schema =
RequestId . encodeUtf8
Expand Down
3 changes: 1 addition & 2 deletions libs/wai-utilities/src/Network/Wai/Utilities/Request.hs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
{-# LANGUAGE OverloadedStrings #-}
{-# OPTIONS_GHC -Wno-orphans #-}

-- This file is part of the Wire Server implementation.
Expand Down Expand Up @@ -56,7 +55,7 @@ lookupRequestId reqIdHeaderName =

getRequestId :: HeaderName -> Request -> RequestId
getRequestId reqIdHeaderName req =
RequestId $ fromMaybe "N/A" $ lookupRequestId reqIdHeaderName req
RequestId $ fromMaybe defRequestId $ lookupRequestId reqIdHeaderName req

----------------------------------------------------------------------------
-- Typed JSON 'Request'
Expand Down
11 changes: 6 additions & 5 deletions libs/wai-utilities/src/Network/Wai/Utilities/Server.hs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import Data.ByteString.Builder
import Data.ByteString.Char8 qualified as C
import Data.ByteString.Lazy qualified as LBS
import Data.Domain (domainText)
import Data.Id
import Data.Metrics.GC (spawnGCMetricsCollector)
import Data.Streaming.Zlib (ZlibException (..))
import Data.Text.Encoding qualified as Text
Expand Down Expand Up @@ -168,7 +169,7 @@ compile routes = Route.prepare (Route.renderer predicateError >> routes)
r = reasonStr <$> reason e
t = message e
in case catMaybes [l, s, r] of
[] -> maybe "N/A" (LT.decodeUtf8With lenientDecode . LBS.fromStrict) t
[] -> maybe defRequestId (LT.decodeUtf8With lenientDecode . LBS.fromStrict) t
bs -> LT.decodeUtf8With lenientDecode . toLazyByteString $ mconcat bs <> messageStr t
labelStr [] = Nothing
labelStr ls =
Expand Down Expand Up @@ -311,7 +312,7 @@ heavyDebugLogging sanitizeReq lvl lgr reqIdHeaderName app = \req cont -> do
logMostlyEverything req bdy resp = Log.debug lgr logMsg
where
logMsg =
field "request" (fromMaybe "N/A" $ lookupRequestId reqIdHeaderName req)
field "request" (fromMaybe defRequestId $ lookupRequestId reqIdHeaderName req)
. field "request_details" (show req)
. field "request_body" bdy
. field "response_status" (show $ responseStatus resp)
Expand Down Expand Up @@ -350,7 +351,7 @@ rethrow5xx getRequestId logger app req k = app req k'
let logMsg =
field "canoncalpath" (show $ pathInfo req)
. field "rawpath" (rawPathInfo req)
. field "request" (fromMaybe "N/A" $ getRequestId req)
. field "request" (fromMaybe defRequestId $ getRequestId req)
. msg (val "ResponseRaw - cannot collect metrics or log info on errors")
Log.log logger Log.Debug logMsg
k resp
Expand Down Expand Up @@ -436,7 +437,7 @@ logError' g mr e = liftIO $ doLog g (logErrorMsgWithRequest mr e)

logJSONResponse :: (MonadIO m) => Logger -> Maybe ByteString -> JSONResponse -> m ()
logJSONResponse g mReqId e = do
let r = fromMaybe "N/A" mReqId
let r = fromMaybe defRequestId mReqId
liftIO $
doLog g $
field "request" r
Expand All @@ -462,7 +463,7 @@ logErrorMsg (Wai.Error c l m md inner) =

logErrorMsgWithRequest :: Maybe ByteString -> Wai.Error -> Msg -> Msg
logErrorMsgWithRequest mr e =
field "request" (fromMaybe "N/A" mr) . logErrorMsg e
field "request" (fromMaybe defRequestId mr) . logErrorMsg e

runHandlers :: SomeException -> [Handler IO a] -> IO a
runHandlers e [] = throwIO e
Expand Down
4 changes: 2 additions & 2 deletions libs/wire-subsystems/src/Wire/EmailSubsystem/Interpreter.hs
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ renderNewClientEmail email name locale Client {..} NewClientEmailTemplate {..} b
html = renderHtmlWithBranding newClientEmailBodyHtml replace branding
subj = renderTextWithBranding newClientEmailSubject replace branding
replace "name" = fromName name
replace "label" = fromMaybe "N/A" clientLabel
replace "model" = fromMaybe "N/A" clientModel
replace "label" = fromMaybe defRequestId clientLabel
replace "model" = fromMaybe defRequestId clientModel
replace "date" =
formatDateTime
"%A %e %B %Y, %H:%M - %Z"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
module Wire.NotificationSubsystem.InterpreterSpec (spec) where

import Bilge (RequestId (..))
import Control.Concurrent.Async (async, wait)
import Control.Exception (throwIO)
import Data.Data (Proxy (Proxy))
import Data.Id
import Data.List.NonEmpty (NonEmpty ((:|)), fromList)
import Data.List1 qualified as List1
import Data.Range (fromRange, toRange)
Expand Down Expand Up @@ -37,7 +37,7 @@ spec = describe "NotificationSubsystem.Interpreter" do
{ fanoutLimit = toRange $ Proxy @30,
chunkSize = 12,
slowPushDelay = 0,
requestId = RequestId "N/A"
requestId = RequestId defRequestId
}

connId2 <- generate arbitrary
Expand Down Expand Up @@ -98,7 +98,7 @@ spec = describe "NotificationSubsystem.Interpreter" do
{ fanoutLimit = toRange $ Proxy @30,
chunkSize = 12,
slowPushDelay = 0,
requestId = RequestId "N/A"
requestId = RequestId defRequestId
}

connId2 <- generate arbitrary
Expand Down Expand Up @@ -153,7 +153,7 @@ spec = describe "NotificationSubsystem.Interpreter" do
{ fanoutLimit = toRange $ Proxy @30,
chunkSize = 12,
slowPushDelay = 1,
requestId = RequestId "N/A"
requestId = RequestId defRequestId
}

connId2 <- generate arbitrary
Expand Down Expand Up @@ -211,7 +211,7 @@ spec = describe "NotificationSubsystem.Interpreter" do
{ fanoutLimit = toRange $ Proxy @30,
chunkSize = 12,
slowPushDelay = 1,
requestId = RequestId "N/A"
requestId = RequestId defRequestId
}

user1 <- generate arbitrary
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ pushNotification runningFlag targetDomain (msg, envelope) = do
ceHttp2Manager <- asks http2Manager
let ceOriginDomain = notif.ownDomain
ceTargetDomain = targetDomain
ceOriginRequestId = fromMaybe (RequestId "N/A") notif.requestId
ceOriginRequestId = fromMaybe (RequestId defRequestId) notif.requestId
cveEnv = FederatorClientEnv {..}
cveVersion = Just V0 -- V0 is assumed for non-versioned queue messages
fcEnv = FederatorClientVersionedEnv {..}
Expand All @@ -135,7 +135,7 @@ pushNotification runningFlag targetDomain (msg, envelope) = do
ceFederator = federator,
ceHttp2Manager = manager,
ceOriginRequestId =
fromMaybe (RequestId "N/A") . (.requestId) . NE.head $ bundle.notifications
fromMaybe (RequestId defRequestId) . (.requestId) . NE.head $ bundle.notifications
}
remoteVersions :: Set Int <-
liftIO
Expand Down Expand Up @@ -166,7 +166,7 @@ pushNotification runningFlag targetDomain (msg, envelope) = do
ceHttp2Manager <- asks http2Manager
let ceOriginDomain = notif.ownDomain
ceTargetDomain = targetDomain
ceOriginRequestId = fromMaybe (RequestId "N/A") notif.requestId
ceOriginRequestId = fromMaybe (RequestId defRequestId) notif.requestId
cveEnv = FederatorClientEnv {..}
fcEnv = FederatorClientVersionedEnv {..}
sendNotificationIgnoringVersionMismatch fcEnv notif.targetComponent notif.path notif.body
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ spec = do
path = "/on-user-deleted-connections",
body = RawJson $ Aeson.encode notifContent,
bodyVersions = Nothing,
requestId = Just $ RequestId "N/A"
requestId = Just $ RequestId defRequestId
}
envelope <- newMockEnvelope
let msg =
Expand Down Expand Up @@ -104,7 +104,7 @@ spec = do
notifContent <-
generate $
ClientRemovedRequest <$> arbitrary <*> arbitrary <*> arbitrary
let bundle = toBundle @'OnClientRemovedTag (RequestId "N/A") origDomain notifContent
let bundle = toBundle @'OnClientRemovedTag (RequestId defRequestId) origDomain notifContent
envelope <- newMockEnvelope
let msg =
Q.newMsg
Expand Down Expand Up @@ -148,8 +148,8 @@ spec = do
}
let update0 = conversationUpdateToV0 update
let bundle =
toBundle (RequestId "N/A") origDomain update
<> toBundle (RequestId "N/A") origDomain update0
toBundle (RequestId defRequestId) origDomain update
<> toBundle (RequestId defRequestId) origDomain update0
envelope <- newMockEnvelope
let msg =
Q.newMsg
Expand Down Expand Up @@ -215,7 +215,7 @@ spec = do
path = "/on-user-deleted-connections",
body = RawJson $ Aeson.encode notifContent,
bodyVersions = Nothing,
requestId = Just $ RequestId "N/A"
requestId = Just $ RequestId defRequestId
}
envelope <- newMockEnvelope
let msg =
Expand Down
4 changes: 2 additions & 2 deletions services/brig/src/Brig/App.hs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ module Brig.App
)
where

import Bilge (RequestId (..))
import Bilge qualified as RPC
import Bilge.IO
import Bilge.RPC (HasRequestId (..))
Expand Down Expand Up @@ -122,6 +121,7 @@ import Control.Monad.Trans.Resource
import Data.ByteString.Conversion
import Data.Credentials (Credentials (..))
import Data.Domain
import Data.Id
import Data.Misc
import Data.Qualified
import Data.Text qualified as Text
Expand Down Expand Up @@ -278,7 +278,7 @@ newEnv opts = do
awsEnv = aws, -- used by `journalEvent` directly
appLogger = lgr,
internalEvents = (eventsQueue :: QueueEnv),
requestId = RequestId "N/A",
requestId = RequestId defRequestId,
userTemplates = utp,
providerTemplates = ptp,
teamTemplates = ttp,
Expand Down
4 changes: 2 additions & 2 deletions services/cannon/src/Cannon/App.hs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import Control.Monad.Catch
import Data.Aeson hiding (Error, Key, (.=))
import Data.ByteString.Conversion
import Data.ByteString.Lazy (toStrict)
import Data.Id (ClientId)
import Data.Id
import Data.Text.Lazy qualified as Text
import Data.Timeout
import Imports hiding (threadDelay)
Expand Down Expand Up @@ -155,7 +155,7 @@ rejectOnError :: PendingConnection -> HandshakeException -> IO a
rejectOnError p x = do
let f lb mg = toStrict . encode $ mkError status400 lb mg
case x of
NotSupported -> rejectRequest p (f "protocol not supported" "N/A")
NotSupported -> rejectRequest p (f "protocol not supported" defRequestId)
MalformedRequest _ m -> rejectRequest p (f "malformed-request" (Text.pack m))
OtherHandshakeException m -> rejectRequest p (f "other-error" (Text.pack m))
_ -> pure ()
Expand Down
5 changes: 3 additions & 2 deletions services/cannon/src/Cannon/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module Cannon.Types
)
where

import Bilge (Manager, RequestId (..))
import Bilge (Manager)
import Bilge.RPC (HasRequestId (..))
import Cannon.Dict (Dict)
import Cannon.Options
Expand All @@ -42,6 +42,7 @@ import Cannon.WS qualified as WS
import Control.Concurrent.Async (mapConcurrently)
import Control.Lens ((^.))
import Control.Monad.Catch
import Data.Id
import Data.Text.Encoding
import Imports
import Prometheus
Expand Down Expand Up @@ -100,7 +101,7 @@ mkEnv ::
Clock ->
Env
mkEnv external o l d p g t =
Env o l d (RequestId "N/A") $
Env o l d (RequestId defRequestId) $
WS.env external (o ^. cannon . port) (encodeUtf8 $ o ^. gundeck . host) (o ^. gundeck . port) l p d g t (o ^. drainOpts)

runCannon :: Env -> Cannon a -> IO a
Expand Down
4 changes: 2 additions & 2 deletions services/cannon/src/Cannon/WS.hs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ import Data.ByteString.Char8 (pack)
import Data.ByteString.Conversion
import Data.ByteString.Lazy qualified as L
import Data.Hashable
import Data.Id (ClientId, ConnId (..), UserId)
import Data.Id (ClientId, ConnId (..), UserId, defRequestId)
import Data.List.Extra (chunksOf)
import Data.Text.Encoding (decodeUtf8)
import Data.Timeout (TimeoutUnit (..), (#))
Expand Down Expand Up @@ -192,7 +192,7 @@ env ::
Clock ->
DrainOpts ->
Env
env leh lp gh gp = Env leh lp (host gh . port gp $ empty) (RequestId "N/A")
env leh lp gh gp = Env leh lp (host gh . port gp $ empty) (RequestId defRequestId)

runWS :: (MonadIO m) => Env -> WS a -> m a
runWS e m = liftIO $ runReaderT (_conn m) e
Expand Down
Loading
Loading