-
Notifications
You must be signed in to change notification settings - Fork 17
feat: add system logger #457
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
Changes from 8 commits
753a7da
32c4629
f985e05
0c0edcc
5d27881
54badea
810112e
463b687
139158e
1ac7353
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
// this file is exported as @netlify/functions/internal, | ||
// and only meant for consumption by Netlify Teams. | ||
// While we try to adhere to semver, this file is not considered part of the public API. | ||
|
||
export { systemLogger, LogLevel } from './lib/system_logger.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
const systemLogTag = '__nfSystemLog' | ||
|
||
const serializeError = (error: Error): Record<string, unknown> => { | ||
const cause = error?.cause instanceof Error ? serializeError(error.cause) : error.cause | ||
|
||
return { | ||
error: error.message, | ||
error_cause: cause, | ||
error_stack: error.stack, | ||
} | ||
} | ||
|
||
// eslint pretends there's a different enum at the same place - it's wrong! | ||
// eslint-disable-next-line no-shadow | ||
export enum LogLevel { | ||
Debug = 1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The edge functions logger has a simpler "Debug/Log/Error" hierarchy here. Since log levels were introduced there only very recently, we can still change what's being used here. I picked those levels because they match the Go levels. @eduardoboucas do you see any value in having an additional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong opinions. I personally don't feel the need for extra levels, and it's not clear to me when we'd use If we do it, do we make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then let's stick with Debug/Log/Error for now - we can always add the additional levels in the future.
That's the current impl, yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, missed that part! |
||
Log, | ||
Error, | ||
} | ||
|
||
class SystemLogger { | ||
// eslint-disable-next-line no-useless-constructor | ||
constructor(private readonly fields: Record<string, unknown> = {}, private readonly logLevel = LogLevel.Log) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused. Don't we need to store these things in the class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. To me we're left with something that is less expressive and an empty constructor that we need an ESLint exception for. 🤷🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there you go: 139158e 😁 |
||
|
||
private doLog(logger: typeof console.log, message: string) { | ||
logger(systemLogTag, JSON.stringify({ msg: message, fields: this.fields })) | ||
} | ||
|
||
log(message: string) { | ||
if (this.logLevel > LogLevel.Log) { | ||
return | ||
} | ||
|
||
this.doLog(console.log, message) | ||
} | ||
|
||
debug(message: string) { | ||
if (this.logLevel > LogLevel.Debug) { | ||
return | ||
} | ||
|
||
this.doLog(console.debug, message) | ||
} | ||
|
||
error(message: string) { | ||
if (this.logLevel > LogLevel.Error) { | ||
return | ||
} | ||
|
||
this.doLog(console.error, message) | ||
} | ||
|
||
withLogLevel(level: LogLevel) { | ||
return new SystemLogger(this.fields, level) | ||
} | ||
|
||
withFields(fields: Record<string, unknown>) { | ||
return new SystemLogger( | ||
{ | ||
...this.fields, | ||
...fields, | ||
}, | ||
this.logLevel, | ||
) | ||
} | ||
|
||
withError(error: unknown) { | ||
const fields = error instanceof Error ? serializeError(error) : { error } | ||
|
||
return this.withFields(fields) | ||
} | ||
} | ||
|
||
export const systemLogger = new SystemLogger() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
const test = require('ava') | ||
|
||
const { systemLogger, LogLevel } = require('../../dist/internal') | ||
|
||
test('Log Level', (t) => { | ||
const originalDebug = console.debug | ||
|
||
const debugLogs = [] | ||
console.debug = (...message) => debugLogs.push(message) | ||
|
||
systemLogger.debug('hello!') | ||
t.is(debugLogs.length, 0) | ||
|
||
systemLogger.withLogLevel(LogLevel.Debug).debug('hello!') | ||
t.is(debugLogs.length, 1) | ||
|
||
systemLogger.withLogLevel(LogLevel.Log).debug('hello!') | ||
t.is(debugLogs.length, 1) | ||
|
||
console.debug = originalDebug | ||
}) | ||
|
||
test('Fields', (t) => { | ||
const originalLog = console.log | ||
const logs = [] | ||
console.log = (...message) => logs.push(message) | ||
systemLogger.withError(new Error('boom')).withFields({ foo: 'bar' }).log('hello!') | ||
t.is(logs.length, 1) | ||
t.is(logs[0][0], '__nfSystemLog') | ||
const log = JSON.parse(logs[0][1]) | ||
t.is(log.msg, 'hello!') | ||
t.is(log.fields.foo, 'bar') | ||
t.is(log.fields.error, 'boom') | ||
t.is(log.fields.error_stack.split('\n').length > 2, true) | ||
|
||
console.log = originalLog | ||
}) |
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.
Nice way to gate the internal stuff I like!