feat: add LSP extension for sending telemetry for inline chat result action by pras0131 · Pull Request #408 · aws/language-server-runtimes

pras0131 · 2025-04-03T15:02:01Z

Problem

Add LSP extension to report the result of action taken by user on inline chat response to the LSP server. As of now, the implementation will be limited to logging telemetry for the action taken by the user on the response provided - ACCEPT/REJECT/DISMISS but in future, it will be extended to have trackers (CodeDiffTracker, CodePercentageTracker) similar to what we have for inline completion result: https://github.com/aws/language-servers/blob/ecd755576df2d2e3ca4591cf44b605d7b72b0bc3/server/aws-lsp-codewhisperer/src/language-server/inline-completion/codeWhispererServer.ts#L603

Solution

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…action

justinmk3 · 2025-04-03T15:25:42Z

types/chat.ts

 export const INLINE_CHAT_REQUEST_METHOD = 'aws/chat/sendInlineChatPrompt'
 export const CONTEXT_COMMAND_NOTIFICATION_METHOD = 'aws/chat/sendContextCommands'
 export const CREATE_PROMPT_NOTIFICATION_METHOD = 'aws/chat/createPrompt'
+export const LOG_INLINE_CHAT_RESULT_NOTIFICATION_METHOD = 'aws/chat/logInlineChatResult'


Why do we need to add a method to the protocol just to log telemetry?

This should be a basic feature of every lsp sever response. We shouldn't need explicit methods just to log telemetry results.

E.g. the onInlineChatPrompt or sendInlineChatPrompt result should have enough info already which the client can then log. Or the result could include a new metric field or something like that (idk if the runtimes protocol has a recommended pattern for this, yet).

Or if onInlineChatPrompt and sendInlineChatPrompt aren't the right methods/events, then maybe a new method or event is needed. But surely adding a "logFoo" event for every feature is not a sustainable approach.

We do follow the similar pattern for inline completion result action as well here

This is to move telemetry emission to Flare, so clients have a single place to emit telemetry.

My question applies to that case too. And it's not too late to change this pattern, though it will be if we keep repeating it.

We wish to emit the telemetry to CWSPR DW as we do for other use cases through servers itself, that's why it's there. Also, there has to be some way for server to know what was accepted/rejected etc and this is one way. If not this, what and how do you suggest otherwise?

Suggestion from my first comment:

This should be a basic feature of every lsp sever response. We shouldn't need explicit methods just to log telemetry results.

Alternative suggestion:

Have one single generic "telemetry" notification. Why model specific events for telemetry? The client action is always the same: log the telemetry (and map various fields, which should be programmatic, not manually-mapped for every telemetry emission from the LSP server).

If this is for similar use case as onLogInlineCompletionSessionResultsHandler, then it needs to be implemented the proposed way.

Interaction flow with InlineCompletion/InlineChat goes like this:

Client sends request to server to get inline suggestion (aws/chat/sendInlineChatPrompt)

Server returns suggestions

Client shows suggestions in UI

User interacts with suggestions (accepts, rejects, cancels, etc.)

Client applies user choice

Client sends results and data about interaction back to server (aws/chat/logInlineChatResult)

This protocol is not for server sending info to client, but opposite - client sends a result of interaction with chat suggestions back to the server. With onLogInlineCompletionSessionResultsHandler, we need this to be able to calculate telemetry correctly on the server, for which it needs some data from client-side interaction, which is not available otherwise. Reason we did it this way for InlineCompletions was to encapsulate business logic of computing telemetry events in Language Server.

@pras0131 we need to document the purpose of each new API very clearly in code docstring, similar to LSP specs, otherwise all these changes are very confusing to reason about.

viktorsaws · 2025-04-15T09:18:39Z

types/chat.ts

+
+export type InlineChatUserDecision = 'ACCEPT' | 'REJECT' | 'DISMISS' | string

