fix(core): make sure requests made outside z.request are logged
#387
Conversation
There was a problem hiding this comment.
Thanks for fixing! I didn't test because I assume you did. This could also fix PDE-2465, where calling z.request from legacy scripting isn't logged. If that's true, I'll backport this fix to 9.x, on which most of the converted apps depend.
| @@ -205,7 +205,8 @@ const createLambdaHandler = (appRawOrPath) => { | |||
| // Adds logging for _all_ kinds of http(s) requests, no matter the library | |||
| if (!skipHttpPatch) { | |||
| const httpPatch = createHttpPatch(event); | |||
| httpPatch(require('http')); // 'https' uses 'http' under the hood | |||
| httpPatch(require('http')); | |||
| httpPatch(require('https')); // 'https' needs to be patched separately | |||
There was a problem hiding this comment.
'https' uses 'http' under the hood
I wonder how you found out ☝️ isn't true.
There was a problem hiding this comment.
A lot of digging and console.log 😅
I saw that the patching was happening, but our proxied request object wasn't being called. Then I saw that https wasn't being patched when we patched http. So, apparently something changed in the https implementation. I scanned through some of it and it doesn't directly require('http') - it seems like they both require common modules now.
I wasn't quite curious enough to dig back through its history to find out when that happened. But that's the exact sort of thing that is fair game for a major node version upgrade.
From a brief test, that looks to be the case! I'll amend that jira ticket to cover backporting the fix instead of doing the fix itself. |
fixes PDE-2542.
The actual fix itself is a one-liner (marked). The rest of this PR is actually running the tests; this went unnoticed for a long time because we had been skipping these tests.
The opt-out test is a special case because if any other test creates a lambda handler (and many do), then that one test fails. We have to give it its own test run for it to hope to pass.
But, we also need to verify that the test actually ran. When I was first writing the mocha command, it was saying
0 tests ran👍, which we don't want. So, we have to plug into mocha and make an assertion about the number of tests that ran (1) and the number that passed (1). So, we wrap the whole thing in a bashtestcall and usejqand mocha's JSON test reporter to get the number of passes (which should be exactly 1). Is this overkill? Maybe, but this is the exact sort of regression that unit tests should catch, and ours didn't.