util/logging.ts: Add support for logging #308
Conversation
src/client.ts
Outdated
| #getLogger(partialClientConfig?: ClientConfiguration): LogHandler { | ||
| if ( | ||
| partialClientConfig && | ||
| "logger" in partialClientConfig && | ||
| partialClientConfig.logger === undefined | ||
| ) { | ||
| throw new TypeError(`ClientConfiguration option logger must be defined.`); | ||
| } | ||
| if ( | ||
| typeof process != "undefined" && | ||
| process && | ||
| typeof process === "object" && | ||
| process.env && | ||
| typeof process.env === "object" | ||
| ) { | ||
| if (faunaDebugEnabled(process.env["FAUNA_DEBUG"])) { | ||
| return new ConsoleLogHandler(LogLevel.DEBUG); | ||
| } | ||
| } | ||
| return new ConsoleLogHandler(LogLevel.ERROR); | ||
| } |
There was a problem hiding this comment.
Anything that the user provides in the logger option is ignored, other than checking that it is undefined. We should return the explicitly given object if provided and fall back on the env variables
| #getLogger(partialClientConfig?: ClientConfiguration): LogHandler { | |
| if ( | |
| partialClientConfig && | |
| "logger" in partialClientConfig && | |
| partialClientConfig.logger === undefined | |
| ) { | |
| throw new TypeError(`ClientConfiguration option logger must be defined.`); | |
| } | |
| if ( | |
| typeof process != "undefined" && | |
| process && | |
| typeof process === "object" && | |
| process.env && | |
| typeof process.env === "object" | |
| ) { | |
| if (faunaDebugEnabled(process.env["FAUNA_DEBUG"])) { | |
| return new ConsoleLogHandler(LogLevel.DEBUG); | |
| } | |
| } | |
| return new ConsoleLogHandler(LogLevel.ERROR); | |
| } | |
| #getLogger(partialClientConfig?: ClientConfiguration): LogHandler { | |
| if ( | |
| partialClientConfig && | |
| "logger" in partialClientConfig && | |
| partialClientConfig.logger === undefined | |
| ) { | |
| throw new TypeError(`ClientConfiguration option logger must be defined.`); | |
| } | |
| if (partialClientConfig?.logger) { | |
| return partialClientConfig.logger; | |
| } | |
| if ( | |
| typeof process !== "undefined" && | |
| process && | |
| typeof process === "object" && | |
| process.env && | |
| typeof process.env === "object" | |
| ) { | |
| const env_debug = parseDebugLevel(process.env["FAUNA_DEBUG"]); | |
| return new ConsoleLogHandler(env_debug); | |
| } | |
| return new ConsoleLogHandler(LOG_LEVELS.ERROR); | |
| } |
There was a problem hiding this comment.
I think we also want to pass in the log level provided to the ConsoleLogHandler, rather than hardcoding DEBUG and ERROR.
src/util/logging.ts
Outdated
| } | ||
|
|
||
| export class ConsoleLogHandler implements LogHandler { | ||
| constructor(private readonly level: LogLevel) { |
There was a problem hiding this comment.
prefer native private class properties over typescript magic ones. Declaring the field above the constructor will also ensure at a type level that it is assigned (which is currently not guaranteed).
| constructor(private readonly level: LogLevel) { | |
| readonly #level: LogLevel; | |
| constructor(level: LogLevel) { |
src/util/logging.ts
Outdated
| if (level === null) { | ||
| this.level = LogLevel.ERROR; | ||
| } |
There was a problem hiding this comment.
How should level be set if level is not provided or is null?
src/util/logging.ts
Outdated
| if (fauna_debug.length == 0) { | ||
| return false; | ||
| } else { | ||
| if (fauna_debug.startsWith("0")) { |
There was a problem hiding this comment.
If FAUNA_DEBUG is set to "0", e.g. with FAUNA_DEBUG=0, then how does one configure a log level of TRACE?
ptpaterson
force-pushed
the
BT-5155-logging
branch
from
cf79b32 to
8578593
Compare
src/util/logging.ts
Outdated
| @@ -0,0 +1,77 @@ | |||
| export enum LogLevel { | |||
There was a problem hiding this comment.
prefer a constant and companion union type over enum magic. We can reuse the values if they are strings (e.g. TRACE = "0" instead of TRACE = 0), but string enums have type safety issues.
|
We should put together some tests that demonstrate using logger stuff. But I've updated the things I have opinions and commented about. |
ptpaterson
force-pushed
the
BT-5155-logging
branch
from
da4ab7b to
1a68e24
Compare
findgriffin
force-pushed
the
BT-5155-logging
branch
from
1077745 to
9cffc75
Compare
src/client.ts
Outdated
| logger: this.#getLogger(clientConfiguration), | ||
| }; | ||
|
|
||
| if (clientConfiguration && clientConfiguration.logger) { | ||
| this.#logger = clientConfiguration.logger; | ||
| } else { | ||
| this.#logger = new ConsoleLogHandler(LOG_LEVELS.ERROR); | ||
| } |
There was a problem hiding this comment.
getLogger returns a logger that's turned off by default (sounds good). But then we set a logger on the client itself at error (sounds bad)? Am I reading that right?
By default we want zero logging to the console.
There was a problem hiding this comment.
oh I missed that when adding the extra log levels. It ought to be OFF.
There was a problem hiding this comment.
Made logger a required configuration and updated calls to use that instead of keeping a copy in two places.
util/logging.ts: Add LogHandler interface, LogLevel enum, and ConsoleLogHandler.
Description
Add a log handler interface and implementation.
Motivation and context
Following a LSE where we couldn't get reliable information from the customer side, we decided to add a logging facility to all of the drivers.
I spent a few hours surveying log library options in JavaScript/TypeScript. The conclusion that I came to is that they are either bulkier than what we need (log4js-node) or so simple that it's not worth us taking a dependency (pino), hence the strategy I adopted in this PR.
TODO: This PR does not yet actually call our log function from the request/response path, I thought the tests were failing on Monday. They appear to pass in GitHub Actions, so maybe that's a "fails on my machine" type of thing.
How was the change tested?
Unit tests?
Screenshots (if appropriate):
Change types
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.