fix(console): Re-patch console in AWS Lambda runtimes#20337
fix(console): Re-patch console in AWS Lambda runtimes#20337
Conversation
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 249d85d. Configure here.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Mostly just some questions and edge cases to consider, but this is a very straightforward way around AWS's instrumentation thwarting ours.
Out of scope for this PR, but it does make me wonder if there's a way to abstract this into core/src/utils/object.ts wrapMethod or something. If we find ourselves in this position again, we can consider doing that. Probably premature otherwise, since just wrapping as a plain old method assignment is usually fine.
Tests look good (rubber-stamp LGTM, I did not run the tests.)
| if (isExecuting) { | ||
| // Re-entrant call: a third party captured `wrapper` via the getter and calls it | ||
| // from inside their replacement (e.g. `const prev = console.log; console.log = (...a) => { prev(...a); }`). | ||
| // Calling `consoleDelegate` here would recurse, so fall back to the native method. | ||
| nativeMethod.apply(GLOBAL_OBJ.console, args); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think this is fine; presumably if someone overrides the wrapper, and then calls it, they're trying to call the native function, not the wrapper. However, as written, if they do this, then it'll avoid the triggerHandlers call below.
Consider:
const actualMethod = isExecuting ? consoleDelegate : nativeMethod;
isExecuting = true;
try {
triggerHandlers('console', { args, level } as HandlerDataConsole);
// support `console.log.apply(someOtherConsole, args)`
return actualMethod.apply(this, args);
} finally {
isExecuting = false;
}There was a problem hiding this comment.
The current behavior is correct as skipping triggerHandlers on re-entrant calls avoids duplicate breadcrumbs. The user called console.log("hello") once...the re-entrant call is just the Lambda runtime forwarding that same message. We already captured it on the first pass.
And the ternary in the suggestion is flipped, was that on purpose? The first call would use nativeMethod (skipping the Lambda delegate entirely), and the re-entrant call would use consoleDelegate (infinite recursion). Did you maybe mean isExecuting ? nativeMethod : consoleDelegate?
There was a problem hiding this comment.
Oh yes, oops. Flipped the ternary.
If you have a test verifying the correct behavior, then that's fine. Might be worth a comment so no one comes alone making the same mistake I did and "fixes" it 😅
| triggerHandlers('console', { args, level } as HandlerDataConsole); | ||
|
|
||
| const log = originalConsoleMethods[level]; | ||
| log?.apply(GLOBAL_OBJ.console, args); |
There was a problem hiding this comment.
| log?.apply(GLOBAL_OBJ.console, args); | |
| return log?.apply(this, args); |
| typeof newValue === 'function' && | ||
| newValue !== wrapper && | ||
| newValue !== originalConsoleMethods[level] && | ||
| !(newValue as WrappedFunction).__sentry_original__ |
There was a problem hiding this comment.
Clobbering the wrapper if it's a sentry wrapped function makes sense (since that's likely ourselves doing it), but I'm unclear why you're allowing it to be set back to the originalConsoleMethods[level].
That would mean:
const original = console.log
// Sentry setup happens
console.log = someAwsThing;
// later...
console.log = original; // lose the Sentry instrumentation!It seems like setting it to the original should just set the consoleDelegate, no?
There was a problem hiding this comment.
True, that's probably a bug. I'm gonna look into that. Currently, it makes sure that the consoleSandbox still works.
3 participants
On AWS Lambda, the Node.js runtime replaces
console.*methods with its own loggers. This means Sentry's console instrumentation gets silently overwritten, and integrations likeconsoleLoggingIntegrationstop capturing console output entirely.This PR fixes that by introducing a
defineProperty-based patching strategy for Lambda environments. Instead of simply assigning a wrapper toconsole.log(which Lambda can overwrite), we define a getter/setter on the console property. When the Lambda runtime assigns its logger, the setter intercepts it, stores the new function as the underlying delegate, and keeps Sentry's wrapper in place. The handler continues to fire, and the Lambda logger still gets called underneath (I checked that manually - the log is still shown in the CloudWatch logs).This behavior is guarded behind
process.env.LAMBDA_TASK_ROOT, so non-Lambda environments continue to use the existingfill()-based patching with zero behavioral change. IfdefinePropertyfails for any reason, it falls back tofill().The setter also handles
consoleSandboxcorrectly (recognizes when it restores the original method and allows it through), and defers to other Sentry wrappers by checking for__sentry_original__.Closes #18238