Skip to content

Log raw response data. #4

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.

Sign up for GitHub

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

Merged
my-name-is-lad merged 5 commits into from
Apr 2, 2025
Merged

Log raw response data. #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f90fcff
Log responses
my-name-is-lad Apr 1, 2025
5340ff5
Log response body even if response is ignored.
my-name-is-lad Apr 1, 2025
318ade4
Revert "Log response body even if response is ignored."
my-name-is-lad Apr 1, 2025
b051396
Revert "Log responses"
my-name-is-lad Apr 1, 2025
91f3a86
Log responses via a callback inside req.
my-name-is-lad Apr 1, 2025
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: 0 additions & 1 deletion connexpay/src/Web/Connexpay/Payments.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ sendRequest' resp endpoint body =
r <- if tls
then reqCb POST (url https) jbody resp auth (logRequest obj)
else reqCb POST (url http) jbody resp auth (logRequest obj)
logResponse r
pure r
where
addGuid b =
Expand Down
24 changes: 15 additions & 9 deletions connexpay/src/Web/Connexpay/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import Control.Monad.Except (MonadError, ExceptT, runExceptT, throwError)
import Control.Monad.IO.Class
import Control.Monad.Reader
import Data.Aeson
import Data.ByteString (ByteString)
import Data.Text (Text)
import Data.Text qualified as Text
import Data.UUID (UUID)
import Network.HTTP.Client hiding (responseHeaders)
import Network.HTTP.Client as Client
import Network.HTTP.Req
import Network.HTTP.Types

Expand Down Expand Up @@ -45,8 +46,14 @@ instance MonadHttp ConnexpayM where
, Just f <- guessFailure (statusCode $ responseStatus resp) err.message = throwError (PaymentFailure f)
handleHttpException (VanillaHttpException (HttpExceptionRequest _ c)) = throwError (ConnectionError $ HttpFailure c)

getHttpConfig = do mgr <- asks (.manager)
pure (defaultHttpConfig { httpConfigAltManager = Just mgr })
getHttpConfig =
do mgr <- asks (.manager)
log_ <- asks (.logAction)
let cfg =
defaultHttpConfig
{ httpConfigAltManager = Just mgr
, httpConfigLogResponse = logResponse log_ }
pure cfg


runConnexpay :: Connexpay -> ConnexpayM a -> IO (Either ConnexpayError a)
Expand All @@ -62,11 +69,10 @@ bearerToken :: ConnexpayM (Maybe BearerToken)
bearerToken = do v <- asks (.bearerToken)
liftIO (readMVar v)

logResponse :: HttpResponse r => r -> ConnexpayM ()
logResponse r =
do log_ <- asks (.logAction)
liftIO (log_ msg)
logResponse :: (Text -> IO ()) -> Request -> Response a -> ByteString -> IO ()
logResponse log_ _req resp body = log_ msg
where msg = Text.unlines [ "Connexpay response:"
, "HTTP code: " <> tshow (responseCode r)
, "Headers: " <> tshow (responseHeaders r)
, "HTTP code: " <> tshow (statusCode (responseStatus resp))
, "Headers: " <> tshow (Client.responseHeaders resp)
, "Body: " <> tshow body
]
4 changes: 2 additions & 2 deletions connexpay/src/Web/Connexpay/Utils.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ whenJust Nothing _ = pure ()
tshow :: Show a => a -> Text
tshow = Text.pack . show

responseHeaders :: HttpResponse response => response -> [Header]
responseHeaders = Client.responseHeaders . toVanillaResponse
--responseHeaders :: HttpResponse response => response -> [Header]
--responseHeaders = Client.responseHeaders . toVanillaResponse

responseCode :: HttpResponse response => response -> Int
responseCode = statusCode . Client.responseStatus . toVanillaResponse
Expand Down
3 changes: 2 additions & 1 deletion stack.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ packages:
extra-deps:
- git: https://github.com/typeable/bucks.git
commit: 9e378b675fe7fb88d5ddb3af068d82eb441f343c

