Conversation
| // Lets us dump console.log()s to stdout when running test-runner with --verbose flag, to make | ||
| // it easier to debug tests. Note that when --verbose is not passed, KJ_LOG(INFO, ...) will | ||
| // not even evaluate its arguments, so `message()` will not be called at all. | ||
| KJ_LOG(INFO, "console.log()", message()); |
There was a problem hiding this comment.
I was going to remove this line completely, but there are some tests in server-test.c++ that expect logs to be written, and intercepting fprintf didn't seem like a good idea. Maybe it would be better to pass an output stream for writing to, instead of using fprintf directly? This was definitely simpler though... 😃
There was a problem hiding this comment.
Yeah, for this I'm not sure. These end up being non-ops in production. @kentonv any thoughts on this one?
|
|
||
| enum ConsoleMode { | ||
| // Only send `console.log`s to the inspector | ||
| INSPECTOR_ONLY, |
There was a problem hiding this comment.
INSPECTOR_ONLY is the previous behaviour, and the default. STDOUT must be explicitly enabled. The production runtime will continue to have the previous behaviour,
There was a problem hiding this comment.
I would add this additional detail into the comment, tbh.
| auto setHandler = [&](const char* method, LogLevel level) { | ||
| auto methodStr = jsg::v8StrIntern(lock->v8Isolate, method); | ||
| v8::Global<v8::Function> original( | ||
| lock->v8Isolate, jsg::check(console->Get(context, methodStr)).As<v8::Function>()); | ||
|
|
||
| auto f = lock->wrapSimpleFunction(context, | ||
| [mode, level, original = kj::mv(original)](jsg::Lock& js, | ||
| const v8::FunctionCallbackInfo<v8::Value>& info) { | ||
| handleLog(js, mode, level, original, info); | ||
| }); | ||
| jsg::check(console->Set(context, methodStr, f)); | ||
| }; | ||
|
|
||
| setHandler("debug", LogLevel::DEBUG_); | ||
| setHandler("error", LogLevel::ERROR); | ||
| setHandler("info", LogLevel::INFO); | ||
| setHandler("log", LogLevel::LOG); | ||
| setHandler("warn", LogLevel::WARN); |
There was a problem hiding this comment.
While I was looking into calling the original console methods, I came across https://source.chromium.org/chromium/chromium/src/+/main:v8/src/d8/d8-console.h. Is there a reason we don't use ConsoleDelegate for this custom console stuff? Is it because we wouldn't be able to link up logs with the correct worker or something like that?
There was a problem hiding this comment.
Good question. This particular bit predates me so I'm not sure and I haven't looked into it yet. @kentonv might have some insight. It might be that ConsoleDelegate would just work but no one has tried? Really not sure.
|
Amazing. Thank you @mrbbot! |
|
Extremely happy to see this! Initial review pass looks very good but I want to take a more detailed dig through a bit later. |
|
One clarifying question: You say, "Logs to the console even if an inspector is connected" ... it still logs to the inspector in that case tho, correct? |
Yep, it logs to both the inspector and the console. 👍 |
|
@mrbbot ... I'm marking this "needs-internal-pr" to ensure that we get a good internal CI run with this change before it lands. I just want to verify in CI that the default behavior on internal builds remains unchanged. Otherwise, other than the few comments I've already left, this LGTM. Once we have the internal CI run and you're done with whatever additional edits you want to make here, I'll approve the PR. |
50bbc20 to
0cc40a7
Compare
0cc40a7 to
4daaf83
Compare
|
@jasnell Internal PR |
Hey! 👋 This PR...
console.log()to log directly to stdout/err, regardless of whether the--verboseflag is setutil.inspect()function to stringify values as opposed toJSON.stringify()The end result is easier-to-read, detailed, coloured logs when running
workerdlocally. As an example, with the following script:Before:
After:
Follow-up tasks:
util.inspect()from Node.js: the current polyfill has a few limitations. We should be able to implement the required native code to get around these.util.formatWithOptions()for formatting: this will add supportprintf-style%strings, as required forconsole.log()util.inspect(): if ourutil.inspect()function matches Node's, we should be able to export it from thenode:utilmodule ifnodejs_compatis enabled.request.headersabove should probably log something similar to theMap.