feat: enhance API logging and add expandable data view in logs UI#247
feat: enhance API logging and add expandable data view in logs UI#247DragonnZhang wants to merge 4 commits intobadrisnarayanan:mainfrom
Conversation
|
Thanks for the PR. The expandable log data view is a nice addition for debugging. A few things I'd like addressed before merging: Below as been updated on 8 February 2026 Issues1. Sensitive data stored in log history (High)Source: maintainer review
The Redact utility only handles email display in the UI; it doesn't sanitize message content. At minimum:
2. Index-based expansion state breaks on filter/new logs (Medium)Source: maintainer review
Use a stable identifier like 3. Error logs have a positional argument bug (Medium)Source: local analysis The // server.js:918 & 956 — `error` takes the `data` slot, withData goes to ...args
logger.error(`[API] Transaction failed...`, error, logger.withData({...}));
logger.error('[API] Error:', error, logger.withData({...}));Since 4. Unguarded
|
a9ad45b to
eaf1388
Compare
|
Thanks for rebasing onto latest Here's a recap with pointers into the current code: 1. Sensitive data stored in log history (High)
Fix: Only include full payloads when const logDetails = {
request: {
model: request.model,
stream: !!request.stream,
messageCount: request.messages?.length,
messageRoles: request.messages?.map(m => m.role),
hasSystem: !!request.system,
toolCount: request.tools?.length,
thinking: request.thinking,
// Only include full payloads in debug mode
...(logger.isDebugEnabled && {
messages: request.messages,
system: request.system,
tools: request.tools
})
}
};2. Index-based expansion state breaks on filter/new logs (Medium)
Fix: Use expandedLogs: new Set(),
toggleLog(timestamp) {
if (this.expandedLogs.has(timestamp)) {
this.expandedLogs.delete(timestamp);
} else {
this.expandedLogs.add(timestamp);
}
},
isExpanded(timestamp) {
return this.expandedLogs.has(timestamp);
},<template x-for="(log, idx) in filteredLogs" :key="log.timestamp">
...
@click="log.data ? toggleLog(log.timestamp) : null"
:class="{'bg-white/[0.05]': isExpanded(log.timestamp)}"
...
:class="{'rotate-90': isExpanded(log.timestamp)}"
...
<template x-if="log.data && isExpanded(log.timestamp)">3. Error logs positional argument bug (Medium)
logger.error(`[API] Transaction failed...`, error, logger.withData({...}));
logger.error('[API] Error:', error, logger.withData({...}));Since Fix (option A): Scan all args for the print(level, color, message, ...args) {
let actualData = null;
const formatArgs = [];
for (const arg of args) {
if (arg && typeof arg === 'object' && arg._isExtraData) {
actualData = arg.data;
} else {
formatArgs.push(arg);
}
}
// ...
}Fix (option B): Restructure the error calls to merge the error into logger.error(`[API] Transaction failed for model: ${request.model}`, logger.withData({
request: logDetails.request,
error: { message: error.message, stack: logger.isDebugEnabled ? error.stack : undefined }
}));Issues 1-3 should be addressed before merge. I have removed the remaining issues as 1-3 are of priority/merge blocking and I can address them after this PR is resolved. Happy to help if you have questions on any of these. |
c4117a8 to
4831abd
Compare
|
@jgor20 is this good enough to be merged now? |
Summary
Test plan