code snippet provider #13018
code snippet provider #13018
Conversation
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
| snippetsKind = kind; | ||
| // //?? TODO Add telemetry for Computed vs Cached. | ||
|
|
||
| return codeSnippets ?? []; |
There was a problem hiding this comment.
Won't matter if we're the only provider, but we may want to experiment with lowering the importance if the snippets were from the cache.
|
|
||
| export async function registerCopilotContextProvider(): Promise<void> { | ||
| try { | ||
| const isCustomSnippetProviderApiEnabled = await telemetry.isExperimentEnabled("CppToolsCustomSnippetsApi"); |
There was a problem hiding this comment.
We can't do audience filtering and set this flag at the same time. Instead, on the Copilot side you'll need to modify the construction of activeExperiments in CompletionContext to check for the experiment, then update the resolver here to respect activeExperiments.
Alternatively, if the intent of this check was to act as a kill-switch then you can leave it as-is (although we'll still need the activeExperiments plumbing). We'll just need to be sure that CppToolsCustomSnippetsApi is at 100% rollout for all VS Code traffic before starting the actual experiment.
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
|
@lukka Soon you should be able to take a dependency on https://www.npmjs.com/package/@github/copilot-language-server to get appropriate typings. The types aren't quite there yet, but https://github.com/github/copilot-client/pull/6250 is adding them. |
lukka
force-pushed
the
dev/lucappa/cop-ctx-api
branch
from
1c49d45 to
991df60
Compare
lukka
force-pushed
the
dev/lucappa/cop-ctx-api
branch
from
991df60 to
22235f3
Compare
| private static readonly defaultTimeBudgetFactor: number = 0.5; | ||
| private completionContextCancellation = new vscode.CancellationTokenSource(); | ||
|
|
||
| // Get the default value if the timeout expires, but throws an exception if the token is cancelled. |
There was a problem hiding this comment.
this is not accurate anymore, needs an update
| import { randomUUID } from 'crypto'; | ||
| import * as telemetry from '../telemetry'; | ||
|
|
||
| export class CopilotContextTelemetry { |
There was a problem hiding this comment.
more appropriate name should be CopilotCompletionContextTelemetry
| telemetry.addKind(codeSnippetsKind.toString()); | ||
| const duration: number = performance.now() - startTime; | ||
| if (codeSnippets === undefined) { | ||
| out.appendLine(`Copilot: getCompletionContext(): no snkppets provided (${codeSnippetsKind.toString()}), elapsed time (ms): ${duration}`); |
| private readonly metrics: Record<string, number> = {}; | ||
| private readonly properties: Record<string, string> = {}; | ||
| private readonly id: string; | ||
| constructor(correlationId?: string) { |
There was a problem hiding this comment.
There should be a blank newline before constructor.
There was a problem hiding this comment.
I'll add it manually Should this be enforced by whatever the formatter is? I think I have enabled ESLint and formatting the document should be the "correct" one.
There was a problem hiding this comment.
I'm not sure which formatter we're using for TypeScript. We have "editor.defaultFormatter": "vscode.typescript-language-features", set in settings.json but we also have "eslint.format.enable": true, -- I've seen cases recently where the 2 formatters had different opinions and so I had to disable the vscode.typescript-language-features in order to fix the eslint issue.
I think eslint formatting is deprecated.
I'm not sure if there's a formatter rule for a newline before a member function, but feel free to add one if you know of a rule.
| telemetry.addKind(codeSnippetsKind.toString()); | ||
| const duration: number = performance.now() - startTime; | ||
| if (codeSnippets === undefined) { | ||
| out.appendLine(`Copilot: getCompletionContext(): no snkppets provided (${codeSnippetsKind.toString()}), elapsed time (ms): ${duration}`); |
There was a problem hiding this comment.
Can you round this duration to a whole number of ms? e.g. 83 instead of 83.27320000017062.
| return codeSnippets; | ||
| } catch (e) { | ||
| const err = e as Error; | ||
| out.appendLine(`Copilot: getCompletionContextWithCancellation(): Error: '${err?.message}', stack '${err?.stack}`); |
There was a problem hiding this comment.
It doesn't seem like we should output an error message on Cancel, since Cancelling seems like a non-error. Also, it seems like telemetry would want that tracked differently too.
| // //?? TODO telemetry.file(); | ||
| } | ||
|
|
||
| return []; |
There was a problem hiding this comment.
I'm seeing this "unreachable code":
| if (codeSnippets === undefined) { | ||
| out.appendLine(`Copilot: getCompletionContext(): no snkppets provided (${codeSnippetsKind.toString()}), elapsed time (ms): ${duration}`); | ||
| } else { | ||
| out.appendLine(`Copilot: getCompletionContext(): provided ${codeSnippets?.length} snippets (${codeSnippetsKind.toString()}), elapsed time (ms): ${duration}`); |
There was a problem hiding this comment.
Are you able to log the id of the completion context request here? Or is that not valid here?
There was a problem hiding this comment.
Related to this I'm seeing "Copilot: getCompletionContext(): provided 148 snippets (gotFromCacheHit)" but then I see it make a request LSP: (received) cpptools/getCompletionContext (id: 641) but I don't understand why it's sending a request if it used the cache.
There was a problem hiding this comment.
Is the cache just used to return results faster and it still sends a request to update the cache for later?
There was a problem hiding this comment.
As mentioned elsewhere, feel free to not log the id if it's not easily available. I'm not sure if it's worth sending it in the return message just for logging -- it depends on if there's any case where the nearest server-side message may not match the TypeScript message.
No description provided.