Add support for logging capability - #521
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Rspack Bundle Analysis
Main CompilerSource:
Total emitted JS: 5.28 MB Runtime CompilerSource:
Total emitted JS: 4.34 MB Package Footprint (npm pack + npm install)
|
|
| Filename | Overview |
|---|---|
| packages/xmcp/src/utils/context.ts | Upgraded fallback store from a single .current reference to a stack to prevent cross-request bleed; however the provider finally block is empty, so a synchronous callback throw leaks the stack entry permanently. |
| packages/xmcp/src/runtime/utils/logger.ts | New logger implementation with per-session TTL-based log levels, isLogLevel validation, and try/catch around sendLoggingMessage; two minor performance issues: indexOf used twice per log call and O(n) session cleanup on every log statement. |
| packages/xmcp/src/runtime/transports/http/stateless-streamable-http.ts | Added per-request notification routing via _requestContextToCollectorMapping and notifications array per collector; logging/setLevel is validated with isLogLevel before being applied. |
| packages/xmcp/src/runtime/transports/http/web-stateless-http.ts | Web transport mirrors the Node.js transport changes: per-request notification buffering and validated logging/setLevel interception. |
| packages/xmcp/src/runtime/utils/transformers/tool.ts | Handler wrapped in loggerContextProvider so the logger's ALS context is available inside tool execution; McpServer import should be import type. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/xmcp/src/utils/context.ts
Line: 104-120
Comment:
**Stack not popped when callback throws synchronously**
The `finally` block is intentionally left empty, relying on the `Promise.finally()` path for async cleanup. However, if `context.run(initialValue, callback)` throws synchronously (e.g., when the callback itself throws before returning a promise), the code reaches neither the `isPromiseLike` branch nor the direct `stack.pop()` call — only the empty `finally` runs. The stack entry is permanently leaked.
Every subsequent call to `getContext()` outside the ALS chain will then see a stale context from the failed request, potentially routing log messages to the wrong session or server.
The fix is to always pop in the `finally` block for the non-async case:
```ts
const provider = <R>(initialValue: T, callback: () => R): R => {
fallbackStoreWrapper.stack.push(initialValue);
let asyncInFlight = false;
try {
const result = context.run(initialValue, callback);
if (isPromiseLike(result)) {
asyncInFlight = true;
return Promise.resolve(result).finally(() => {
fallbackStoreWrapper.stack.pop();
}) as R;
}
return result;
} finally {
if (!asyncInFlight) {
fallbackStoreWrapper.stack.pop();
}
}
};
```
In the current codebase all callers pass an `async () => {}` callback so the async path is always taken — but the function's type signature accepts synchronous callbacks and the fallback path is silently broken.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/xmcp/src/runtime/utils/logger.ts
Line: 87-92
Comment:
**Use a pre-built index Map instead of `indexOf` for level comparisons**
`shouldLog` calls `LOG_LEVELS.indexOf()` twice on every single log statement, making each check O(n) even though `n` is fixed. A module-level `Map<LogLevel, number>` built once at startup gives O(1) lookups at call time.
`LOG_LEVEL_SET` is already defined for membership checks — extend the same pattern to ordering:
```ts
const LOG_LEVEL_INDEX = new Map<LogLevel, number>(
LOG_LEVELS.map((level, i) => [level, i])
);
function shouldLog(level: LogLevel, sessionId?: string): boolean {
return (
(LOG_LEVEL_INDEX.get(level) ?? 0) >=
(LOG_LEVEL_INDEX.get(getCurrentLogLevel(sessionId)) ?? 0)
);
}
```
**Rule Used:** advanced-typescript-google/references/runtime-use-... ([source](https://app.greptile.com/review/custom-context?memory=83961b3a-d9ef-4dd6-93fa-8a5cde219f99))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/xmcp/src/runtime/utils/logger.ts
Line: 74-76
Comment:
**O(n) session cleanup on every log statement**
`cleanupExpiredSessionLogLevels` iterates the entire `sessionLogLevels` Map on every call to `getCurrentLogLevel`, which is invoked for every log statement (`logger.debug(...)`, `logger.info(...)`, etc.). As the number of tracked sessions grows, this becomes a linear scan on a hot path.
The cleanup is already triggered whenever `setLogLevel` is called (once per `logging/setLevel` request), which is a much lower-frequency path. Consider moving the cleanup there only, and keeping `getCurrentLogLevel` a pure read:
```ts
function getCurrentLogLevel(sessionId?: string): LogLevel {
if (!sessionId) return "debug";
const entry = sessionLogLevels.get(sessionId);
if (!entry) return "debug";
if (entry.expiresAt <= Date.now()) {
sessionLogLevels.delete(sessionId);
return "debug";
}
// Sliding TTL
entry.expiresAt = Date.now() + SESSION_LOG_LEVEL_TTL_MS;
return entry.level;
}
```
This checks only the current session's entry instead of scanning all sessions, and still handles expiry lazily on lookup.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/xmcp/src/runtime/utils/transformers/tool.ts
Line: 4
Comment:
**Use `import type` for type-only imports**
`McpServer` is only used as a type annotation for the `server` parameter — it has no runtime value reference. The same pattern applies to the equivalent imports in `transformers/prompt.ts` (line 4) and `transformers/resource.ts` (line 4). All three should use `import type` to keep the emitted JavaScript free of unnecessary side-effect imports and to make intent explicit:
```suggestion
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp";
```
Also applies at:
- `packages/xmcp/src/runtime/utils/transformers/prompt.ts:4`
- `packages/xmcp/src/runtime/utils/transformers/resource.ts:4`
**Rule Used:** advanced-typescript-google/references/module-use-t... ([source](https://app.greptile.com/review/custom-context?memory=a14c574a-72bb-4068-980b-d36a745a3784))
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: c6b8038
valebearzotti
left a comment
There was a problem hiding this comment.
- solve merge conflicts
| return "debug"; | ||
| } | ||
|
|
||
| // Sliding TTL: keep active sessions alive. |
There was a problem hiding this comment.
how does this session work? http requests are alive during the request alone, what does this do?
There was a problem hiding this comment.
this is tied to the mcp session id, not the raw http request lifetime. the request itself is stateless, but the client keeps sending the same mcp-session-id, and we use that to keep the chosen log level across handler executions. otherwise an exported logger would only respect logging/setLevel for the exact request that sent it, which would be wrong for actual handler usage. on the node/http path we do keep session lifecycle in memory already.
|
Implemented what was asked, and some comments about it:
also worth calling out: since logger is exported from xmcp, i treated it as a public handler api, not just an internal helper. so now it works consistently in tool/prompt/resource handlers, and if someone calls it outside an active xmcp handler context it just no-ops instead of throwing |
Summary
servers to send structured log messages to connected clients
critical, alert, emergency)
Usage
Example
For the following code
In the client with

debuglevel logsIn the client with

infolevel logs