+export interface InlineChatResultParams {


@pras0131 please document every new Protocol addition and meaning of all fields in doc strings. It should be very clear what is expected from client to pass here, and we should follow LSP spec example. You can see example of documentation I did in latest PR https://github.com/aws/language-server-runtimes/pull/433/files

It's difficult to reason about all fields in Params without documentation, it is not clear why some fields are needed.

I can add a doc string to inlineChatResultNotificationType but adding a doc string to all field does not seem to be required here post the renaming of variables to fuller names as they are clearly indicate their purpose.

viktorsaws · 2025-04-15T09:21:37Z

types/chat.ts

+    numSelectedLines?: number
+    numSuggestionAddChars?: number
+    numSuggestionAddLines?: number
+    numSuggestionDelChars?: number


It's better to use full words in protocol, it's clearer and more understandable.

viktorsaws · 2025-04-15T09:22:16Z

types/chat.ts

+    codeIntent?: boolean
+    userDecision?: InlineChatUserDecision
+    responseStartLatency?: number
+    responseEndLatency?: number


Why do we need 2 values for latency? Can client just return computed latency back?

These different values are required while emitting Telemetry for InlineChatEvent.
Structure of InlineChatEvent: link

Sample usage in Eclipse: link

viktorsaws · 2025-04-15T09:22:50Z

types/chat.ts


+export interface InlineChatResultParams {
+    requestId: string
+    timestamp: Date


What is this timestamp needed for and why it is required?

Same as above, required by InlineChatEvent but I do agree that we can just simply calculate it at server end instead of client passing it. This is the time when event is sent. Sample: here

…notification type

justinmk3 · 2025-04-15T15:15:55Z

types/chat.ts

+    numberOfSelectedLines?: number
+    numberOfCharactersAddedFromSuggestion?: number
+    numberOfLinesAddedFromSuggestion?: number
+    numberOfCharactersDeletedFromSuggestion?: number
+    numberOfLinesDeletedFromSuggestion?: number


this is a new convention that doesn't follow any existing convention in this codebase. prefixing "numberOf" in front of a number just isn't discoverable or helpful.

what is discoverable, is using a common prefix that groups things in a meaningful way. here, the grouping is "suggestion".

and, the type already mentions number, so there's no need to repeat that in the name.

and it is never necessary to spell out "characters". that is extremely verbose. it's like spelling out "hypertext markup language".

Suggested change

numberOfSelectedLines?: number

numberOfCharactersAddedFromSuggestion?: number

numberOfLinesAddedFromSuggestion?: number

numberOfCharactersDeletedFromSuggestion?: number

numberOfLinesDeletedFromSuggestion?: number

selectedLines?: number

suggestionAddedChars?: number

suggestionAddedLines?: number

suggestionDeletedChars?: number

suggestionDeletedLines?: number

feat: add LSP extension for sending telemetry for inline chat result …

c064079

…action

pras0131 requested a review from a team as a code owner April 3, 2025 15:02

justinmk3 reviewed Apr 3, 2025

View reviewed changes

pras0131 and others added 4 commits April 9, 2025 14:46

Merge branch 'main' into addExtensionToEmitInlineChatMetric

77bce83

fix: correct formatting

9f457a8

Merge branch 'main' into addExtensionToEmitInlineChatMetric

50d3b14

feat: update the naming from logInlineChatResult to inlineChatResult

dc75d6d

viktorsaws reviewed Apr 15, 2025

View reviewed changes

Paras added 2 commits April 15, 2025 11:47

Merge branch 'main' into addExtensionToEmitInlineChatMetric

bd9a068

fix: full variable names, removed timestamp and added doc string for …

3d24963

…notification type

justinmk3 reviewed Apr 15, 2025

View reviewed changes

Paras added 2 commits April 16, 2025 14:20

Merge branch 'main' into addExtensionToEmitInlineChatMetric

d4aadeb

fix: refactored tha InlineChatResultParams field names

c7018c5

viktorsaws approved these changes Apr 16, 2025

View reviewed changes

pras0131 merged commit 26c7a0f into main Apr 16, 2025
4 checks passed

pras0131 deleted the addExtensionToEmitInlineChatMetric branch April 16, 2025 12:32

github-actions bot mentioned this pull request Apr 16, 2025

chore(release): release packages from branch main #446

Merged


		export type InlineChatUserDecision = 'ACCEPT' \| 'REJECT' \| 'DISMISS' \| string

		export interface InlineChatResultParams {

Conversation

pras0131 commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

License

Uh oh!

justinmk3 Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viktorsaws Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pras0131 Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pras0131 commented Apr 3, 2025 •

edited

Loading

justinmk3 Apr 3, 2025 •

edited

Loading

justinmk3 Apr 7, 2025 •

edited

Loading

justinmk3 Apr 9, 2025 •

edited

Loading

viktorsaws Apr 15, 2025 •

edited

Loading

pras0131 Apr 15, 2025 •

edited

Loading

justinmk3 Apr 15, 2025 •

edited

Loading