Repository navigation
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -230,6 +230,7 @@ function writeCallArtifact(artifact: CallLogArtifact): string | null { | |||
| try { | |||
| fs.mkdirSync(path.dirname(absPath), { recursive: true }); | |||
| fs.writeFileSync(absPath, JSON.stringify(artifact, null, 2)); | |||
| rotateCallLogs(); | |||
There was a problem hiding this comment.
WARNING: Performance concern - rotation and overflow cleanup runs synchronously after every artifact write. This includes retention-based directory cleanup and file count pruning, which could cause latency spikes under high request throughput.
Consider deferring the cleanup to a scheduled job or batching it (e.g., run cleanup every N writes or every X seconds).
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)
Files Reviewed (12 files)
Analysis: The PR correctly implements the follow-up cleanup tasks:
The single warning is about the synchronous cleanup ( |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the logging system to version 3.4.0, introducing more granular control over log retention and rotation. Key changes include the removal of redundant 'Source Request' logs from the pipeline and UI, and the addition of new environment variables (APP_LOG_MAX_FILES, CALL_LOG_MAX_ENTRIES) to cap the total number of log files on disk. The rotation logic was updated for both application logs and call log artifacts, supported by new unit tests. Feedback highlights a performance concern regarding the synchronous execution of log rotation on every request and suggests optimizing the file pruning process by leveraging the existing database index instead of performing expensive disk scans.
| @@ -230,6 +230,7 @@ function writeCallArtifact(artifact: CallLogArtifact): string | null { | |||
| try { | |||
| fs.mkdirSync(path.dirname(absPath), { recursive: true }); | |||
| fs.writeFileSync(absPath, JSON.stringify(artifact, null, 2)); | |||
| rotateCallLogs(); | |||
There was a problem hiding this comment.
Executing rotateCallLogs() synchronously on every request that persists to disk introduces a significant performance bottleneck. This function performs a full recursive scan of the call_logs directory (potentially thousands of files across multiple subdirectories) to enforce retention caps. In high-traffic scenarios, this will lead to substantial I/O overhead and increased request latency.
Consider moving this maintenance task to a background interval or a periodic schedule (e.g., once an hour) instead of running it on the hot path of every request.
| const files = fs | ||
| .readdirSync(baseDir) | ||
| .flatMap((entry) => { | ||
| const entryPath = path.join(baseDir, entry); | ||
| try { | ||
| const stat = fs.statSync(entryPath); | ||
| if (!stat.isDirectory()) return []; | ||
|
|
||
| return fs | ||
| .readdirSync(entryPath) | ||
| .filter((file) => file.endsWith(".json")) | ||
| .map((file) => { | ||
| const filePath = path.join(entryPath, file); | ||
| const fileStat = fs.statSync(filePath); | ||
| return { filePath, mtimeMs: fileStat.mtimeMs }; | ||
| }); | ||
| } catch { | ||
| return []; | ||
| } | ||
| }) | ||
| .sort((a, b) => b.mtimeMs - a.mtimeMs); |
There was a problem hiding this comment.
The implementation of cleanupOverflowCallLogFiles performs a full recursive scan of the disk (readdirSync + statSync on every file) to determine which files to prune. Since call logs are already indexed in the SQLite call_logs table, it would be much more efficient to query the database for the oldest entries exceeding the limit and delete those specific files. This would avoid expensive disk-crawling operations, especially as the number of logs approaches the 10,000 entry limit.
There was a problem hiding this comment.
Pull request overview
This PR refines the unified logging pipeline by removing the redundant SOURCE REQUEST stage, simplifying the dashboard toggle text, and adding count-based retention caps alongside existing time-based retention for both app logs and per-request call log artifacts.
Changes:
- Removed
sourceRequest/2_req_source.jsonfrom pipeline capture and related UI/API allowlists. - Added
APP_LOG_MAX_FILESandCALL_LOG_MAX_ENTRIESenv controls and implemented overflow cleanup logic. - Added unit tests covering log archive/file capping behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/log-rotation.test.mjs | New unit tests for app log config parsing + retention/count cleanup behavior. |
| tests/unit/call-log-file-rotation.test.mjs | New unit test ensuring call log pruning honors both retention days and max entries. |
| tests/unit/call-log-cap.test.mjs | Asserts newly written artifacts no longer include sourceRequest. |
| src/shared/validation/schemas.ts | Removes 2_req_source.json from translator filename validation. |
| src/shared/components/RequestLoggerV2.tsx | Renames “Detailed Logs” → “Pipeline Logs” and simplifies UI copy. |
| src/shared/components/RequestLoggerDetail.tsx | Removes “Source Request” section from the pipeline detail modal. |
| src/lib/usage/callLogs.ts | Adds CALL_LOG_MAX_ENTRIES pruning + triggers rotation after artifact writes. |
| src/lib/logRotation.ts | Adds APP_LOG_MAX_FILES and overflow cleanup for rotated app logs. |
| src/lib/logEnv.ts | Adds env getters/defaults for APP_LOG_MAX_FILES and CALL_LOG_MAX_ENTRIES. |
| src/app/api/translator/save/route.ts | Removes 2_req_source.json from server allowlist. |
| src/app/api/translator/load/route.ts | Removes 2_req_source.json from server allowlist. |
| README.md | Documents new env vars and updated rotation semantics. |
| docs/openapi.yaml | Bumps OpenAPI version to 3.4.0. |
| CHANGELOG.md | Notes follow-up logging cleanup and bumps release section to 3.4.0. |
| .env.example | Updates logging env vars to the new APP_LOG_* / CALL_LOG_* model. |
| open-sse/utils/requestLogger.ts | Removes sourceRequest payload capture and the logRawRequest method. |
| open-sse/handlers/chatCore.ts | Stops emitting the redundant raw/source request stage into pipeline logs. |
Comments suppressed due to low confidence (1)
src/shared/components/RequestLoggerDetail.tsx:102
- The detail modal’s
payloadSectionslist no longer includessourceRequest, which means older call log artifacts that already havepipelinePayloads.sourceRequestwill not display that stage in the UI. This conflicts with the PR note that historical artifacts remain readable. Consider renderingsourceRequestwhen present (or generating sections dynamically from keys) so older artifacts can still be fully inspected while new ones simply stop capturing it.
const pipelinePayloads = detail?.pipelinePayloads || null;
const payloadSections = pipelinePayloads
? [
["clientRawRequest", "Client Raw Request"],
["clientRequest", "Client Request"],
["openaiRequest", "OpenAI Request"],
["providerRequest", "Provider Request"],
["providerResponse", "Provider Response"],
["clientResponse", "Client Response"],
["error", "Pipeline Error"],
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| fs.mkdirSync(path.dirname(absPath), { recursive: true }); | ||
| fs.writeFileSync(absPath, JSON.stringify(artifact, null, 2)); | ||
| rotateCallLogs(); | ||
| return relPath; |
There was a problem hiding this comment.
writeCallArtifact() now calls rotateCallLogs() after every artifact write. Since rotateCallLogs() + cleanupOverflowCallLogFiles() do synchronous directory scans + statSync over potentially many entries, this adds per-request filesystem work on the hot path and can increase request latency under load. Consider throttling/debouncing rotation (e.g., run at most once per N seconds / N writes), or moving pruning to a background interval/job so request handling isn’t blocked by cleanup.
| - **Translator Playground** — 4 debugging modes: Playground (format translation), Chat Tester (round-trip), Test Bench (batch), Live Monitor (real-time) | ||
| - **Request Telemetry** — p50/p95/p99 latency + X-Request-Id tracing | ||
| - **File-Based Logging with Rotation** — Console interceptor captures everything to JSON log with size-based rotation | ||
| - **File-Based Logging with Rotation** — App logs rotate by size, retention days, and archive count; call log artifacts rotate by retention days and file count |
There was a problem hiding this comment.
This section documents the new rotation behavior, but other repo docs still describe the old request-logger pipeline/file layout (including 2_req_source.json and ENABLE_REQUEST_LOGS=true), e.g. docs/ARCHITECTURE.md and docs/CODEBASE_DOCUMENTATION.md. That drift can confuse operators upgrading to the unified logging model; please update or remove those older references (and any localized copies if applicable) to match the new pipeline stages and env var model.
| - **File-Based Logging with Rotation** — App logs rotate by size, retention days, and archive count; call log artifacts rotate by retention days and file count | |
| - **Unified File-Based Logging with Rotation** — Part of a single unified logging pipeline; app logs rotate by size, retention days, and archive count, and call log artifacts rotate by retention days and file count. | |
| - **Modern Logging Env Vars** — Configurable via the unified logging env-var model; older request-logger files like `2_req_source.json` and the `ENABLE_REQUEST_LOGS` flag are no longer used and are superseded by this pipeline. |
| process.env.APP_LOG_TO_FILE = "false"; | ||
| process.env.APP_LOG_FILE_PATH = "/tmp/omniroute-test-app.log"; | ||
| process.env.APP_LOG_MAX_FILE_SIZE = "64M"; | ||
| process.env.APP_LOG_RETENTION_DAYS = "14"; | ||
| process.env.APP_LOG_MAX_FILES = "12"; |
There was a problem hiding this comment.
This test hardcodes APP_LOG_FILE_PATH to /tmp/..., which will fail on Windows runners (no /tmp). Use os.tmpdir() + path.join(...) (or reuse the per-test temp dir pattern used below) to make the unit test cross-platform.
|
Merged manually into |
… release/v3.4.2
… release/v3.4.2
Why this change
The unified logging upgrade is already on
main, but there were still a few follow-up issues in the current logging flow:SOURCE REQUESTstageThis PR tightens that behavior without changing the overall unified logging design that is already in place on
main.What changed
SOURCE REQUESTfrom newly captured pipeline payloads2_req_source.jsontranslator whitelist entryDetailed LogstoPipeline LogsAPP_LOG_MAX_FILESfor rotated application log archivesCALL_LOG_MAX_ENTRIESfor per-request call log artifacts.env.example, README, CHANGELOG, and OpenAPI version-sync docsNotes
mainDATA_DIR/logs/behavior is not being reintroduced or changed heresourceRequestremain readable; new artifacts simply stop capturing that stageValidation
npm run check:docs-syncnpm run typecheck:corenode --import tsx/esm --test tests/unit/call-log-file-rotation.test.mjs tests/unit/log-rotation.test.mjs tests/unit/call-log-cap.test.mjs tests/unit/log-retention.test.mjs tests/unit/console-log-levels.test.mjsRepo baseline note
mainbranch also has an unrelated failure intests/unit/copilot-usage.test.mjs(100 !== 60ingithub copilot business seats infer business plan and hide unlimited buckets)