Skip to content

refactor: split loggers #111

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 25, 2025
Merged

refactor: split loggers #111

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8841667
refactor: split loggers
fmenezes Apr 24, 2025
ed64115
fix: styles
fmenezes Apr 24, 2025
3e5b6d8
refactor: CompositeLogger constructor
fmenezes Apr 24, 2025
92e247e
fix: check for loggers
fmenezes Apr 24, 2025
0c57934
Merge branch 'main' into fmenezes/refactor_log
fmenezes Apr 24, 2025
6733c9e
fix: address comments
fmenezes Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 77 additions & 28 deletions src/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import { LoggingMessageNotification } from "@modelcontextprotocol/sdk/types.js";
export type LogLevel = LoggingMessageNotification["params"]["level"];

abstract class LoggerBase {
async initialize(): Promise<void> {
return Promise.resolve();
}

abstract log(level: LogLevel, id: MongoLogId, context: string, message: string): void;

info(id: MongoLogId, context: string, message: string): void {
this.log("info", id, context, message);
}
Expand Down Expand Up @@ -47,22 +52,39 @@ class ConsoleLogger extends LoggerBase {
}
}

class Logger extends LoggerBase {
constructor(
private logWriter: MongoLogWriter,
private server: McpServer
) {
class DiskLogger extends LoggerBase {
private logWriter?: MongoLogWriter;

constructor(private logPath: string) {
super();
}

async initialize(): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having an async instance method for initializing the logger, wouldn't it be cleaner to turn it into an async factori that calls a private constructor? That way we won't have ambiguous optionality for the logWriter.

Something like:

class DiskLogger extends LoggerBase {
  private constructor(private logWriter: MongoLogWriter) {
    super();
  }

  public static async create(logPath: string) {
    ...
    const logWriter = await manager.createLogWriter();
    return new DiskLogger(logWriter);
  }
}

await fs.mkdir(this.logPath, { recursive: true });

const manager = new MongoLogManager({
directory: this.logPath,
retentionDays: 30,
onwarn: console.warn,
onerror: console.error,
gzip: false,
retentionGB: 1,
});

await manager.cleanupOldLogFiles();

this.logWriter = await manager.createLogWriter();
}

log(level: LogLevel, id: MongoLogId, context: string, message: string): void {
message = redact(message);
const mongoDBLevel = this.mapToMongoDBLogLevel(level);

if (!this.logWriter) {
throw new Error("DiskLogger is not initialized");
}

this.logWriter[mongoDBLevel]("MONGODB-MCP", id, context, message);
void this.server.server.sendLoggingMessage({
level,
data: `[${context}]: ${message}`,
});
}

private mapToMongoDBLogLevel(level: LogLevel): "info" | "warn" | "error" | "debug" | "fatal" {
Expand All @@ -86,31 +108,58 @@ class Logger extends LoggerBase {
}
}

class ProxyingLogger extends LoggerBase {
private internalLogger: LoggerBase = new ConsoleLogger();
class McpLogger extends LoggerBase {
constructor(private server: McpServer) {
super();
}

log(level: LogLevel, id: MongoLogId, context: string, message: string): void {
this.internalLogger.log(level, id, context, message);
log(level: LogLevel, _: MongoLogId, context: string, message: string): void {
void this.server.server.sendLoggingMessage({
level,
data: `[${context}]: ${message}`,
});
}
}

const logger = new ProxyingLogger();
export default logger;
class CompositeLogger extends LoggerBase {
private loggers: LoggerBase[];

export async function initializeLogger(server: McpServer, logPath: string): Promise<void> {
await fs.mkdir(logPath, { recursive: true });
constructor(...loggers: LoggerBase[]) {
super();

const manager = new MongoLogManager({
directory: logPath,
retentionDays: 30,
onwarn: console.warn,
onerror: console.error,
gzip: false,
retentionGB: 1,
});
if (loggers.length === 0) {
// default to ConsoleLogger
this.loggers = [new ConsoleLogger()];
return;
}

await manager.cleanupOldLogFiles();
this.loggers = [...loggers];
}

const logWriter = await manager.createLogWriter();
logger["internalLogger"] = new Logger(logWriter, server);
async initialize(): Promise<void> {
for (const logger of this.loggers) {
await logger.initialize();
}
}

setLoggers(...loggers: LoggerBase[]): void {
if (loggers.length === 0) {
throw new Error("At least one logger must be provided");
}
this.loggers = [...loggers];
}

log(level: LogLevel, id: MongoLogId, context: string, message: string): void {
for (const logger of this.loggers) {
logger.log(level, id, context, message);
}
}
}

const logger = new CompositeLogger();
export default logger;

export async function initializeLogger(server: McpServer, logPath: string): Promise<void> {
logger.setLoggers(new McpLogger(server), new DiskLogger(logPath));
Copy link
Collaborator

@gagik gagik Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combined with Nikola's comment, I'd move the logger from global variable into this scope and return it; that way the logger variable will only exist in a valid, initalized state similar to https://github.com/mongodb-js/mongosh/blob/0ad691f4878dc52a8782ecaf66fc46928b2613f4/packages/logging/src/logging-and-telemetry.ts#L59-L67

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to do this originally, but it became a bigger change, leaving for another PR

await logger.initialize();
}
Loading