Add cross-fork logger for runtime-handler #459
Conversation
🦋 Changeset detectedLatest commit: c82f7e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dkundel
force-pushed
the
dkundel/fix-458
branch
from
926cb39
to
b519b6f
Compare
dkundel
force-pushed
the
dkundel/fix-458
branch
from
b519b6f
to
082d05f
Compare
| typeof config.logger[crossForkLogMessage.level] === 'function' | ||
| ) { | ||
| config.logger[crossForkLogMessage.level]( | ||
| // @ts-ignore |
There was a problem hiding this comment.
I'm not too happy about this but typing here would have been quite the pain and in my opinion would distract from the fact that this is just a pass through.
| }: { | ||
| err?: Error | number | string; | ||
| reply?: Reply; | ||
| debugMessage?: string; | ||
| debugArgs?: any[]; | ||
| crossForkLogMessage?: { | ||
| level: keyof LoggerInstance; | ||
| args: [string] | [string, number] | [string, string]; |
There was a problem hiding this comment.
I can't imagine these ever changing, but does it make sense to colocation the potential args shapes with the LoggerInstance type? In case the logger instance type is used similarly elsewhere.
There was a problem hiding this comment.
args in the sendLog has the type (string | number)[] . This was cleaner to read.
| process.send({ | ||
| crossForkLogMessage: { | ||
| level, | ||
| args: args, |
There was a problem hiding this comment.
| args: args, | |
| args, |
|
|
||
| if (crossForkLogMessage) { | ||
| if ( | ||
| config.logger && | ||
| typeof config.logger[crossForkLogMessage.level] === 'function' | ||
| ) { |
There was a problem hiding this comment.
nit: crossForkLogMessage && config.logger && typeof config.logger[crossForkLogMessage.level] === 'function'
will typeof config.logger[crossForkLogMessage.level] ever return something other than a function?
| log(msg: string, level: number) { | ||
| this.sendLog('log', msg, level); | ||
| } |
There was a problem hiding this comment.
curious, when do we use a log level? Also, what does level represent here?
For every invocation of a local Function we spin up a new forked process. The
loggerinstance was passed across the fork but wasn't appropriately serialized/deserialized. As a result you got cryptic error messages like the one in #458 when the process was trying to write to the logger. I changed that by implementing a cross fork logger that has the same interface as the regular instance but then passes the data back viaprocess.sendthe same way that the fork was already communicating with the main thread basically as an RPC to trigger the original logger instance.Contributing to Twilio