Yemohyle/dedup messages telemetry #952
Conversation
roblourens
left a comment
roblourens
left a comment
There was a problem hiding this comment.
It's really sad that we have to do this at this layer but thanks for doing it! I worry that telemetry could still be too lossy to reconstruct the trajectories but I hope it works. I think I mostly follow it but just a few questions.
| telemetryService.sendInternalMSFTTelemetryEvent('engine.messages', multiplexProperties(telemetryDataWithPrompt.properties), telemetryDataWithPrompt.measurements); | ||
|
|
||
| // Send all model telemetry events (model.request.added, model.message.added, model.modelCall.input/output, model.request.options.added) | ||
| // Comment out the line below to disable the new deduplicated model telemetry events |
There was a problem hiding this comment.
Why the comment about disabling it, is that something somebody needs to do?
There was a problem hiding this comment.
Hopefully not. At the moment the existing engine.messages event and model.... events I am adding are substitutes of each other. So after conformation that the new schema works better we would disable engine.messages events. In case the new schema (shorter events but more of them) still does not help with drop rate, it may be disabled.
| * If it's the same conversationId, increments the turn. | ||
| * Returns the current conversationTurn for the conversationId. | ||
| */ | ||
| function updateConversationTracker(conversationId: string): number { |
There was a problem hiding this comment.
Isn't this wrong when you have more than one conversation going at a time?
There was a problem hiding this comment.
Are parallel conversations allowed by UI? It would be then. I changed the conversationTracker to allow for parallel conversations.
| return; | ||
| } | ||
|
|
||
| // Check if this is a conversation mode (has conversationId) or supplementary mode |
There was a problem hiding this comment.
When do we not have a conversationId?
There was a problem hiding this comment.
I do not see conversationId available when semantic_search has its own model call, or edit healing ... I called it supplementary mode.
| }).toString(); | ||
|
|
||
| // Get existing UUID for this message content + headerRequestId combination, or generate a new one | ||
| let messageUuid = messageHashToUuid.get(messageHash); |
There was a problem hiding this comment.
Can we send the hashes instead of having to map it to uuids? So that we can even track conversatations that continue after reloading the window, etc
There was a problem hiding this comment.
But I have no idea if that would be allowed.
There was a problem hiding this comment.
I am not sure I follow here. If the question is why don't we use hash itself instead of creating messageUuids? I was not sure how large those hashes could be, so used a standard uuid.
| } | ||
|
|
||
| function sendModelCallTelemetry(telemetryService: ITelemetryService, messageData: Array<{ uuid: string; headerRequestId: string }>, telemetryData: TelemetryData, messageDirection: 'input' | 'output', logService?: ILogService) { | ||
| // Get the unique model call ID |
There was a problem hiding this comment.
I don't think I understand what this function is doing, what's a "model call"?
There was a problem hiding this comment.
For every model call, there’s an input array of messages and an output array containing a single message.
This function doesn’t send the full messages directly. Instead, it sends events with an array of messageUuids (unique message identifiers).
For example, if the agent takes n turns, there are n model calls. A system message will appear at the start of each input array across all n turns. Rather than sending the entire system message n times, we send it once in a model.message.added event. Then, in each of the model.modelCall events, we only include the corresponding messageUuid and its position in the array.
…d version of new model... events Comment out the internal MSFT telemetry event sending.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors internal Microsoft telemetry for agent mode to reduce event size and eliminate duplicates by sending telemetry in smaller, deduplicated events instead of the large engine.messages events that contain entire conversation histories.
Key Changes
- Introduces new granular telemetry events (
model.request.added,model.message.added,model.modelCall.input/output,model.request.options.added) that use deduplication via LRU caches and UUID mapping - Comments out the internal MSFT
engine.messagesevent to reduce telemetry volume while keeping the enhanced GitHub telemetry - Updates the existing
engine.messages.lengthtelemetry to use a newTelemetryData.createAndMarkAsIssuedpattern instead ofextendedBy
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/platform/networking/node/chatStream.ts | Adds comprehensive deduplication logic for model telemetry events with LRU caches and UUID tracking, plus modifications to existing length telemetry |
| src/extension/conversation/vscode-node/test/interactiveSessionProvider.telemetry.test.ts | Updates integration test to verify the new model telemetry events are sent correctly |
Comments suppressed due to low confidence (1)
src/platform/networking/node/chatStream.ts:1
- This TODO comment indicates that telemetry functions are misplaced in a chat stream file. Consider moving these telemetry functions to a dedicated telemetry module to improve code organization and separation of concerns.
/*---------------------------------------------------------------------------------------------
| //telemetryService.sendInternalMSFTTelemetryEvent('engine.messages', multiplexProperties(telemetryDataWithPrompt.properties), telemetryDataWithPrompt.measurements); | ||
|
|
There was a problem hiding this comment.
Replace the commented-out code with a clear explanation of why this telemetry event was disabled, or remove it entirely if it's permanently disabled. Commented-out code can cause confusion and should be avoided in production.
| //telemetryService.sendInternalMSFTTelemetryEvent('engine.messages', multiplexProperties(telemetryDataWithPrompt.properties), telemetryDataWithPrompt.measurements); |
| //telemetryService.sendInternalMSFTTelemetryEvent('engine.messages', multiplexProperties(telemetryDataWithPrompt.properties), telemetryDataWithPrompt.measurements); | ||
|
|
||
| // Send all model telemetry events (model.request.added, model.message.added, model.modelCall.input/output, model.request.options.added) | ||
| // Comment out the line below to disable the new deduplicated model telemetry events |
There was a problem hiding this comment.
This comment appears to be development instructions rather than production documentation. Consider either removing this comment or replacing it with proper documentation explaining the purpose of the telemetry events.
| // Comment out the line below to disable the new deduplicated model telemetry events | |
| // The following line sends deduplicated model telemetry events to track user requests, message additions, model calls, and request options. |
Comment out the old telemetry event sending method for testing.
5 participants
Refactor microsoft internal telemetry for agent mode to be sent in smaller events to avoid high drop rate. This is specifically targeted to substitute engine.messages events that attempt to send entire input to each model call, which includes all the messages from the conversation history up to the moment and contains lots of duplicates