ref(logger): refactor logger impl to be separate from interface #784

seaerchin · 2023-05-29T10:05:00Z

Problem

Previously, our loggers were a thin wrapper over CloudWatchLogger and adding an external logging source was difficult. This was because our loggers depended on a concrete implementation rather than an interface.

Closes IS-209

Solution

separate the logging interface from the logger
- this can be further extended when we refactor our errors, so that instead of taking string, the method takes string | InternalError
add a main logger that exposes the Logger interface + allows us to inject loggers into it
- this also exposes a useFormatter method to allow people to customise their log formats (idk if this should be on main or sub loggers ._. feel free to discuss)
- this handles calling our sub loggers
adds a new consoleLogger in that is a simple object. prefixes w/ the level + has colours so it's obv.

Tests

hit an endpoint on our backend
there should be logs recorded, with colours

notes

init function on cloudWatchLogger was copied over as-is and this wasn't called previously but is now.
formatter was chosen to be placed on the main logger - we can change this if needed?
const defaultImport = require("module") had to be changed to const defaultImport = require("module").default due to how default imports work in es6 vs node (see here) for details.

kishore03109

[Tests]: as a quick sanity check to confirm that this is working could we push to to staging now then check tomorrow just see if visually looks ok?

src/logger/cloudwatch.logger.ts

kishore03109 · 2023-05-31T03:27:08Z

src/logger/cloudwatch.logger.ts

+
+// Constants
+// TODO: Check this env var as it is not in example
+const LOG_GROUP_NAME = `${process.env.AWS_BACKEND_EB_ENV_NAME}/nodejs.log`


Hey I dont understand this comment :(
this is in reference so some example in the documentation ah?

i actually dk too - i tried asking @alexanderleegs but he didn't know too HAHAHAHA

for context, this was in the old logger.js and i copied this over because it seemed important

Oh this comment is because me and Alex tried to find where this log exists, but we could never find it.

Might need to look again and if we conclude this is not being used anywhere, can remove

cc @alexanderleegs

this wasn't actually used until this PR - this was used in init, which was never called till now. i decided to call it because it seemed to be setting transport defaults and cloudwatch log settings ._.

src/logger/cloudwatch.logger.ts

src/logger/console.logger.ts

src/utils/media-utils.ts

kishore03109

npm run build fails :(

src/logger/cloudwatch.logger.ts

src/logger/logger.ts

seaerchin · 2023-05-31T06:17:42Z

npm run build fails :(

hmm - we actually lack a build step in ci; will be adding this.

package.json

harishv7 · 2023-05-31T06:08:09Z

src/logger/cloudwatch.logger.ts

+
+// Constants
+// TODO: Check this env var as it is not in example
+const LOG_GROUP_NAME = `${process.env.AWS_BACKEND_EB_ENV_NAME}/nodejs.log`


Oh this comment is because me and Alex tried to find where this log exists, but we could never find it.

Might need to look again and if we conclude this is not being used anywhere, can remove

cc @alexanderleegs

harishv7 · 2023-05-31T06:26:21Z

src/logger/logger.types.ts

+export type Formatter = (message: string) => string
+
+export interface Logger {
+  info: LogMethod


can we add debug & fatal as well?

sure, but i do have a few questions:

should any of those be optional? we might wish to make debug optional cos prod/staging loggers might not require those (but take note, also, that winston-cloudwatch has debug but not fatal)

what colour should we use on console logger? thinking blue for debug, bg red for fatal

should any of those be optional?
Debug & fatal can be both optional

what colour should we use on console logger? thinking blue for debug, bg red for fatal
yea that's what we usually use, sounds good!

harishv7 · 2023-06-05T07:46:56Z

src/logger/console.logger.ts

+    BLUE: "\x1b[34m%s\x1b[0m",
+  },
+  BACKGROUND: {
+    RED: "\x1b[41m%s\x1b[0m",


why is this a background.red?

thinking blue for debug, bg red for fatal

like we agreed above, fatal should be background red, no? we already use foreground red for error and this is more severe, so this is bg red.

seaerchin added 5 commits May 29, 2023 15:58

build(package): install bluebird typedefs

cdd883c

refactor(logger): shift to ts and add types

477ba4a

feat(logger): add sub loggers

254a06a

chore(logger imports): fixed import to have .default behind

fe1fa5b

test(logger): add spec

cf6f879

seaerchin requested a review from a team May 29, 2023 10:05

seaerchin added 6 commits May 29, 2023 18:40

fix(errorhandler): fix double default import

c9f9300

test(logger.spec): add spec for formatter

c71454a

chore(logger): extend logging types

33d5a55

chore(logger): add new type for readability

bfa2f9e

chore(cloudwatch logger): changed export + init to avoid spamming logs

8d0b5fc

fix(errors): use json.parse to get a representable form

2490f44

kishore03109 reviewed May 31, 2023

View reviewed changes

refactor(console.logger): shift colours into obj

2cc4bc0

seaerchin requested review from kishore03109 and a team May 31, 2023 04:22

kishore03109 reviewed May 31, 2023

View reviewed changes

src/logger/cloudwatch.logger.ts Outdated Show resolved Hide resolved

src/logger/logger.ts Outdated Show resolved Hide resolved

seaerchin added 3 commits May 31, 2023 14:03

fix(errors): use json stringify

82e5559

refactor(cloudwatch.logger): removed async + shift fallback logging

37a9933

fix(logger): use cloudwatch logger for staging

859f01a

harishv7 reviewed May 31, 2023

View reviewed changes

seaerchin added 3 commits May 31, 2023 14:37

chore(aws regeion): shift from const -> env var

3f4ee6f

chore(cloudwatch.logger): update import

3487b01

chore(logger): add more types and logger methods

14f89f4

seaerchin requested review from kishore03109, harishv7 and a team June 5, 2023 05:28

harishv7 approved these changes Jun 5, 2023

View reviewed changes

seaerchin merged commit 1d50f25 into develop Jun 5, 2023

seaerchin deleted the ref/IS-209-logger-ts branch June 5, 2023 08:13

kishore03109 mentioned this pull request Jun 8, 2023

Release/0.30.0 #796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(logger): refactor logger impl to be separate from interface #784

ref(logger): refactor logger impl to be separate from interface #784

seaerchin commented May 29, 2023 •

edited

Loading

kishore03109 left a comment

kishore03109 May 31, 2023

seaerchin May 31, 2023

seaerchin May 31, 2023

harishv7 May 31, 2023

seaerchin May 31, 2023

kishore03109 left a comment

seaerchin commented May 31, 2023

harishv7 May 31, 2023

harishv7 May 31, 2023

seaerchin May 31, 2023 •

edited

Loading

harishv7 Jun 1, 2023

harishv7 Jun 5, 2023

seaerchin Jun 5, 2023 •

edited

Loading

ref(logger): refactor logger impl to be separate from interface #784

ref(logger): refactor logger impl to be separate from interface #784

Conversation

seaerchin commented May 29, 2023 • edited Loading

Problem

Solution

Tests

kishore03109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 left a comment

Choose a reason for hiding this comment

seaerchin commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

seaerchin commented May 29, 2023 •

edited

Loading

seaerchin May 31, 2023 •

edited

Loading

seaerchin Jun 5, 2023 •

edited

Loading