-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sm/new logger #876
Sm/new logger #876
Conversation
import { compare } from 'semver'; | ||
// needed for TS to not put everything inside /lib/src | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
import * as pjson from '../package.json'; | ||
import { Logger } from './logger/logger'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Logger instead of debug for logging. this allows debug to be removed
@@ -0,0 +1,90 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored out the "filters" from the original logger into its own function,
and then refactored it to speed things up a bit.
When the perf tests run, they log a LOT through to the fs. That becomes a stream bottleneck, and backpressure eventually makes its way from fs => transport stream => worker thread => logger => main process. improving this redactor perf should keep the logger fast.
@@ -0,0 +1,56 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loads on a worker thread, and pino streams messages through it before pipelining them to the final destination (file, or pretty terminal output)
The exported types aren't good, and the pipeline doesn't know what precedes it (node.PipelineSource uses any
) I tried to clean this up but couldn't get them all.
src/logger/logger.ts
Outdated
* @deprecated. Has no effect | ||
*/ | ||
|
||
public readonly logRotationCount = new Env().getNumber('SF_LOG_ROTATION_COUNT') ?? 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could support this by comparing it against SF_LOG_ROTATION_PERIOD
in the cleanup
method. Right now it does not look like users are able to configure keeping logs older than 7 days (ignoring the cleanup odds)
chore: new prerelease logic
The merge-base changed after approval.
The merge-base changed after approval.
What does this PR do?
logger via pino instead of bunyan fork.
some useful background on pino: https://www.nearform.com/blog/pino7-0-0-pino-transport-worker_thread-transport/
docs: https://getpino.io/#/
many of the existing methods on Logger class become no-ops
getBunyanLogger will return a pino Logger. It'll still work for some things (ex: trace(), debug()) but probably breaks if you're trying to do "fancy" things to the logger (add/remove streams/serializers/etc)
the log filtering is now its own reusable function so it can be re-used . It's also quite a bit faster.
What issues does this PR fix or reference?
@W-13586057@
forcedotcom/cli#2209
forcedotcom/cli#2196 (haven't tested this one, but it's worth checking. We don't swallow uncaught exceptions in the new logger)
forcedotcom/cli#1706
forcedotcom/cli#1699
and also 3 closed issues
forcedotcom/cli#1928
forcedotcom/cli#2198
forcedotcom/cli#1408