Skip to content

fix: cheaper log time function - #1305

Open
ferhatelmas wants to merge 1 commit into
masterfrom
ferhat/cheaper-time
Open

fix: cheaper log time function#1305
ferhatelmas wants to merge 1 commit into
masterfrom
ferhat/cheaper-time

Conversation

@ferhatelmas

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Using isoTime in logger provides some readability in stdout logs. Otherwise, it's always normalized by logflare. In production environment, it nearly does nothing but does allocation of date and formatting per log.

What is the new behavior?

Use epochTime (pino default).
Keeping the explicit form communicates the decision better.

Additional context

This is a free win for heavy logging.

@ferhatelmas
ferhatelmas requested a review from a team as a code owner August 7, 2026 11:18
Copilot AI review requested due to automatic review settings August 7, 2026 11:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes the base Pino logger timestamp function from ISO formatting to epoch time to reduce per-log allocation/formatting overhead, aligning with Pino’s default timestamp behavior while keeping the choice explicit in code.

Changes:

  • Switch baseLogger timestamp from pino.stdTimeFunctions.isoTime to pino.stdTimeFunctions.epochTime.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/internal/monitoring/logger.ts

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — trivial one-line perf change swapping pino's isoTime for epochTime timestamp function.

Extended reasoning...

Overview

The PR changes a single line in src/internal/monitoring/logger.ts, switching the pino timestamp option from pino.stdTimeFunctions.isoTime to pino.stdTimeFunctions.epochTime (pino's default). This avoids a Date allocation and ISO string formatting on every log line, since logs are normalized by Logflare downstream anyway.

Security risks

None. This only changes the format of the timestamp field embedded in structured logs; it does not touch auth, redaction, header allow-listing, or any other security-sensitive logic in this file.

Level of scrutiny

Very low. This is a mechanical, well-scoped one-line config swap using a built-in pino function, with an isolated blast radius (log timestamp formatting only). No behavioral change to request handling, serialization, or redaction logic.

Other factors

I checked the existing test mock in logger.test.ts, which still only stubs pino.stdTimeFunctions.isoTime. Since the mocked pino factory doesn't actually invoke the timestamp function, accessing the now-unstubbed epochTime just resolves to undefined and is passed through inertly — this doesn't cause test failures, just a stale/cosmetic mismatch in the mock, not worth blocking on.

@coveralls

coveralls commented Aug 7, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 31174343684

Coverage remained the same at 80.602%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 12756
Covered Lines: 10703
Line Coverage: 83.91%
Relevant Branches: 7571
Covered Branches: 5681
Branch Coverage: 75.04%
Branches in Coverage %: Yes
Coverage Strength: 511.88 hits per line

💛 - Coveralls

Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants