-
Notifications
You must be signed in to change notification settings - Fork 67
chore: avoid log files on disk and move them to console for containers #340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes container detection logic, removes disk-based log files in container environments, and adds presets to switch between console and file logging.
- Extract
detectContainerEnv
into a shared util - Update
Telemetry
andServer
to use container detection for choosing log presets - Rename and split
initializeLogger
intosetStdioPreset
(file logging) andsetDockerPreset
(console logging)
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/common/container.ts | New detectContainerEnv helper to detect Linux container runtime |
src/telemetry/telemetry.ts | Remove inline isContainerEnv , import and use detectContainerEnv |
src/server.ts | Replace initializeLogger with setStdioPreset /setDockerPreset based on container detection |
src/logger.ts | Split logger init into setStdioPreset and setDockerPreset |
Comments suppressed due to low confidence (3)
src/common/container.ts:3
- Add unit tests for
detectContainerEnv
to verify correct container detection across different environments (e.g., with and without/.dockerenv
files).
export async function detectContainerEnv(): Promise<boolean> {
src/logger.ts:183
- [nitpick] Public API functions
setStdioPreset
andsetDockerPreset
are undocumented; adding JSDoc comments would clarify their behavior and parameters.
export async function setStdioPreset(server: McpServer, logPath: string): Promise<void> {
src/logger.ts:190
- [nitpick] The name
setStdioPreset
implies STDIO logging but configures aDiskLogger
; consider renaming it (e.g.,setFilePreset
) to avoid confusion.
export function setDockerPreset(server: McpServer): void {
src/common/container.ts
Outdated
export async function detectContainerEnv(): Promise<boolean> { | ||
if (process.platform !== "linux") { | ||
return false; // we only support linux containers for now | ||
} | ||
|
||
if (process.env.container) { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider caching the result of detectContainerEnv
in a module‐level variable to avoid repeated filesystem checks when invoked multiple times.
export async function detectContainerEnv(): Promise<boolean> { | |
if (process.platform !== "linux") { | |
return false; // we only support linux containers for now | |
} | |
if (process.env.container) { | |
return true; | |
let cachedContainerEnv: boolean | null = null; | |
export async function detectContainerEnv(): Promise<boolean> { | |
if (cachedContainerEnv !== null) { | |
return cachedContainerEnv; | |
} | |
if (process.platform !== "linux") { | |
cachedContainerEnv = false; // we only support linux containers for now | |
return cachedContainerEnv; | |
} | |
if (process.env.container) { | |
cachedContainerEnv = true; | |
return cachedContainerEnv; |
Copilot uses AI. Check for mistakes.
Pull Request Test Coverage Report for Build 16078025857Details
💛 - Coveralls |
Proposed changes
stop storing log files on disk for containers, log them to console
Checklist