- git: https://github.com/typeable/req.git
commit: 8829ac5197f7a4b3f04b7fdfc3ea66cfe70ab0a5
Comment on lines +10 to +11
Copy link
Contributor

@stevladimir stevladimir Apr 2, 2025

Choose a reason for hiding this comment

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

Adding yet another fork into our collection just to add response logging looks fishy. Are we sure there high chances to propagate this change upstream?

Why not to use, e.g. LbsResponse?

do
  resp <- reqCb POST (url https) jbody lbsResponse auth (logRequest obj)
  logResponse resp
  case Aeson.eitherDecode resp of
    ...

Won't that work?

Copy link
Contributor Author

@my-name-is-lad my-name-is-lad Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Only in case of success. Any status code outside of [200;300) range throws an exception and logging that is much trickier, hence this. I tried to work around this without the fork, but it's just not worth it.

Also, I submitted a PR (mrkkrp/req#184) and the upstream seems active enough.

Copy link
Contributor

@stevladimir stevladimir Apr 2, 2025

Choose a reason for hiding this comment

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

I think you should just overwrite httpConfigCheckResponse. The default one indeed fails on 4XX statuses

Copy link
Contributor

@stevladimir stevladimir Apr 2, 2025

Choose a reason for hiding this comment

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

So it is like

-- Modify 'httpConfigCheckResponse' to do nothing
resp <- reqCb POST (url https) jbody lbsResponse auth (logRequest obj)
logResponse resp
if
  | isOk resp.statusCode -> Aeson.decode resp
  | ...

Copy link
Contributor

@Yuras Yuras Apr 2, 2025

Choose a reason for hiding this comment

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

It'd be too much work to drop req in favor to http-client at this point, right?

Copy link
Contributor

@stevladimir stevladimir Apr 2, 2025

Choose a reason for hiding this comment

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

Got your point. Thought there are some other issues with it not mentioned.
I agree that it would be nice to do a replacement. I would also on the way solve the problem with logger context. We can mostly copy-paste what we do in NDC packages.
So do we want to invest time into the replacement? I can take it in case of

Copy link
Contributor Author

@my-name-is-lad my-name-is-lad Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Btw logResponse callback looks too ad hoc. If to go that way then it should be something a la https://hackage.haskell.org/package/http-client-0.7.17/docs/Network-HTTP-Client.html#v:managerModifyResponse.

The problem with this is that the response could be potentially huge. Currently, logResponse only sees a preview (first 1024 bytes by default), which should be enough for our needs here. But if say the response is a multi-gigabyte file, this would be tricky to do correctly.

Copy link
Contributor

@stevladimir stevladimir Apr 2, 2025

Choose a reason for hiding this comment

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

Currently, logResponse only sees a preview (first 1024 bytes by default), which should be enough for our needs here.

Zzz... Are we sure all Connexpay responses fit into 1024 bytes?

That's why managerModifyResponse is built around BodyReader and it is an obligation of library user to care about response size.

Copy link
Contributor Author

@my-name-is-lad my-name-is-lad Apr 2, 2025

Choose a reason for hiding this comment

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

Zzz... Are we sure all Connexpay responses fit into 1024 bytes?

so far they did, but we can increase that easily.

That's why managerModifyResponse is built around BodyReader and it is an obligation of library user to care about response size.

True. We can also use this here, if we want. But I agree, it's a good idea to add a similar feature to req.

Copy link
Contributor

@stevladimir stevladimir Apr 16, 2025

Choose a reason for hiding this comment

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

Currently, logResponse only sees a preview (first 1024 bytes by default), which should be enough for our needs here.

If bad things may happen they will 🤷‍♂️
Got biten by this. In the task where we need to see information not fit into 1024 bytes. Costed me time trying to find out addressVerificationCode in the logs until I remembered about this thread

nix:
packages: [zlib]
local-bin-path: bin