feat: Allow early logger initialization #804
Conversation
elliottt
left a comment
elliottt
left a comment
There was a problem hiding this comment.
I think this all seems reasonable, but I wonder if the additional logging to stderr should be configurable.
runtime/fastly/builtins/logger.cpp
Outdated
| fprintf(stdout, "Log [%s]: %s\n", endpoint_name_str.ptr.get(), msg.ptr.get()); | ||
| fflush(stdout); | ||
|
|
There was a problem hiding this comment.
What do you think about making this behavior configurable? I know that enableDebugLogging() will be removed eventually, but this seems like a pretty ideal use of that.
My worry is that there are reasonable non-debugging uses of the logging api, and unconditionally logging to stderr without a way to disable that introduces additional unnecessary host calls.
There was a problem hiding this comment.
Apparently this behaviour is to match the Rust SDK, and that the behaviour is desireable by default for JS and Go.
Having a way to turn it off might well make sense, and agreed enableDebugLogging() sounds like a good integration point. I do not know about the design of enableDebugLogging() though.
There was a problem hiding this comment.
Actually enableDebugLogging is off by default so doesn't fit the requirements here unfortunately?
elliottt
left a comment
elliottt
left a comment
There was a problem hiding this comment.
As this is matching existing rust SDK behavior, it all seems good to me!
guybedford
force-pushed
the
logger-stdout
branch
from
e3e9b83 to
9ad4792
Compare
|
Looking into this further, it seems Viceroy does handle the logging to stdout as I would have expected, so this PR actually causes a double log when Instead I'm just going to refactor this to not do the stdout logging and just be a fix for #225. |
guybedford
changed the title
|
I've updated this PR title and description to just be a fix for #225. |
This resolves #225 in supporting lazy initialization of the logging endpoint allowing the logger to be created during preinitialization.