Skip to content
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

feat: add system logger #457

Merged
merged 10 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
"dist/**/*.d.ts",
"types/**/*.d.ts"
],
"exports": {
".": "./dist/main.js",
Copy link
Contributor

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!

"./internal": "./dist/internal.js"
},
"scripts": {
"build": "tsc",
"prepare": "husky install node_modules/@netlify/eslint-config-node/.husky/",
Expand Down
5 changes: 5 additions & 0 deletions src/internal.ts
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'
74 changes: 74 additions & 0 deletions src/lib/system_logger.ts
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Warn level and using the Info name? Should we change this to be Debug/Log/Error as well?

Copy link
Member

Choose a reason for hiding this comment

The 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 warn vs. info.

If we do it, do we make log() an alias of info()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

If we do it, do we make log() an alias of info()?

That's the current impl, yes.

Copy link
Member

Choose a reason for hiding this comment

The 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) {}
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding private before a constructor parameter makes it a class field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The 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. 🤷🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
37 changes: 37 additions & 0 deletions test/unit/system_logger.js
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
})
2 changes: 1 addition & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
// "disableReferencedProjectLoad": true, /* Reduce the number of projects loaded automatically by TypeScript. */

/* Language and Environment */
"target": "ES2020" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
"target": "ES2022" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
// "lib": [], /* Specify a set of bundled library declaration files that describe the target runtime environment. */
// "jsx": "preserve", /* Specify what JSX code is generated. */
// "experimentalDecorators": true, /* Enable experimental support for TC39 stage 2 draft decorators. */
Expand Down