Skip to content

perf(logger): cache ColorEnabled value #4006

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf(logger): cache ColorEnabled value #4006

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/middleware/logger/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ enum LogPrefix {
}

const humanize = (times: string[]) => {
const [delimiter, separator] = [',', '.']
Copy link
Member

Choose a reason for hiding this comment

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

These lines should not be changed.

Copy link
Author

Choose a reason for hiding this comment

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

this change doesn't align with the minor tweak mentioned in the title.
I believe modern JS engines will likely inline it after a few iterations anyway. That said, it still introduces unnecessary runtime overhead.

Copy link
Member

@usualoma usualoma Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It might be better to use a different PR, but I think it would be better to do the following because there is a lot of waste in the current humanize() and time().

/**
 * Humanize the time.
 * @param {number} time - The time to humanize.
 * @returns {string} The humanized time.
 * 
 * @example
 * ```ts
 * humanize(1000) // => 1,000
 * ```
 */
const humanize = (time: number): string =>
  time < 1000 ? String(time) : String(time).replace(/(\d)(?=(\d\d\d)+(?!\d))/g, '$1,')

const time = (start: number): string => {
  const delta = Date.now() - start
  return delta < 1000 ? `${delta}ms` : `${humanize(Math.round(delta / 1000))}s`
}

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, humanize() only does its job when the request is over 1000 seconds long, but since there are no such long requests normally, I feel that humanize() itself is unnecessary.

const delimiter = ','
const separator = '.'

const orderTimes = times.map((v) => v.replace(/(\d)(?=(\d\d\d)+(?!\d))/g, '$1' + delimiter))

Expand All @@ -25,8 +26,7 @@ const time = (start: number) => {
return humanize([delta < 1000 ? delta + 'ms' : Math.round(delta / 1000) + 's'])
}

const colorStatus = (status: number) => {
const colorEnabled = getColorEnabled()
const colorStatus = (status: number, colorEnabled: boolean) => {
if (colorEnabled) {
switch ((status / 100) | 0) {
case 5: // red = error
Expand All @@ -52,13 +52,14 @@ function log(
prefix: string,
method: string,
path: string,
colorEnabled: boolean,
status: number = 0,
elapsed?: string
) {
const out =
prefix === LogPrefix.Incoming
? `${prefix} ${method} ${path}`
: `${prefix} ${method} ${path} ${colorStatus(status)} ${elapsed}`
: `${prefix} ${method} ${path} ${colorStatus(status, colorEnabled)} ${elapsed}`
Copy link
Member

Choose a reason for hiding this comment

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

I like the following implementation. This is because the colorStatus argument can be left as one. I think it's better not to have the colorEnabled argument here.

diff --git a/src/middleware/logger/index.ts b/src/middleware/logger/index.ts
index 5bb45383..a358e4c5 100644
--- a/src/middleware/logger/index.ts
+++ b/src/middleware/logger/index.ts
@@ -26,18 +26,15 @@ const time = (start: number) => {
 }

 const colorStatus = (status: number) => {
-  const colorEnabled = getColorEnabled()
-  if (colorEnabled) {
-    switch ((status / 100) | 0) {
-      case 5: // red = error
-        return `\x1b[31m${status}\x1b[0m`
-      case 4: // yellow = warning
-        return `\x1b[33m${status}\x1b[0m`
-      case 3: // cyan = redirect
-        return `\x1b[36m${status}\x1b[0m`
-      case 2: // green = success
-        return `\x1b[32m${status}\x1b[0m`
-    }
+  switch ((status / 100) | 0) {
+    case 5: // red = error
+      return `\x1b[31m${status}\x1b[0m`
+    case 4: // yellow = warning
+      return `\x1b[33m${status}\x1b[0m`
+    case 3: // cyan = redirect
+      return `\x1b[36m${status}\x1b[0m`
+    case 2: // green = success
+      return `\x1b[32m${status}\x1b[0m`
   }
   // Fallback to unsupported status code.
   // E.g.) Bun and Deno supports new Response with 101, but Node.js does not.
@@ -52,13 +49,14 @@ function log(
   prefix: string,
   method: string,
   path: string,
+  colorEnabled: boolean,
   status: number = 0,
   elapsed?: string
 ) {
   const out =
     prefix === LogPrefix.Incoming
       ? `${prefix} ${method} ${path}`
-      : `${prefix} ${method} ${path} ${colorStatus(status)} ${elapsed}`
+      : `${prefix} ${method} ${path} ${colorEnabled ? colorStatus(status) : status} ${elapsed}`
   fn(out)
 }

Copy link
Member

Choose a reason for hiding this comment

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

@usualoma, What do you think of this implementation?

Copy link
Member

Choose a reason for hiding this comment

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

${colorEnabled ? colorStatus(status) : status}

That's good, I was just thinking the same thing!

fn(out)
}

Expand All @@ -79,17 +80,18 @@ function log(
* ```
*/
export const logger = (fn: PrintFunc = console.log): MiddlewareHandler => {
const colorEnabled = getColorEnabled()
return async function logger(c, next) {
const { method, url } = c.req

const path = url.slice(url.indexOf('/', 8))

log(fn, LogPrefix.Incoming, method, path)
log(fn, LogPrefix.Incoming, method, path, colorEnabled)

const start = Date.now()

await next()

log(fn, LogPrefix.Outgoing, method, path, c.res.status, time(start))
log(fn, LogPrefix.Outgoing, method, path, colorEnabled, c.res.status, time(start))
}
}
Loading