Add hierarchical token support for request logger grouping #2940
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds hierarchical token support to the request logger to enable proper grouping of subagent and child requests even when AsyncLocalStorage context is lost. The implementation allows tokens to reference parent tokens, creating a tree structure that can be traversed and displayed hierarchically.
Changes:
- Added
parentTokenproperty toCapturingTokenwith helper methods (createChild,getRoot,isDescendantOf) - Exposed
currentTokengetter onIRequestLoggerto allow explicit token passing to child operations - Updated
requestLogTree.tsto build hierarchical tree structure based onparentTokenrelationships - Added comprehensive unit tests for token hierarchy and request grouping behavior
- Added documentation explaining the problem, solution, and architecture
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/platform/requestLogger/common/capturingToken.ts |
Adds parentToken field and hierarchy methods (createChild, getRoot, isDescendantOf) |
src/platform/requestLogger/node/requestLogger.ts |
Exposes currentToken getter to allow explicit token passing |
src/extension/log/vscode-node/requestLogTree.ts |
Implements buildHierarchicalTree to nest child tokens under parents |
src/platform/requestLogger/test/node/testRequestLogger.ts |
Provides test implementation of RequestLogger for unit testing |
src/platform/requestLogger/test/node/requestLogger.spec.ts |
Comprehensive tests for token hierarchy and grouping behavior |
docs/request-logger-testing.md |
Documents the context loss problem and proposed hierarchical solution |
docs/request-logger-architecture.md |
Documents the request logger architecture and components |
| - `label`: Display name for the parent tree element | ||
| - `icon`: Optional icon | ||
| - `flattenSingleChild`: Whether to flatten single-child groups | ||
| - `promoteMainEntry`: Whether to make the parent item clickable |
There was a problem hiding this comment.
The CapturingToken documentation is incomplete. Line 83-87 lists the properties of CapturingToken but omits the new parentToken property that was added in this PR. The documentation should include this new property along with its purpose for hierarchical grouping.
| - `promoteMainEntry`: Whether to make the parent item clickable | |
| - `promoteMainEntry`: Whether to make the parent item clickable | |
| - `parentToken`: Optional parent `CapturingToken` used to build hierarchical groupings of related requests in the tree |
| } | ||
|
|
||
| type TreeChildItem = ChatRequestItem | ChatElementItem | ToolCallItem; | ||
| type TreeChildItem = ChatRequestItem | ChatElementItem | ToolCallItem | ChatPromptItem; |
There was a problem hiding this comment.
The TreeChildItem type union should include ChatPromptItem to match the updated children type. However, this creates a circular type issue - TreeChildItem is used to define ChatPromptItem.children, but now ChatPromptItem is being added to TreeChildItem. This circular definition may work in TypeScript but is conceptually unclear. Consider using a recursive type definition or a separate type for the full tree structure.
| for (const [token, promptItem] of tokenToPromptItem) { | ||
| if (token.parentToken) { | ||
| // This token has a parent - try to nest it | ||
| const parentPromptItem = tokenToPromptItem.get(token.parentToken); | ||
| if (parentPromptItem) { | ||
| // Add this prompt item as a child of the parent | ||
| parentPromptItem.children.push(promptItem); | ||
| nestedTokens.add(token); |
There was a problem hiding this comment.
The buildHierarchicalTree method has a potential issue with ordering. When a token has a parent, the entire ChatPromptItem (with all its children) is added to the parent's children array in line 661. However, this happens after all the original children have already been added in the first pass (lines 640-648). This means nested ChatPromptItems will always appear at the end of their parent's children list, regardless of their actual insertion order. This could result in unexpected ordering in the tree view where child tokens appear after all sibling entries instead of in chronological order.
| createChild(label: string, icon?: string): CapturingToken { | ||
| return new CapturingToken(label, icon, false, false, this); |
There was a problem hiding this comment.
The documentation shows an incorrect signature for the createChild method. The documentation (line 117) shows it accepting only 2 parameters (label and icon), but the actual implementation (line 47 in capturingToken.ts) accepts 3 parameters: label, icon, and flattenSingleChild. The documentation should be updated to reflect the actual signature.
| createChild(label: string, icon?: string): CapturingToken { | |
| return new CapturingToken(label, icon, false, false, this); | |
| createChild(label: string, icon?: string, flattenSingleChild: boolean = false): CapturingToken { | |
| return new CapturingToken(label, icon, flattenSingleChild, false, this); |
| toJSON(): object { | ||
| const baseInfo = { | ||
| id: this.id, | ||
| kind: 'request', | ||
| type: this.entry.type, | ||
| name: this.entry.debugName | ||
| }; | ||
|
|
||
| if (this.entry.type === LoggedRequestKind.MarkdownContentRequest) { | ||
| return { | ||
| ...baseInfo, | ||
| startTime: new Date(this.entry.startTimeMs).toISOString(), | ||
| content: this.entry.markdownContent | ||
| }; | ||
| } | ||
|
|
||
| // Build metadata similar to the real RequestLogger | ||
| const metadata = { | ||
| model: this.entry.chatParams?.model, | ||
| location: this.entry.chatParams?.location, | ||
| startTime: this.entry.startTime.toISOString(), | ||
| endTime: this.entry.endTime.toISOString(), | ||
| duration: this.entry.endTime.getTime() - this.entry.startTime.getTime(), | ||
| maxResponseTokens: this.entry.chatParams?.body?.max_tokens ?? this.entry.chatParams?.body?.max_output_tokens, | ||
| }; | ||
|
|
||
| return { | ||
| ...baseInfo, | ||
| metadata, | ||
| isConversationRequest: this.entry.isConversationRequest | ||
| }; | ||
| } |
There was a problem hiding this comment.
The toJSON method in TestLoggedRequestInfo doesn't handle all LoggedRequest types. It only handles MarkdownContentRequest and then assumes all other types are ChatML requests with startTime/endTime Date properties. However, the code will fail at runtime for ChatMLCancelation, ChatMLFailure, and ChatMLSuccess requests because it tries to access this.entry.startTime and this.entry.endTime (as Date objects), but for MarkdownContentRequest type, there's only this.entry.startTimeMs (as number). The fallthrough case on line 124 will crash when it tries to call .toISOString() on undefined values. Consider adding proper type guards or handling for all LoggedRequestKind types.
| ## TreeView Grouping Logic | ||
|
|
||
| The TreeView groups entries by their `CapturingToken`: | ||
|
|
||
| ```typescript | ||
| // Simplified logic from getChildren() | ||
| for (const currReq of this.requestLogger.getRequests()) { | ||
| if (currReq.token !== lastPrompt?.token) { | ||
| // Token changed - start new group | ||
| pushLastPrompt(); | ||
| lastPrompt = ChatPromptItem.create(currReq, currReq.token, seen.has(currReq.token)); | ||
| } | ||
|
|
||
| // Add current request to the group | ||
| lastPrompt.children.push(currReqTreeItem); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The TreeView grouping logic description is outdated. The code example shows the old sequential grouping logic that was replaced by the new buildHierarchicalTree method. This section should be updated to explain how the new hierarchical grouping works, including how tokens with parentToken are nested under their parent's ChatPromptItem rather than being sequentially grouped.
No description provided.