feat(framework): Control logging #7475
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
SokratisVidros
force-pushed
the
control_framework_logging
branch
from
0c0fad9 to
97d3c44
Compare
Add coarse graining logging control to prevent flooding production and test logs
SokratisVidros
force-pushed
the
control_framework_logging
branch
from
910f4c3 to
964dcc5
Compare
SokratisVidros
temporarily deployed
to
Linting
GitHub Actions
Inactive
SokratisVidros
temporarily deployed
to
Linting
GitHub Actions
Inactive
SokratisVidros
temporarily deployed
to
Linting
GitHub Actions
Inactive
| // eslint-disable-next-line no-console | ||
| console.error(bridgeError); | ||
| // eslint-disable-next-line @typescript-eslint/no-base-to-string | ||
| log((l) => l.error(bridgeError.message || bridgeError.toString())); |
There was a problem hiding this comment.
feels a bit complex and not the usual format of logging. what are the cons of doing something like this?
| log((l) => l.error(bridgeError.message || bridgeError.toString())); | |
| log.error(bridgeError.message || bridgeError.toString()); |
in case of
log((l) => `${prefix} ${l.emoji.STEP} Discovered stepId: '${step.stepId}'\tType: '${step.type}'`);
we could do
log.info(`${prefix} ${log.emoji.STEP} Discovered stepId: '${step.stepId}'\tType: '${step.type}'`);
the formatting should be the responsibility of the logger user; this way, it is easier to read. in addition, having the default log level hidden is a bit confusing and not straightforward.
| @@ -72,21 +73,31 @@ export class Client { | |||
|
|
|||
| public strictAuthentication: boolean; | |||
|
|
|||
| public logging: boolean = true; | |||
There was a problem hiding this comment.
| public logging: boolean = true; | |
| public loggingEnabled | isLoggingEnabled: boolean = true; |
| log.enable = () => { | ||
| enabled = true; | ||
| }; | ||
| log.disable = () => { | ||
| enabled = false; |
| } | ||
|
|
||
| // Disable verbose logging in test and production environments | ||
| return !['test', 'production'].includes(process.env.NODE_ENV); |
There was a problem hiding this comment.
Love it!
But can't we reuse NOVU_LOGGING to toggle this? so for test and prod it will be false, but in case we want to debug some edge cases, we could turn it on in prod.
in addition, we could use more specific env variables so they won't collide in the future in any way FRAMEWORK_LOGGING, although it should not be an issue.
What changed? Why was the change needed?
Add coarse-graining logging control and test logs to prevent flooding production.
This is the first step in switching on/off framework logging so we can use it in production and e2e tests. In future work, we need to add logging levels and log differently in development (log everything) and production (log only errors and a summary of the discovered workflows).