Support for custom logger function

[hoho]

It is useful to have the ability to redefine logger function (especially for an embedder).
To achieve that, process._logger method and C++ logging function are added.
This change is designed to avoid affecting the current behaviour (unless you've set the custom logger up).

[hoho]

Guys, please, have a look at this. This pull request doesn't have tests. If the way I do the task is ok, I'll add some tests.

[mscdex]

My one concern would be what happens when multiple modules overwrite ._logger. That could lead to some confusion.

The other thing is, since the constants are not being exported, any custom logger would have to make a copy of those constants in their own project.

[hoho]

I guess the first problem could be solved by making process._logger property non-writable.

Good point about exporting the constants, I'll do it. I've actually thought about the whole thing from an embedder's point of view (and C++ part has enum for them). So, in this case, the fact that it is possible to redefine the whole set of console logging functions with just replacing console._logger, is an unexpected benefit.

[rlidwka]

Why not monkey-patch console.log itself if you need it?

[hoho]

I need it on a low level. I've done it mostly because I need to replace all fprintf(stderr, ...) calls in src/node.cc. Right now I monkey-patch fprintf for this purpose...

[hoho]

@mscdex process._logger is readonly now, and I've set logger function types as readonly properties of process._logger, so there's no constant duplication now.

[mscdex]

@hoho Now I'm more confused. How is someone supposed to be able to override it at all now?

[hoho]

@mscdex it's actually supposed to be used with node::SetLogger() C++ method.
So, when you do node::SetLogger() before node::Start(), you'll have all logging (including raw calls which use fprintf(stderr, ...) in node.cc). To redefine logging for some exact console object instance, one can redefine console._logger() method (which is not readonly).

It's just when I use io.js as a library, I want to customize all the logging (including raw fprintf calls, like, for example RawDebug() from node.cc, which bypass JavaScript).

[brendanashworth]

Hi @hoho!

I was wondering what you'd think about taking a higher level view of the logger. I'm not a big fan of the enums (LOGGER_FUNC_TYPE_INFO, etc), and was thinking just a simple wrapper of fprintf may be better:

// node.h
typedef int (*node_logger)(FILE* stream, const char* fmt, ...);

NODE_EXTERN void SetLogger(node_logger logger);
NODE_EXTERN void Log(FILE* stream, const char* fmt, ...);

// node.cc
// somewhere in main?
node::SetLogger(fprintf);
node::Log(stderr, "foo %d\n", 42);

I'm unsure how to interact with console.log, because many users either monkey-patch console.log or process.stdout.write / process.stderr.write to do this exact sort of thing, and breaking that would probably be semver-major imho (since it is so common). What are your thoughts on that?

[hoho]

@brendanashworth, thanks for your reply.

From my point of view, just fprintf wrapper is not a higher level thing, it is exactly the opposite. On a higher level you log something. On a lower level it might go to a file (or to any other destination).

The fact that people monkey-patch something might be a signal that they are missing some logger extensibility options. I wasn't aiming to fix that though.

This thing is for embedding. I link with io.js statically and it is just a part of my application. That's why I want to have full control over the output (from both internal fprintfs and console.log). I made a function that will allow an embedder to redirect all the output. Everything is defaulted to keep the current behaviour, unless you customize the logger.

[brendanashworth]

@hoho forgive the high level / low level wording in that respect, low level would probably better describe what I intended. I agree monkey patching means that too.

• I think the console.log changes don't belong in a patch designed for embedders. I understand that you wish to avoid writes to stdout/stderr and do this through C++, but console.log isn't the only place that data is written explicitly and just console.log wouldn't fix it all.
• I think this approach doesn't quite match how node.js manages logging. Previously there was no difference between warn, error and info, log, dir. Do you have a need to redirect error and warn to different outputs?

Don't get me wrong, I like what you're doing here and would like to see this merged for embedders - it seems very useful. I just think it overreaches a little bit where it doesn't need to.

As a foot note, this PR will eventually need to be rebased and re-submitted as a PR to the master branch - v1.x has become LTS and is ~400 commits behind development. It is up to you whether you want to continue discussion here or resubmit now, because if you resubmit now you may get more people discussing.

[hoho]

Well, I guess I can monkey-patch console.log by myself separately in my project.
I'll reduce this PR to be able to customize low level fprintfs only.

[jasnell]

@hoho ... is this still something you'd like to pursue? If so, it would need to be updated and rebased on master.. and I think there are still quite a few "Do we really want to do this" kind of questions.

[hoho]

Yeah, I'm still into it. Just had several pretty busy months. I'll try to update the PR on holidays.

About «do we really want to do this» — having fprintf() as the only output ability doesn't feel quite right anyway. I'll skip customizing console.log() functions (I can monkey-patch Console object alright), but for the low level logging it would be nice to have the ability to redirect the output (especially for the embedders).

[brendanashworth]

ping @hoho for the holidays :)

[hoho]

Happy new year, everybody :).
I've just force pushed the current node.js master to my dusty io.js fork. Starting to redo the thing upon the new master. Just for the low level logging as we discussed earlier.

[brendanashworth]

@hoho could you submit a new pull request so that it correctly targets the master branch?

[hoho]

@brendanashworth sure, I'll close this one as soon as the new one is ready.

[hoho]

I've created pre pull request: #4523.
Tested on OSX. Going to test Windows and Linux tomorrow.