Răsfoiți Sursa

fix: update logger calls to pino-style format across multiple packages

Yuki Takei 2 săptămâni în urmă
părinte
comite
39d0222013

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

@@ -71,6 +71,19 @@
   - Run TypeScript compilation to verify no type errors
   - _Requirements: 8.4_
 
+- [x] 5.3 Fix pino-style logger call sites in packages/slack
+  - In the following files, convert all `logger.method('message', obj)` calls to the pino-canonical form `logger.method({ obj }, 'message')` (object first, message second)
+  - `src/middlewares/verify-growi-to-slack-request.ts` (lines 25, 34)
+  - `src/middlewares/verify-slack-request.ts` (lines 25, 36, 45, 76)
+  - `src/utils/interaction-payload-accessor.ts` (line 104)
+  - Run `pnpm --filter @growi/slack lint:typecheck` and confirm zero TS2769 errors
+  - _Requirements: 10.1_
+
+- [x] 5.4 Fix pino-style logger call site in packages/remark-attachment-refs
+  - In `src/client/services/renderer/refs.ts` (line 107), convert `logger.debug('message', attributes)` to `logger.debug({ attributes }, 'message')`
+  - Run `pnpm --filter @growi/remark-attachment-refs lint:typecheck` and confirm the TS2769 error is gone
+  - _Requirements: 10.1_
+
 - [x] 6. Migrate apps/slackbot-proxy to @growi/logger
 - [x] 6.1 Replace the logger factory and HTTP middleware in slackbot-proxy
   - Update the slackbot-proxy logger utility to import from `@growi/logger` and call `initializeLoggerFactory` with its existing dev/prod config
@@ -80,6 +93,20 @@
   - Run TypeScript compilation to verify no type errors
   - _Requirements: 8.2, 6.1_
 
+- [x] 6.6 Fix pino-style logger call sites in apps/slackbot-proxy
+  - In the following files, convert all `logger.method('message', obj)` calls to `logger.method({ obj }, 'message')`
+  - `src/controllers/growi-to-slack.ts` (lines 109, 179, 231, 243, 359)
+  - `src/controllers/slack.ts` (lines 388, 586)
+  - `src/services/RegisterService.ts` (line 165)
+  - Run `pnpm --filter @growi/slackbot-proxy lint:typecheck` and confirm zero TS2769 errors
+  - _Requirements: 10.1_
+
+- [x] 6.7 Fix @growi/logger Logger type export and remove `as any` cast in slackbot-proxy
+  - In `packages/logger`, update the `loggerFactory` return type so it is compatible with `pino-http`'s `logger` option (i.e., `pino.Logger` without `<never>` narrowing, or by exporting `Logger<string>`)
+  - After the type export is fixed, remove the `as any` cast from `apps/slackbot-proxy/src/Server.ts` (line 166) and the associated `biome-ignore` comment
+  - Run `pnpm --filter @growi/slackbot-proxy lint:typecheck` to confirm no residual type errors
+  - _Requirements: 10.3_
+
 - [x] 6.5 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

+ 1 - 2
apps/slackbot-proxy/src/Server.ts

@@ -162,8 +162,7 @@ export class Server {
    * Setup logger for requests
    */
   private setupLogger(): void {
-    // biome-ignore lint/suspicious/noExplicitAny: pino-http expects Logger<string> but loggerFactory returns Logger<never>; safe at runtime
-    const httpLogger = pinoHttp({ logger: loggerFactory('express') as any });
+    const httpLogger = pinoHttp({ logger: loggerFactory('express') });
     this.app.use(httpLogger);
   }
 }

+ 5 - 5
apps/slackbot-proxy/src/controllers/growi-to-slack.ts

