feat: add system logger #457
Conversation
src/lib/system_logger.ts
Outdated
| ) {} | ||
|
|
||
| private doLog(logger: typeof console.log, message: string) { | ||
| if (this.samplingRate < 1 && Math.random() > this.samplingRate) { |
There was a problem hiding this comment.
I've added support for sampling here, because I feel like it's a useful thing for collecting metrics without increasing the log volume too much. The more I think about it, though, the more I feel like the sampling rate should be set on a per-log basis, instead of on a per-logger basis. @eduardoboucas curious what you think - is it useful to have sampling built-in to the logger? If yes, should we also add it to the edge functions logger?
There was a problem hiding this comment.
I think this adds unnecessary complexity at this point.
| // 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.
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?
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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.
src/lib/system_logger.ts
Outdated
| return this.withFields(fields) | ||
| } | ||
|
|
||
| withRequest(req: Request) { |
There was a problem hiding this comment.
Edge-Utils only has a withRequestID function, not a withRequest one. I've opened a PR over there to also add withRequest, so both our loggers adhere to the same shape: https://github.com/netlify/edge-utils/pull/36
|
Removed sampling, withRequest, and reduced the number of log levels. Let's start small! |
package.json
Outdated
| @@ -9,6 +9,10 @@ | |||
| "dist/**/*.d.ts", | |||
| "types/**/*.d.ts" | |||
| ], | |||
| "exports": { | |||
| ".": "./dist/main.js", | |||
There was a problem hiding this comment.
Nice way to gate the internal stuff I like!
src/lib/system_logger.ts
Outdated
|
|
||
| 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.
I'm confused. Don't we need to store these things in the class?
There was a problem hiding this comment.
adding private before a constructor parameter makes it a class field
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
there you go: 139158e 😁
Tried locally with The system logging itself works perfectly. |
🤖 I have created a release *beep* *boop* --- ## [2.5.0](v2.4.1...v2.5.0) (2024-01-18) ### Features * add system logger ([#457](#457)) ([f09ddc4](f09ddc4)) ### Bug Fixes * **deps:** update dependency @netlify/serverless-functions-api to v1.13.0 ([#451](#451)) ([d15b673](d15b673)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: token-generator-app[bot] <82042599+token-generator-app[bot]@users.noreply.github.com>
Follow-up to #457. The system logger shouldn't be visible in local dev.
Adds a system logger for internal usage, closely modeled after the edge-functions one.