isomerpages / isomercms-backend

A static website builder and host for the Singapore Government
5 stars 1 forks source link

feat(pino): add logger #1207

Closed seaerchin closed 7 months ago

seaerchin commented 7 months ago

Problem

this pr adds a structured logging library (pino) to our codebase. it also supersedes winston (which hasn't been working anyway).

Closes [ISOM-772]

Solution

  1. add both pino and pino-pretty (will be described below)
  2. replaced consoleLogger implementation with a call to pino with transport as pino-pretty. pino pipes logs by default to stdout. this means that on local, the logs get transformed by pino-pretty before going to stdout, so it'll be seen as unstructured text instead of json.
  3. for production loggers, we also pipe to stdout - this gets picked up by aws and forwarded to the correct log group. this is actually how logging works now - our winston logger has been missing a transport and broken since forever. this was also chosen over using pino-cloudwatch because the latter is legacy.

Screenshot 2024-03-13 at 2 56 56 PM

see examples of the logger at work https://ogp.datadoghq.com/logs?query=service:isomer-infra%20env:%22staging%22%20sourc[…]=stream&from_ts=1710241200000&to_ts=1710244740000&live=false

seaerchin commented 7 months ago

auth spec is somehow failing but this doesn't touch it

dcshzj commented 7 months ago

@seaerchin the auth spec fixed alr, can rebase from develop

harishv7 commented 7 months ago

@seaerchin will adding of context attributes in our log statements be done in future PRs?

seaerchin commented 7 months ago

@seaerchin will adding of context attributes in our log statements be done in future PRs?

this is already done at present unless you mean per module attributes. if that's the case, we can use pino child loggers on a per module basis

timotheeg commented 7 months ago

Right, I didn't think we'd merge that as-is but it's fine, it should work and we can iterate to make things better:

Known issue so far:

Also for reference, the condition selection of loggers from env vars is a bit iffy, Armoury has recently adopted a different system which is simpler and more reliable and we can consider adopting. Reference PR here.

seaerchin commented 7 months ago

Known issue so far:

  • Logging from axios instance are not connected to APM traces. That needs to be understood and remedied

re: axios logging not connected - my hunch is that it's not being connected because we start a new trace. due to the call of tracer.use, we create a new trace and the logs are not connected to the original trace - we should be able to verify this post rollout tomorrow.

Also for reference, the condition selection of loggers from env vars is a bit iffy, Armoury has recently adopted a different system which is simpler and more reliable and we can consider adopting. Reference PR here.

actually this seems smarter hahahahahahah will change