Log raw response data. #4
Conversation
| - git: https://github.com/typeable/req.git | ||
| commit: 8829ac5197f7a4b3f04b7fdfc3ea66cfe70ab0a5 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think you should just overwrite httpConfigCheckResponse. The default one indeed fails on 4XX statuses
There was a problem hiding this comment.
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
| ...
There was a problem hiding this comment.
It'd be too much work to drop req in favor to http-client at this point, right?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Currently,
logResponseonly 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
No description provided.