search

Financial-Times / dotcom-reliability-kit

🪨 A well tested suite of tools designed to help FT.com applications be more reliable and measurable
MIT License
7 stars 1 forks source link

timestamp property is silently removed #1069

Closed alexmuller closed 4 months ago

alexmuller commented 4 months ago

Reported by Kara in #cp-reliability.

Node.js version: v20.10.0 Impacted package(s): logger@3.1.1 (latest)

Steps to reproduce

  1. Log something with a timestamp property
const logger = require('./packages/logger');
logger.error({timestamp: 'helloooooo', message: 'hi'})
  1. Observe that the timestamp property doesn't appear
[14:21:25.852] ERROR: hi

Expected behaviour

Dunno, maybe a warning that whatever you put in the timestamp property is gone forever?

Or (suggestion from Kara) we also rename that field to something else to keep it.

rowanmanning commented 4 months ago

The issue with renaming the field is that we'd need some logic that'll be annoying to maintain. What if that field is already set? Do we then have to warn that it's been overwritten or do we not set the timestamp if that's the case? I think it's worth investigating whether there's a way we can stop Pino overwriting this property (if that's what's happening)

rowanmanning commented 4 months ago

This is a smaller issue than I originally thought. The timestamp is only stripped when using pino-pretty, because it has to make a decision about whether to use the time or timestamp property when rendering the date.

The test in this PR illustrates that, in production, the timestamp is still present. Do we still want to do anything about this? It's a little confusing but possibly enough of an edge-case to ignore (anyone having the same issue in future will hopefully find this issue).

@apaleslimghost, @alexmuller, thoughts?

apaleslimghost commented 4 months ago

hahahaha okay wow. yeah let's close this