feat: enhance logging in githubWebhook and update logging methods in appFn #494
Conversation
WalkthroughProbot 関数内のログ出力を console.info/console.error に整理・詳細化し、deleteServer を try/catch で保護、appFn を default export に追加。Firebase Functions の githubWebhook ハンドラ起動時に logger で起動ログを出力。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub
participant FF as Firebase Function (githubWebhook)
participant PB as Probot App (appFn)
participant RS as Runner Server
participant LG as Logger
GH->>FF: Webhook 受信
FF->>LG: log("githubWebhook has started")
FF->>PB: Probot middleware 呼び出し
note over PB: workflow_job.queued 受信
PB->>LG: info("runner server is being created")
PB->>RS: Create server
RS-->>PB: Created
PB->>LG: info("Runner server has been created")
loop 可用化待機/リトライ
PB->>LG: info("Waiting / retry: n")
PB->>RS: Check availability
RS-->>PB: Available?
end
PB->>LG: info("runner became available")
alt 削除成功
PB->>RS: deleteServer()
PB->>LG: info("Successfully deleted runner")
else 削除失敗
PB->>RS: deleteServer()
PB-->>LG: error("failed to delete runner id ...")
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openci-runner/firebase/functions/src/index.ts (1)
30-30: ログメッセージにリクエストコンテキストを追加することを検討してください。起動ログは良いですが、リクエストID、メソッド、パスなどのコンテキスト情報を含めると、デバッグ時の追跡が容易になります。
以下のdiffを適用してログを強化できます：
- log("githubWebhook has started"); + log("githubWebhook has started", { + method: req.method, + path: req.path, + headers: { "x-github-delivery": req.headers["x-github-delivery"] }, + });openci-runner/firebase/functions/probot/index.ts (1)
18-18: コメントを具体的にするか削除してください。「This is for debugging.」というコメントは、デバッグの目的や理由を説明していないため、あまり有用ではありません。より具体的な説明（例：どの機能をテストしているか）を追加するか、コメントを削除することを検討してください。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
openci-runner/firebase/functions/probot/index.ts(5 hunks)openci-runner/firebase/functions/src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/probot/index.ts (2)
openci-runner/firebase/functions/probot/hetzner.ts (3)
getServerStatusById(53-64)deleteServer(9-16)initRunner(90-99)openci-runner/firebase/functions/probot/github.ts (1)
isJobRequired(4-6)
🔇 Additional comments (6)
openci-runner/firebase/functions/src/index.ts (1)
2-2: LGTM!Firebase Functions用の適切なロガーのインポートです。
openci-runner/firebase/functions/probot/index.ts (5)
26-26: LGTM!
console.infoへの変更は、ログレベルの使い分けとして適切です。
49-62: LGTM!サーバー作成と状態ポーリングのロギングが改善されています。
console.infoの一貫した使用と、各段階での明確なメッセージが追加されています。
67-67: LGTM!リトライカウントと成功メッセージのロギングが改善されています。
Also applies to: 87-87
122-127: エラーハンドリングの改善を確認してください。
deleteServer呼び出しをtry/catchでラップし、エラーをログに記録するようになったことは良い改善です。ただし、サーバー削除の失敗がワークフローを継続させる設計になっていることを確認してください。これは、クリーンアップがベストエフォートであることを意図している場合は適切です。削除失敗時の動作が意図通りであることを確認するため、以下の点を検討してください：
- 削除失敗したサーバーは手動でクリーンアップする必要がありますか？
- アラートやモニタリングで削除失敗を追跡していますか？
- 孤立したサーバーを検出・削除する仕組みはありますか？
132-132: デフォルトエクスポートは冗長ですが問題ありません。
appFnは既に名前付きエクスポートとして定義されているため、デフォルトエクスポートは冗長です。ただし、特定のインポートスタイルとの互換性のために必要な場合もあるため、問題はありません。
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openci-runner/firebase/functions/probot/index.ts (3)
40-40: ロギングの一貫性のため、console.infoに変更してください。このPRでは情報ログを
console.infoに統一していますが、ここではconsole.logのままです。一貫性を保つために変更することをお勧めします。以下のdiffを適用してください：
- console.log("This workflow job doesn't use openci runner"); + console.info("This workflow job doesn't use openci runner");
110-110: ロギングの一貫性のため、console.infoに変更してください。このPRでは情報ログを
console.infoに統一していますが、ここではconsole.logのままです。一貫性を保つために変更することをお勧めします。以下のdiffを適用してください：
- console.log("This workflow job doesn't use openci runner"); + console.info("This workflow job doesn't use openci runner");
115-115: ロギングの一貫性のため、console.infoに変更してください。このPRでは情報ログを
console.infoに統一していますが、ここではconsole.logのままです。一貫性を保つために変更することをお勧めします。以下のdiffを適用してください：
- console.log("This runner is GitHub hosted one"); + console.info("This runner is GitHub hosted one");
♻️ Duplicate comments (1)
openci-runner/firebase/functions/probot/index.ts (1)
105-105: ロギングが改善されました。
console.infoへの変更により、ロギングの一貫性が向上しています。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
openci-runner/firebase/functions/probot/index.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/probot/index.ts (1)
openci-runner/firebase/functions/probot/hetzner.ts (3)
getServerStatusById(53-64)deleteServer(9-16)initRunner(90-99)
🔇 Additional comments (5)
openci-runner/firebase/functions/probot/index.ts (5)
18-18: デバッグコメントが追加されました。デバッグ目的のコメントが追加されています。問題ありません。
26-26: ロギングの一貫性が向上しました。情報ログが
console.infoに統一されており、ロギングの一貫性が向上しています。Also applies to: 49-49, 56-56, 59-59, 67-67, 87-87
120-120: ロギングが改善されました。
console.infoへの変更により、ロギングの一貫性が向上しています。
122-127: エラーハンドリングが改善されました。
deleteServerをtry/catchで囲むことで、エラーが適切に処理され、構造化されたロギングが実現されています。エラー時にrunner idも含まれており、デバッグに役立ちます。
132-132: デフォルトエクスポートが追加されました。名前付きエクスポートに加えてデフォルトエクスポートが追加され、インポートの柔軟性が向上しています。
| console.info("Successfully start openci runner"); | ||
| break; | ||
| } catch (e) { | ||
| console.log("error, will try again in 1 second", e); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
エラーログをconsole.errorに変更してください。
エラー情報をログ出力する際は、console.errorを使用することで、ログレベルを適切に設定できます。
以下のdiffを適用してください：
- console.log("error, will try again in 1 second", e);
+ console.error("error, will try again in 1 second", e);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("error, will try again in 1 second", e); | |
| console.error("error, will try again in 1 second", e); |
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/probot/index.ts around line 90, replace the
console.log call used for error reporting with console.error so the error is
emitted at the correct log level; specifically, change the call to use
console.error and keep the same message and error argument (e.g.,
console.error("error, will try again in 1 second", e)) so the error details are
preserved in the log.
Summary by CodeRabbit