Yuki Takei hai 2 días
pai
achega
e4378e7a54

+ 5 - 44
.kiro/specs/migrate-logger-to-pino/design.md

@@ -58,6 +58,7 @@ graph TB
         LevelResolver[LevelResolver]
         EnvParser[EnvVarParser]
         TransportSetup[TransportFactory]
+        HttpLogger[HttpLoggerFactory]
     end
 
     subgraph External[External Packages]
@@ -68,7 +69,9 @@ graph TB
     end
 
     App --> Factory
+    App --> HttpLogger
     Slackbot --> Factory
+    Slackbot --> HttpLogger
     Slack --> Factory
     Remark --> Factory
 
@@ -80,9 +83,8 @@ graph TB
     Factory --> Pino
     TransportSetup --> PinoPretty
 
-    App --> PinoHttp
-    Slackbot --> PinoHttp
-    PinoHttp --> Factory
+    HttpLogger --> Factory
+    HttpLogger --> PinoHttp
 ```
 
 **Architecture Integration**:
@@ -541,47 +543,6 @@ Logging infrastructure must be resilient — a logger failure must never crash t
 - Logger initialization errors are written to `process.stderr` directly (cannot use the logger itself)
 - No additional monitoring infrastructure required — this is the monitoring infrastructure
 
-## Testing Strategy
-
-### Unit Tests
-- `LevelResolver.resolveLevel()` with exact matches, glob patterns, env overrides, and fallback
-- `EnvVarParser.parseEnvLevels()` with various env var combinations
-- `TransportFactory.createTransportConfig()` for dev, prod, and browser environments
-- `LoggerFactory` caching behavior (same namespace returns same instance)
-- `DiagLoggerPinoAdapter` level mapping (verbose → trace)
-
-### Integration Tests
-- Logger factory initialization with real config files (dev and prod)
-- HTTP middleware logging with Express (verify request/response logging, route skipping)
-- End-to-end: `loggerFactory('growi:test')` produces correctly formatted output in dev mode
-
-### Migration Validation
-- Verify no imports of removed packages remain after migration
-- Verify all `package.json` files are clean of bunyan/morgan references
-- Verify existing log call sites work without modification (compile check + sample runtime test)
-
-## Migration Strategy
-
-```mermaid
-flowchart TD
-    Phase1[Phase 1: Create @growi/logger package] --> Phase2[Phase 2: Migrate apps/app]
-    Phase2 --> Phase3[Phase 3: Migrate apps/slackbot-proxy]
-    Phase3 --> Phase4[Phase 4: Migrate packages/slack and remark-attachment-refs]
-    Phase4 --> Phase5[Phase 5: Update OTel adapter]
-    Phase5 --> Phase6[Phase 6: Remove old dependencies]
-    Phase6 --> Phase7[Phase 7: Validation and cleanup]
-```
-
-- **Phase 1**: Build and test `@growi/logger` package independently
-- **Phase 2–4**: Migrate consumers one at a time; each migration is independently deployable
-- **Phase 5**: Update OTel adapter (isolated to one file + config)
-- **Phase 6**: Remove all bunyan/morgan packages from `package.json` files
-- **Phase 7**: Final grep verification, lint, build, test across monorepo
-
-Rollback: Each phase can be reverted independently by restoring the previous `loggerFactory` implementation and dependencies.
-
----
-
 ## Addendum: Formatting Improvements (Post-Migration)
 
 > Added 2026-03-30. The core migration is complete. This section covers log output readability improvements based on operator feedback.

+ 0 - 186
.kiro/specs/migrate-logger-to-pino/gap-analysis.md

@@ -1,186 +0,0 @@
-# Gap Analysis: migrate-logger-to-pino
-
-## Analysis Summary
-
-- **Migration status**: The bunyan → pino migration is **structurally complete** — all bunyan/morgan/browser-bunyan dependencies removed, `@growi/logger` package created, all apps integrated.
-- **Primary gap**: Log output formatting does not meet the user's readability goals. pino-pretty is configured with `singleLine: false` (multi-line), but the user wants **one-liner, morgan-like readable output** when `FORMAT_NODE_LOG=true`.
-- **HTTP request logging gap**: `pino-http` uses default message templates, producing verbose multi-line output. No `customSuccessMessage` or `customReceivedMessage` configured — unlike morgan's concise `GET /path 200 12ms` format.
-- **FORMAT_NODE_LOG semantics mismatch**: The code defaults to formatted output when unset (`isFormattedOutputEnabled()` returns `true`), but `.env.production` overrides this to `false`. The **intent** is "production = JSON by default", but the **code logic** says "unset = formatted". If `.env.production` fails to load, production gets pino-pretty instead of JSON.
-- **Effort**: S–M (1–5 days) depending on scope of format customization.
-
----
-
-## 1. Current State
-
-### What's Done (Complete)
-
-| Requirement | Status | Notes |
-|-------------|--------|-------|
-| Req 1: Logger Factory with Namespace | ✅ Done | `@growi/logger` package, `loggerFactory()`, child logger caching |
-| Req 2: Config-file log levels | ✅ Done | `config.dev.ts` / `config.prod.ts` with minimatch glob patterns |
-| Req 3: Env var overrides | ✅ Done | `DEBUG`, `TRACE`, `INFO`, `WARN`, `ERROR`, `FATAL` env vars |
-| Req 4: Platform-aware (Node/Browser) | ✅ Done | `isBrowser` detection, browser console output, `error`-only in prod |
-| Req 5: Dev vs Prod formatting | ⚠️ Partial | See Gap A and Gap B below |
-| Req 6: HTTP request logging | ⚠️ Partial | pino-http integrated but format not customized — see Gap C |
-| Req 7: OpenTelemetry integration | ✅ Done | `DiagLoggerPinoAdapter` maps verbose→trace |
-| Req 8: Multi-app consistency | ✅ Done | apps/app, slackbot-proxy, packages all use `@growi/logger` |
-| Req 9: Dependency cleanup | ✅ Done | Zero bunyan/morgan references remain |
-| Req 10: Backward-compatible API | ✅ Done | Same `.info()`, `.debug()`, etc. method signatures |
-| Req 11: Performance (single transport) | ✅ Done | Single `pino.transport()` call, `.child()` for namespaces |
-
-### Key Files
-
-| File | Purpose |
-|------|---------|
-| `packages/logger/src/transport-factory.ts` | Transport config (pino-pretty options, FORMAT_NODE_LOG) |
-| `packages/logger/src/logger-factory.ts` | Root logger creation, child logger caching |
-| `packages/logger/src/level-resolver.ts` | Namespace → level resolution |
-| `apps/app/src/server/crowi/index.ts:617-627` | pino-http middleware setup |
-| `apps/app/.env.production` | `FORMAT_NODE_LOG=false` |
-
----
-
-## 2. Identified Gaps
-
-### Gap A: `singleLine` should be `true` when `FORMAT_NODE_LOG=true`
-
-**Current**: `transport-factory.ts` always sets `singleLine: false` for pino-pretty (both dev and prod).
-
-**User expectation**: When `FORMAT_NODE_LOG=true`, output should be **one-liner** for readability — similar to morgan's concise format. Multi-line output is fine for dev (where vertical space is cheap), but in production with `FORMAT_NODE_LOG=true` the goal is a quick-glance readable log stream.
-
-**Impact**: Low. Single config value change.
-
-### Gap B: `FORMAT_NODE_LOG` default semantics
-
-**Current**: `isFormattedOutputEnabled()` returns `true` when `FORMAT_NODE_LOG` is unset.
-
-```typescript
-function isFormattedOutputEnabled(): boolean {
-  const val = process.env.FORMAT_NODE_LOG;
-  if (val === undefined || val === null) return true;  // ← unset = formatted
-  return val !== 'false' && val !== '0';
-}
-```
-
-**Problem**: The user's intent is:
-- **Production default** → structured JSON (for log aggregation)
-- **Production with `FORMAT_NODE_LOG=true`** → human-readable one-liners
-
-Currently, production JSON output depends on `.env.production` setting `FORMAT_NODE_LOG=false`. If that file isn't loaded (e.g., Docker override, env misconfiguration), production silently gets pino-pretty instead of JSON.
-
-**Options**:
-1. **Invert the default**: `isFormattedOutputEnabled()` returns `false` when unset in production. This makes JSON the true default without relying on `.env.production`.
-2. **Keep current behavior** but document the dependency on `.env.production` clearly. Simpler, lower risk of breaking CI which sets `FORMAT_NODE_LOG=true`.
-
-### Gap C: HTTP request log format (pino-http customization)
-
-**Current** (`apps/app/src/server/crowi/index.ts:618-626`):
-```typescript
-const httpLoggerOptions: PinoHttpOptions = {
-  logger: loggerFactory('express'),
-  ...(env !== 'production' && {
-    autoLogging: {
-      ignore: (req) => req.url?.startsWith('/_next/static/') ?? false,
-    },
-  }),
-};
-```
-
-No `customSuccessMessage`, `customReceivedMessage`, or `customLogLevel` configured.
-
-**User expectation**: Morgan-like concise one-liner output, e.g.:
-```
-GET /page/path 200 12ms
-```
-
-**Current pino-http default output** (with pino-pretty):
-```
-[timestamp] INFO (express): request completed
-    req: { "method": "GET", "url": "/page/path", ... }
-    res: { "statusCode": 200 }
-    responseTime: 12
-```
-
-This is **much more verbose** than morgan. To achieve morgan-like readability, `pino-http` options need:
-
-1. **`customSuccessMessage`** to produce `GET /path 200 12ms` style messages
-2. **`customReceivedMessage`** (optional, can be suppressed if only logging on response)
-3. Potentially **`customLogLevel`** to log 4xx/5xx at `warn`/`error` level
-
-**Complexity**: Medium. Requires understanding pino-http's callback API and testing the output format.
-
-### Gap D: Dev mode formatting vs production formatting distinction
-
-**Current**: Dev and production (with FORMAT_NODE_LOG=true) use the **exact same pino-pretty config**. There's no distinction in formatting between the two.
-
-**Opportunity**: Dev could keep `singleLine: false` (multi-line, with full context) while `FORMAT_NODE_LOG=true` in production uses `singleLine: true` (concise). This gives developers full detail locally and operators quick-glance output in production.
-
----
-
-## 3. Implementation Approach Options
-
-### Option A: Minimal — Fix singleLine + pino-http message format
-
-**Scope**: Change `singleLine: false` → `true` in the production pino-pretty path, and add `customSuccessMessage` to pino-http.
-
-**Files to change**:
-- `packages/logger/src/transport-factory.ts` — separate dev and prod pino-pretty options
-- `packages/logger/src/transport-factory.spec.ts` — update tests
-- `apps/app/src/server/crowi/index.ts` — add pino-http message customization
-- `apps/slackbot-proxy/src/Server.ts` — same pino-http customization
-
-**Trade-offs**:
-- ✅ Minimal changes, low risk
-- ✅ Directly addresses the user's readability concern
-- ❌ Does not fix the FORMAT_NODE_LOG default semantics (Gap B)
-
-### Option B: Full — Fix formatting + FORMAT_NODE_LOG semantics
-
-**Scope**: Everything in Option A, plus invert `isFormattedOutputEnabled()` to return `false` when unset in production.
-
-**Additional files**:
-- `packages/logger/src/transport-factory.ts` — change default logic
-- `packages/logger/src/transport-factory.spec.ts` — update tests
-- `apps/app/.env.production` — can remove `FORMAT_NODE_LOG=false` (now redundant)
-
-**Trade-offs**:
-- ✅ Production JSON output is guaranteed without `.env.production` dependency
-- ✅ Cleaner semantics: "opt-in to formatting" rather than "opt-out of formatting"
-- ❌ CI env (`FORMAT_NODE_LOG=true`) is not affected (still explicitly set)
-- ❌ Slightly more risk — need to verify all environments load correctly
-
-### Option C: Hybrid — Fix formatting now, defer semantics
-
-**Scope**: Option A now. Document Gap B as a known issue for later resolution.
-
-**Trade-offs**:
-- ✅ Fastest to deliver visible improvement
-- ✅ Defers semantic change to a separate PR for safer review
-- ❌ `.env.production` dependency remains
-
----
-
-## 4. Effort & Risk Assessment
-
-| Aspect | Estimate | Justification |
-|--------|----------|---------------|
-| **Effort** | **S** (1–2 days) for Option A; **M** (3–4 days) for Option B | Formatting changes are config-level; pino-http customization requires testing output |
-| **Risk** | **Low** | All changes are in logging formatting — no business logic impact. pino-http customization is well-documented. |
-
----
-
-## 5. Research Needed (for Design Phase)
-
-1. **pino-http `customSuccessMessage` API**: Confirm the exact callback signature and available fields (`req`, `res`, `responseTime`) in pino-http v11.
-2. **pino-pretty `messageFormat` option**: Could be used to format the `name` (namespace) field inline for even more concise output.
-3. **pino-http `customLogLevel`**: Decide whether 4xx → `warn` and 5xx → `error` mapping is desired (morgan didn't differentiate).
-4. **`autoLogging.ignore` in production**: Currently only applied in dev. Should certain paths (e.g., health checks) also be suppressed in production?
-
----
-
-## 6. Recommendations
-
-1. **Start with Option A (Minimal)** — delivers the most visible improvement (one-liner logs, morgan-like HTTP format) with minimal risk.
-2. **Prioritize Gap C (pino-http format)** — this is the most impactful change for the user's stated goal of "morgan-like one-liner readability".
-3. **Address Gap B (FORMAT_NODE_LOG semantics) separately** — it's a semantic correctness issue, not a readability issue, and can be handled in a follow-up.
-4. **Keep dev mode as multi-line** (`singleLine: false`) — developers benefit from seeing full context; the one-liner optimization is for production/FORMAT_NODE_LOG scenarios.

+ 29 - 0
.kiro/specs/migrate-logger-to-pino/research.md

@@ -186,6 +186,35 @@
   - However, inverting the default is a behavioral change with broader implications
 - **Decision**: Defer to separate PR. Current behavior is correct in practice (`.env.production` always loaded by Next.js dotenv-flow).
 
+## Phase 3: Implementation Discoveries
+
+### Browser Bundle Compatibility — pino-http Top-Level Import
+- **Context**: `pino-http` was initially imported at the module top-level in `http-logger.ts`. This caused Turbopack to include the Node.js-only module in browser bundles, producing `TypeError: __turbopack_context__.r(...).symbols is undefined`.
+- **Root cause**: `@growi/logger` is imported by shared page code that runs in both browser and server contexts. Any top-level import of a Node.js-only module (like pino-http) gets pulled into the browser bundle.
+- **Fix**: Move the `pino-http` import inside the async function body using dynamic import: `const { default: pinoHttp } = await import('pino-http')`. This defers the import to runtime when the function is actually called (server-side only).
+- **Pattern**: This is the standard pattern for Node.js-only modules in packages shared with browser code. Apply the same treatment to any future Node.js-only additions to `@growi/logger`.
+
+### Dev-Only Module Physical Isolation (`src/dev/`)
+- **Context**: `bunyan-format.ts` (custom pino transport) and `morgan-like-format-options.ts` were initially placed at `src/transports/` and `src/` root respectively, mixed with production modules.
+- **Problem**: No clear boundary between dev-only and production-safe modules; risk of accidentally importing dev modules in production paths.
+- **Fix**: Created `src/dev/` directory as the explicit boundary for development-only modules. `TransportFactory` references `./dev/bunyan-format.js` only in the dev branch — the path is never constructed in production code paths.
+- **Vite config**: `preserveModules: true` ensures `src/dev/bunyan-format.ts` builds to `dist/dev/bunyan-format.js` with the exact path that `pino.transport({ target: ... })` references at runtime.
+
+### Single Worker Thread Model — Critical Implementation Detail
+- **Context**: Initial implementation called `pino.transport()` inside `loggerFactory(name)`, spawning a new Worker thread for each namespace.
+- **Fix**: Refactored so `pino.transport()` is called **once** in `initializeLoggerFactory`, and `loggerFactory(name)` calls `rootLogger.child({ name })` to create namespace-bound loggers sharing the single Worker thread.
+- **Root logger level**: Must be set to `'trace'` (not `'info'`) so child loggers can independently set their resolved level without being silenced by the root. If the root is `'info'`, a child with `level: 'debug'` will still be filtered at the root level.
+- **Constraint for future changes**: Never call `pino.transport()` or `pino()` inside `loggerFactory()`. All transport setup belongs in `initializeLoggerFactory()`.
+
+### pino Logger Type Compatibility with pino-http
+- **Context**: `loggerFactory()` returned `pino.Logger<never>` (the default), which is not assignable to pino-http's expected `Logger` type.
+- **Fix**: Export `Logger<string>` from `@growi/logger` and type `loggerFactory` to return `Logger<string>`. This is compatible with pino-http's `logger` option.
+- **Why `<string>` not `<never>`**: pino's default generic `CustomLevels` is `never`, which makes the type incompatible with APIs expecting custom levels to potentially be strings. `Logger<string>` is the correct type for external APIs.
+
+### `@growi/logger` Package Visibility
+- **Decision**: `"private": true` is correct and intentional.
+- **Rationale**: All consumers (`apps/app`, `apps/slackbot-proxy`, `packages/slack`, etc.) are monorepo-internal packages that reference `@growi/logger` via `workspace:*` protocol. The `private` flag only prevents npm publish, not workspace usage. `@growi/logger` is logging infrastructure — there is no reason to expose it externally (unlike `@growi/core` or `@growi/pluginkit` which are published for external plugin developers).
+
 ## References
 - [pino API docs](https://github.com/pinojs/pino/blob/main/docs/api.md)
 - [pino browser docs](https://github.com/pinojs/pino/blob/main/docs/browser.md)

+ 3 - 2
.kiro/specs/migrate-logger-to-pino/spec.json

@@ -1,9 +1,10 @@
 {
   "feature_name": "migrate-logger-to-pino",
   "created_at": "2026-03-23T00:00:00.000Z",
-  "updated_at": "2026-04-02T00:00:00.000Z",
+  "updated_at": "2026-04-06T00:00:00.000Z",
   "language": "en",
-  "phase": "tasks-generated",
+  "phase": "implementation-complete",
+  "cleanup_completed": true,
   "approvals": {
     "requirements": {
       "generated": true,