Introduce ctx.console to provide logging utilities #233
Conversation
slinkydeveloper
force-pushed
the
issues/187
branch
2 times, most recently
from
5864729 to
cda88c5
Compare
slinkydeveloper
changed the title
…write out in the same format. Also modified the debugInvokeMessage and debugJournalMessage which are now static methods accepting the console as parameter. We also by default set the RestateDebugLogLevel to INVOKE.
slinkydeveloper
force-pushed
the
issues/187
branch
from
cda88c5 to
4025115
Compare
|
LGTM, Also works locally for me. 👍 |
IMO it doesn't hurt to keep it around, perhaps it would be more useful if buffers are printed as hex or base64. |
We should definitely keep it around, but maybe indeed print in a human-parseable format. I can create another issue for this. |
There was a problem hiding this comment.
Generally, I like this a lot, many good changes in here.
Most comments are inline, a few high-level comments below:
AWS Request ID
My main issue here is that the AWS Request ID seems a bit too hard-wired into the core classes. Maybe we can do this a bit more generic, like allowing to pass in an additional log fields (which would be a string populates as [AWS RequestId: ${context.awsRequestId}] by the Lambda handler.
Commit structure:
- For the next PRs, I'd encourage to leave renaming of methods/variable out of the core commits) and have dedicated pre-processing or cleanup commits with renamings.
src/invocation.ts
Outdated
| @@ -21,7 +21,7 @@ import { | |||
| import { | |||
| CompletablePromise, | |||
| makeFqServiceName, | |||
| printMessageAsJson, | |||
| formatMessageAsJson, | |||
There was a problem hiding this comment.
Would be nice to have this in pre- or post-processing commits, not mixed into main logic commits.
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| /* eslint-disable no-console */ | ||
|
|
||
| export class LoggerContext { |
There was a problem hiding this comment.
I was a bit confused by this name. I expected this initially to be a context like in log4j that is dynamically available in the methods that implement the logging logic. But this here is more a config object, used to construct the logger.
Maybe call it LoggerConfig ?
There was a problem hiding this comment.
For me this is really the logging context, because it adds contextual information to log events. LoggerConfig for me is more something like "where to write it" and "the format to use to write it".
The fact that is not dynamically available to logging methods now is an implementation detail of how I just generate the prefix when i create the logger. If we extend the logger with "custom formats", then surely we'll make this context available in the log method implementations.
src/state_machine.ts
Outdated
| @@ -85,8 +97,8 @@ export class StateMachine<I, O> implements RestateStreamConsumer { | |||
| } | |||
|
|
|||
| if (m.messageType === COMPLETION_MESSAGE_TYPE) { | |||
| rlog.debugJournalMessage( | |||
| this.invocation.logPrefix, | |||
| debugJournalMessage( | |||
There was a problem hiding this comment.
I think the original encapsulation was a bit nicer, where the debugJournalMessage was on the logger. Why was it necessary to remove that?
There was a problem hiding this comment.
Because I wanted to remove the old rlog and rely on the new console logger, and I didn't want debugJournalMessage and debugInvocationMessage as those are concern of the internal state machine, and not of the public API. I can reintroduce a new class/interface for debugJournalMessage and debugInvocationMessage and wrap the logger created in the state machine.
There was a problem hiding this comment.
It is not a big deal, was just wondering if there was a deeper reason why that changed.
The rlog was also just internal, and extended Concole to keep those concerns separate.
interface RestateInternalConsole extends Console {
debugInvocationMessage(...): void;
debugJournalMessage(...): void;
}But if that isn't working out, one could probably also do this:
``´typescript
interface InternalLogMethods {
debugInvocationMessage(...): void;
debugJournalMessage(...): void;
}
export type RestateInternalConsole = Console & InternalLogMethods;
There was a problem hiding this comment.
Look at the last commit
Fix #187.
In this PR:
INVOKE, that will print only "Invoking function" and "Function completed"/"Function failed"This is a sample output with
RESTATE_DEBUG_LOGGING=OFF:(those log lines come from the example)
This is sample output with
RESTATE_DEBUG_LOGGING=INVOKE:This is the sample output with
RESTATE_DEBUG_LOGGING=JOURNAL:This is the sample output with
RESTATE_DEBUG_LOGGING=JOURNAL_VERBOSE: