Skip to content

timestamp property is silently removed #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alexmuller opened this issue Jun 4, 2024 · 3 comments
Closed

timestamp property is silently removed #1069

alexmuller opened this issue Jun 4, 2024 · 3 comments
Labels
bug Something isn't working package: logger Relates to the logger package

Comments

@alexmuller
Copy link
Member

alexmuller commented Jun 4, 2024

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.

@alexmuller alexmuller added bug Something isn't working package: logger Relates to the logger package labels Jun 4, 2024
@rowanmanning
Copy link
Member

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
Copy link
Member

rowanmanning commented Jun 10, 2024

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
Copy link
Member

hahahaha okay wow. yeah let's close this

@rowanmanning rowanmanning closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
@rowanmanning rowanmanning moved this from 📥 Inbox to ❌ Not Planned in Reliability Kit Roadmap Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package: logger Relates to the logger package
Projects
Archived in project
Development

No branches or pull requests

3 participants