Extend Chat and Completion Telemetry with Customization #493
Conversation
server/aws-lsp-codewhisperer/src/language-server/chat/telemetry/chatTelemetryController.ts
Outdated
Show resolved
Hide resolved
| @@ -54,13 +55,15 @@ export class ChatTelemetryController { | |||
| #activeTabId?: string | |||
| #tabTelemetryInfoByTabId: { [tabId: string]: ConversationTriggerInfo } | |||
| #currentTriggerByTabId: { [tabId: string]: TriggerType } = {} | |||
| #customizationInfoByTabAndMessageId: { [tabId: string]: { [messageId: string]: string } } | |||
There was a problem hiding this comment.
Would be using only messageId to store this value be enough? If messageId is unique uuid across all tabs, then grouping per tab is redundant
There was a problem hiding this comment.
Very good question, I also considered that but storing by message id means we have no way of knowing when a message is no longer active/relevant.
When we store messages inside their respective tabs, then we can safely clean up all the messages that are no longer active by deleting the tab entry. So this is to ensure the customizationInfoByTabAndMessageId doesn't keep growing infinitely
| ) | ||
| const customizationValue = undefinedIfEmpty(qConfig.customization) | ||
| codeWhispererService.customizationArn = customizationValue | ||
| sessionManager.getCurrentSession()?.setCustomizationArn(customizationValue) |
There was a problem hiding this comment.
What happens if customisation changes for inflight requests for current session? Can it result in new value being passed for the request?
There was a problem hiding this comment.
Given sessions are very short lived I was not able to trigger such a scenario in my testing. After some testing I will actually remove this line so that we get set customization arn on the sessions at creation time only
| // Store the customization value associated with the message | ||
| if (metric.cwsprChatMessageId && metric.codewhispererCustomizationArn) { | ||
| this.#customizationInfoByTabAndMessageId[tabId] = { | ||
| ...this.#customizationInfoByTabAndMessageId[tabId], |
There was a problem hiding this comment.
nit: destructing assignment is not needed here. If treating object as set, you can just assign new value for metric.cwsprChatMessageId in this.#customizationInfoByTabAndMessageId[tabId] directly.
There was a problem hiding this comment.
Are you suggesting to do this.#customizationInfoByTabAndMessageId[tabId][metric.cwsprChatMessageId] = metric.codewhispererCustomizationArn because that doesn't work when this.#customizationInfoByTabAndMessageId[tabId] doesn't exist yet
There was a problem hiding this comment.
ah, yeah, either create new array or push to existing one
Description
Update the chat and completion telemetry to contain customization information.
For chat we need to be careful to preserve the correct customization associated with a message, this PR introduces logic into the chat telemetry controller
Tested using the sample extension with IDC login. Verified that customization information is correctly preserved for past messages that were obtained by using customizations and verified that setting a new customization after receiving a response doesn't cause wrong telemetry for current or past chat messages.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.