@@ -106,7 +106,7 @@ export class GrowiToSlackCtrl {
       .leftJoinAndSelect('relation.installation', 'installation')
       .getMany();
 
-    logger.debug(`${relations.length} relations found`, relations);
+    logger.debug({ relations }, `${relations.length} relations found`);
 
     // key: tokenGtoP, value: botToken
     const botTokenResolverMapping: { [tokenGtoP: string]: string } = {};
@@ -176,7 +176,7 @@ export class GrowiToSlackCtrl {
 
     // Returns the result of the test if it already exists
     if (relation != null) {
-      logger.debug('relation found', relation);
+      logger.debug({ relation }, 'relation found');
 
       const token = relation.installation.data.bot?.token;
       if (token == null) {
@@ -228,7 +228,7 @@ export class GrowiToSlackCtrl {
       throw createError(400, `failed to request to GROWI. err: ${err.message}`);
     }
 
-    logger.debug('order found', order);
+    logger.debug({ order }, 'order found');
 
     const token = order.installation.data.bot?.token;
     if (token == null) {
@@ -240,7 +240,7 @@ export class GrowiToSlackCtrl {
       throw createError(400, `failed to get connection. err: ${status.error}`);
     }
 
-    logger.debug('relation test is success', order);
+    logger.debug({ order }, 'relation test is success');
 
     // temporary cache for 48 hours
     const expiredAtCommands = addHours(new Date(), 48);
@@ -356,7 +356,7 @@ export class GrowiToSlackCtrl {
   ): Promise<WebclientRes> {
     const { tokenGtoPs } = req;
 
-    logger.debug('Slack API called: ', { method });
+    logger.debug({ method }, 'Slack API called: ');
 
     if (tokenGtoPs.length !== 1) {
       return res.simulateWebAPIPlatformError(

+ 5 - 2
apps/slackbot-proxy/src/controllers/slack.ts

@@ -385,7 +385,10 @@ export class SlackCtrl {
     @Res() res: Res,
     // biome-ignore lint/suspicious/noConfusingVoidType: TODO: fix in https://redmine.weseek.co.jp/issues/168174
   ): Promise<void | string | Res | WebAPICallResult> {
-    logger.info('receive interaction', req.authorizeResult);
+    logger.info(
+      { authorizeResult: req.authorizeResult },
+      'receive interaction',
+    );
     logger.debug('receive interaction', req.body);
 
     const {
@@ -583,7 +586,7 @@ export class SlackCtrl {
     const installPromise = new Promise<Installation>((resolve, reject) => {
       this.installerService.installer.handleCallback(req, serverRes, {
         success: async (installation, metadata) => {
-          logger.info('Success to install', { installation, metadata });
+          logger.info({ installation, metadata }, 'Success to install');
           resolve(installation);
         },
         failure: async (error) => {

+ 1 - 1
apps/slackbot-proxy/src/services/RegisterService.ts

@@ -162,7 +162,7 @@ export class RegisterService
       await this.insertOrderRecord(authorizeResult, interactionPayloadAccessor);
     } catch (err) {
       if (err instanceof InvalidUrlError) {
-        logger.error('Failed to register:\n', err);
+        logger.error({ err }, 'Failed to register:');
         await respond(interactionPayloadAccessor.getResponseUrl(), {
           text: 'Invalid URL',
           blocks: [markdownSectionBlock('Please enter a valid URL')],

+ 10 - 7
packages/logger/src/logger-factory.ts

@@ -14,13 +14,15 @@ const isBrowser =
 
 let moduleConfig: LoggerConfig = { default: 'info' };
 let envOverrides: Omit<LoggerConfig, 'default'> = {};
-const loggerCache = new Map<string, Logger>();
+const loggerCache = new Map<string, Logger<string>>();
 
 // Shared root logger. pino.transport() is called once here so that all
 // namespace loggers share a single Worker thread (pino's performance model).
-let rootLogger: Logger | null = null;
+let rootLogger: Logger<string> | null = null;
 
-function assertRootLogger(logger: Logger | null): asserts logger is Logger {
+function assertRootLogger(
+  logger: Logger<string> | null,
+): asserts logger is Logger<string> {
   if (logger == null) {
     throw new Error(
       'rootLogger is not initialized. Call initializeLoggerFactory() first.',
@@ -46,16 +48,17 @@ export function initializeLoggerFactory(options: LoggerFactoryOptions): void {
     // Browser: no Worker thread involved; use pino's built-in browser mode.
     // Root level is 'trace' so each child can apply its own resolved level.
     const { browser } = createBrowserOptions(isProduction);
-    rootLogger = pino({ level: 'trace', browser });
+    rootLogger = pino({ level: 'trace', browser }) as Logger<string>;
   } else {
     // Node.js: call pino.transport() ONCE here.
     // Every subsequent loggerFactory() call uses rootLogger.child() which
     // shares this single Worker thread rather than spawning a new one.
     const { transport } = createNodeTransportOptions(isProduction);
-    rootLogger =
+    rootLogger = (
       transport != null
         ? pino({ level: 'trace' }, pino.transport(transport))
-        : pino({ level: 'trace' });
+        : pino({ level: 'trace' })
+    ) as Logger<string>;
   }
 }
 
@@ -63,7 +66,7 @@ export function initializeLoggerFactory(options: LoggerFactoryOptions): void {
  * Create or retrieve a cached pino logger for the given namespace.
  * Returns a child of the shared root logger so the Worker thread is reused.
  */
-export function loggerFactory(name: string): Logger {
+export function loggerFactory(name: string): Logger<string> {
   const cached = loggerCache.get(name);
   if (cached != null) {
     return cached;

+ 4 - 3
packages/logger/src/types.ts

@@ -1,4 +1,4 @@
-import type { Logger } from 'pino';
+import type { Logger as PinoLogger } from 'pino';
 
 /**
  * Maps namespace patterns (with glob support) to log level strings.
@@ -17,5 +17,6 @@ export interface LoggerFactoryOptions {
   config: LoggerConfig;
 }
 
-// Re-export pino Logger type so consumers can type-annotate without importing pino directly
-export type { Logger };
+// Re-export pino Logger type as Logger<string> so consumers can type-annotate without importing
+// pino directly, and so the type is compatible with pino-http's logger option.
+export type Logger = PinoLogger<string>;

+ 1 - 1
packages/remark-attachment-refs/src/client/services/renderer/refs.ts

@@ -104,7 +104,7 @@ export const remarkPlugin: Plugin = () => (tree) => {
         return;
       }
 
-      logger.debug('a node detected', attributes);
+      logger.debug({ attributes }, 'a node detected');
 
       // kebab case to camel case
       attributes.maxWidth = attributes['max-width'];

+ 2 - 2
packages/slack/src/middlewares/verify-growi-to-slack-request.ts

@@ -22,7 +22,7 @@ export const verifyGrowiToSlackRequest = (
   if (str == null) {
     const message =
       "The value of header 'x-growi-gtop-tokens' must not be empty.";
-    logger.warn(message, { body: req.body });
+    logger.warn({ body: req.body }, message);
     next(createError(400, message));
     return;
   }
@@ -31,7 +31,7 @@ export const verifyGrowiToSlackRequest = (
   if (tokens.length === 0) {
     const message =
       "The value of header 'x-growi-gtop-tokens' must include at least one or more tokens.";
-    logger.warn(message, { body: req.body });
+    logger.warn({ body: req.body }, message);
     next(createError(400, message));
     return;
   }

+ 4 - 4
packages/slack/src/middlewares/verify-slack-request.ts

@@ -22,7 +22,7 @@ export const verifySlackRequest = (
 
   if (signingSecret == null) {
     const message = 'No signing secret.';
-    logger.warn(message, { body: req.body });
+    logger.warn({ body: req.body }, message);
     next(createError(400, message));
     return;
   }
@@ -33,7 +33,7 @@ export const verifySlackRequest = (
 
   if (slackSignature == null || timestamp == null) {
     const message = 'Forbidden. Enter from Slack workspace';
-    logger.warn(message, { body: req.body });
+    logger.warn({ body: req.body }, message);
     next(createError(403, message));
     return;
   }
@@ -42,7 +42,7 @@ export const verifySlackRequest = (
   const time = Math.floor(Date.now() / 1000);
   if (Math.abs(time - timestamp) > 300) {
     const message = 'Verification failed.';
-    logger.warn(message, { body: req.body });
+    logger.warn({ body: req.body }, message);
     next(createError(403, message));
     return;
   }
@@ -73,6 +73,6 @@ export const verifySlackRequest = (
   }
 
   const message = 'Verification failed.';
-  logger.warn(message, { body: req.body });
+  logger.warn({ body: req.body }, message);
   next(createError(403, message));
 };

+ 1 - 1
packages/slack/src/utils/interaction-payload-accessor.ts

@@ -101,7 +101,7 @@ export class InteractionPayloadAccessor implements IInteractionPayloadAccessor {
     try {
       parsedOriginalData = JSON.parse(originalData);
     } catch (err) {
-      logger.error('Failed to parse original data:\n', err);
+      logger.error({ err }, 'Failed to parse original data:');
       return null;
     }