Ver Fonte

design: update logger creation flow and performance requirements for pino migration

Yuki Takei há 1 semana atrás
pai
commit
94c379fe49

+ 23 - 6
.kiro/specs/migrate-logger-to-pino/design.md

@@ -106,24 +106,39 @@ graph TB
 
 ### Logger Creation Flow
 
+```mermaid
+sequenceDiagram
+    participant App as Application Startup
+    participant Factory as LoggerFactory
+    participant Transport as pino.transport (Worker)
+    participant Root as Root pino Logger
+
+    App->>Factory: initializeLoggerFactory(options)
+    Factory->>Transport: pino.transport(config) — spawns ONE Worker thread
+    Transport-->>Factory: transport stream
+    Factory->>Root: pino({ level: 'trace' }, transport)
+    Root-->>Factory: rootLogger stored in module scope
+```
+
 ```mermaid
 sequenceDiagram
     participant Consumer as Consumer Module
     participant Factory as LoggerFactory
     participant Cache as Logger Cache
     participant Resolver as LevelResolver
-    participant Pino as pino
+    participant Root as Root pino Logger
 
     Consumer->>Factory: loggerFactory(namespace)
     Factory->>Cache: lookup(namespace)
     alt Cache hit
-        Cache-->>Factory: cached logger
+        Cache-->>Factory: cached child logger
     else Cache miss
         Factory->>Resolver: resolveLevel(namespace, config, envOverrides)
         Resolver-->>Factory: resolved level
-        Factory->>Pino: pino(options with level, name, transport)
-        Pino-->>Factory: logger instance
-        Factory->>Cache: store(namespace, logger)
+        Factory->>Root: rootLogger.child({ name: namespace })
+        Root-->>Factory: child logger (shares Worker thread)
+        Factory->>Factory: childLogger.level = resolved level
+        Factory->>Cache: store(namespace, childLogger)
     end
     Factory-->>Consumer: Logger
 ```
@@ -157,12 +172,13 @@ flowchart TD
 | 8.1–8.5 | Multi-app consistency | @growi/logger package | Package exports | — |
 | 9.1–9.3 | Dependency cleanup | — (removal task) | — | — |
 | 10.1–10.3 | Backward-compatible API | LoggerFactory | `Logger` type export | — |
+| 11.1–11.4 | Pino performance preservation | LoggerFactory | `initializeLoggerFactory`, shared root logger | Logger Creation |
 
 ## Components and Interfaces
 
 | Component | Domain/Layer | Intent | Req Coverage | Key Dependencies | Contracts |
 |-----------|-------------|--------|--------------|-----------------|-----------|
-| LoggerFactory | @growi/logger / Core | Create and cache namespace-bound pino loggers | 1, 4, 8, 10 | pino (P0), LevelResolver (P0), TransportFactory (P0) | Service |
+| LoggerFactory | @growi/logger / Core | Create and cache namespace-bound pino loggers | 1, 4, 8, 10, 11 | pino (P0), LevelResolver (P0), TransportFactory (P0) | Service |
 | LevelResolver | @growi/logger / Core | Resolve log level for a namespace from config + env | 2, 3 | minimatch (P0), EnvVarParser (P0) | Service |
 | EnvVarParser | @growi/logger / Core | Parse env vars into namespace-level map | 3 | — | Service |
 | TransportFactory | @growi/logger / Core | Create pino transport/options for Node.js and browser | 4, 5 | pino-pretty (P1) | Service |
@@ -226,6 +242,7 @@ function loggerFactory(name: string): Logger;
 - Browser detection: `typeof window !== 'undefined' && typeof window.document !== 'undefined'`
 - In browser mode, skip transport setup and use pino's built-in `browser` option
 - The factory is a module-level singleton (module scope cache + config)
+- **Performance critical**: `pino.transport()` spawns a Worker thread. It MUST be called **once** inside `initializeLoggerFactory`, not inside `loggerFactory`. Each `loggerFactory(name)` call creates a child logger via `rootLogger.child({ name })` which shares the single Worker thread. Calling `pino.transport()` per namespace would spawn N Worker threads for N namespaces, negating pino's core performance advantage.
 
 #### LevelResolver
 

+ 10 - 0
.kiro/specs/migrate-logger-to-pino/requirements.md

@@ -107,6 +107,16 @@ This specification covers the complete migration from bunyan to pino, replacing
 2. When migration is complete, the monorepo shall have no references to `morgan` or `@types/morgan` in any `package.json`.
 3. When migration is complete, no source file shall contain imports or requires of the removed packages.
 
+### Requirement 11: Preserve Pino's Performance Characteristics
+
+**Objective:** As a developer, I want the logger implementation to honour pino's design philosophy of minimal overhead in the main thread, so that migrating from bunyan does not introduce performance regressions.
+
+#### Acceptance Criteria
+1. The Logger Factory shall create pino's worker-thread transport (`pino.transport()`) **at most once** per application lifetime (i.e., during `initializeLoggerFactory`), regardless of the number of unique namespaces.
+2. The Logger Factory shall create per-namespace loggers by calling `.child()` on a shared root pino instance, not by calling `pino()` and `pino.transport()` independently for each namespace.
+3. The Logger Factory shall not perform any blocking I/O or expensive computation on the hot path of each log method call (level-checking is performed by pino's internal mechanism and is acceptable).
+4. The number of active Worker threads used by the logger subsystem shall remain constant after the first call to `loggerFactory()`, regardless of how many distinct namespaces are subsequently requested.
+
 ### Requirement 10: Backward-Compatible Log API
 
 **Objective:** As a developer, I want the new logger to expose the same method signatures as the current bunyan logger, so that existing log call sites require minimal or no changes.

+ 8 - 0
.kiro/specs/migrate-logger-to-pino/tasks.md

@@ -80,6 +80,14 @@
   - Run TypeScript compilation to verify no type errors
   - _Requirements: 8.2, 6.1_
 
+- [ ] 6.5 [NEXT] Fix logger factory to preserve pino's single-worker-thread performance model
+  - Refactor `initializeLoggerFactory` to create the pino transport (`pino.transport()`) and root pino logger **once**, storing them in module scope
+  - Set the root logger's level to `'trace'` so that individual child loggers can apply their own resolved level without being silenced by the root
+  - Refactor `loggerFactory(name)` to call `rootLogger.child({ name })` and then set `childLogger.level = resolvedLevel` instead of calling `pino()` + `pino.transport()` per namespace
+  - Handle browser mode separately: the root browser logger is created once in `initializeLoggerFactory`; `loggerFactory` still calls `.child({ name })` and applies the resolved level
+  - Update unit tests in `logger-factory.spec.ts` to verify that calling `loggerFactory` for N distinct namespaces does not create N independent pino instances (all children share the root transport)
+  - _Requirements: 11.1, 11.2, 11.3, 11.4_
+
 - [ ] 7. Migrate apps/app to @growi/logger (largest scope)
 - [ ] 7.1 Replace the logger factory module in apps/app
   - Update the apps/app logger utility to import from `@growi/logger` instead of `universal-bunyan`