telemetry(lsp): Integrate language server/manifest resolver telemetry #6385

Hweinstock · 2025-01-16T23:10:10Z

Problem

LSP downloading processes does not emit any telemetry.

Solution

Refactoring

separate verification from core downloading steps so that we can capture it as its own telemetry event.
Note: this refactor means that if all the downloaded content cannot be verified, no files will be written to disk.
Introduce abstraction of StageResolver to make telemetry instrumentation more natural.

Metric Behavior

metric for each stage of the LSP setup process.
can be emitted multiple times per stage to capture specific error codes. See tests for examples.
Commons repo PR: telemetry(lsp): standardize setup metric and types aws-toolkit-common#961
bumped commons version to 1.0.296 to include this change.

Testing

adds basic tests for ManifestResolver in packages/core/src/test/shared/lsp/manifestResolver.test.ts.
adds basic tests for LanguageServerResolver in packages/core/src/test/shared/lsp/lspResolver.test.ts.
Currently LanguageServerResolver only supports mac, so its tests skip on non-mac platforms. There is a techdebt test to address this.

Future Work

Update logging messages to be LSP specific.

Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
Your code changes must meet the guidelines in CONTRIBUTING.md.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2025-01-16T23:10:24Z

This pull request modifies code in src/* but no tests were added/updated.
- Confirm whether tests should be added or ensure the PR description explains why tests are not required.

packages/core/src/shared/telemetry/vscodeTelemetry.json

## Problem Relevant PR: aws/aws-toolkit-vscode#6385 ## Solution - standardize the metrics across the toolkits.  ## License By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Shruti Sinha <[email protected]>

packages/core/package.nls.json

jpinkney-aws · 2025-02-03T16:17:07Z

packages/core/src/amazonq/lsp/lspController.ts

+        await lspSetupStage('all', async () => {
+            const installResult = await new WorkspaceLSPResolver().resolve()
+            await lspSetupStage('launch', async () => activateLsp(context, installResult.resourcePaths))
+            getLogger().info('LspController: LSP activated')


Not necessarily an issue for this PR, but I think we should probably scope these messages to regular codewhisperer language server vs workspace context one

Yeah, I think we could make use of the logging header here that references which LSP it is. Can do as a follow up.

jpinkney-aws · 2025-02-03T16:17:52Z

packages/core/src/shared/lsp/lspResolver.ts

-                await fs.delete(cacheDirectory, {
-                    recursive: true,
-                })
+        const serverResolvers: StageResolver<LspResult>[] = [


not neccessarily a problem but just wanted to call it out for anyone reading -- this is where we will slightly deviate from eclipse/visual studios implementations. We can just do some things a bit simpler

In what ways does this deviate from their implementations? Would it be easier if I modified the implementation to closer mirror those implementations?

Just the structure of having resolvers. It's not a big deal. I just copied Visual Studios implementation and basically ported it to typescript. AFAIK Eclipse ported Visual Studio's implementation for this code to java. This is just a slight refactor of their work

jpinkney-aws · 2025-02-03T16:20:58Z

packages/core/src/test/shared/lsp/lspResolver.test.ts

+        if (!isMac) {
+            this.skip()
+        }
+        localStub.resolves(lspResult('cache'))


there are technically non stubbed versions of these tests in the integ or e2e folder btw. It might make sense to just add telemetry assertions there

Where are these tests? I am having some trouble finding them.

https://github.com/aws/aws-toolkit-vscode/blob/feature/amazonqLSP/packages/amazonq/test/e2e/lsp/lspInstaller.test.ts#L73

Added the telemetry to this test, and fixed an edge case with validate emitting when the fetch failed. There are definitely some paths not covered in the e2e tests so it might make sense to keep the other tests as well. However, I'm open to deleting them if you don't see value there.

What functionality is covered in this one thats not in those other ones?

Hweinstock · 2025-02-04T17:41:43Z

/runIntegrationTests

jpinkney-aws · 2025-02-04T19:49:47Z

Ah looks like the e2e tests are failing with the thing you mentioned in standup

Hweinstock · 2025-02-04T20:56:45Z

Oh interesting. I suspect this might be something else since I stubbed the workspaceLSPInstaller. The remote manifest fetch fails on local, but works in CI the first time then fails. Not sure why, but appears to be fairly consistent. I'll adjust the tests and add a note that it doesn't work locally.

jpinkney-aws · 2025-02-04T22:48:38Z

packages/amazonq/test/e2e/lsp/lspInstaller.test.ts

@@ -117,6 +121,86 @@ describe('AmazonQLSPInstaller', () => {
            assert.ok(fallback.assetDirectory.startsWith(tempDir))
            assert.deepStrictEqual(fallback.location, 'fallback')
            assert.ok(semver.satisfies(fallback.version, supportedLspServerVersions))
+
+            // Exclude version numbers so that this test doesn't have to be updated on each update.
+            // Fetching manifest fails locally, but passes in CI. Therefore, this test will fail locally.


Can you explain more about this? The test will fail locally?

jpinkney-aws · 2025-02-04T22:49:43Z

packages/core/src/test/shared/lsp/lspResolver.test.ts

+        if (!isMac) {
+            this.skip()
+        }
+        localStub.resolves(lspResult('cache'))


What functionality is covered in this one thats not in those other ones?

Hweinstock added 4 commits January 16, 2025 14:33

define metric within VSC telemetry

75a4d43

implement telemetry for fetching stage

191e0ea

lift telemetry logic to top level

0b4cec4

add telemetry for remaining stages

3727ea9

justinmk3 changed the title ~~telemetry(lsp): Integrate langauge server/manifest resolver telemetry~~ telemetry(lsp): Integrate language server/manifest resolver telemetry Jan 17, 2025

justinmk3 reviewed Jan 17, 2025

View reviewed changes

packages/core/src/shared/telemetry/vscodeTelemetry.json Outdated Show resolved Hide resolved

Hweinstock added 2 commits January 17, 2025 09:50

seperate verification and downloading

5d5a7d8

add telemetry for validation step

a7b446d

Hweinstock force-pushed the telemetry/setup branch from 7f1f21d to f581b17 Compare January 17, 2025 15:49

rename metrics and types

3304e99

Hweinstock force-pushed the telemetry/setup branch 2 times, most recently from 94902b9 to 87913ca Compare January 17, 2025 16:07

reduce duplication across stage implementations

45b0236

Hweinstock force-pushed the telemetry/setup branch from 87913ca to 45b0236 Compare January 17, 2025 16:17

undo unnecessary change

c2596cf

jpinkney-aws reviewed Jan 20, 2025

View reviewed changes

packages/core/src/shared/telemetry/vscodeTelemetry.json Outdated Show resolved Hide resolved

Merge branch 'feature/amazonqLSP' into telemetry/setup

4f2b662

Hweinstock mentioned this pull request Jan 21, 2025

telemetry(lsp): standardize setup metric and types aws/aws-toolkit-common#961

Merged

switch to fallback instead of cache when remote option fails

6ef6633

Hweinstock added 3 commits January 23, 2025 15:17

fix telemetry metadata field

3b3b39b

port metrics down from commons until release

abdb5fd

implement logic for manifest telemetry

0aadddf

Hweinstock changed the title ~~telemetry(lsp): Integrate language server/manifest resolver telemetry~~ telemetry(lsp): Integrate language server/manifest resolver telemetry (WIP) Jan 23, 2025

Hweinstock added 4 commits January 24, 2025 09:37

Merge branch 'feature/amazonqLSP' into telemetry/setup

f03bf8d

telemetry: implement logic for server resolution

7d05c68

refactor: move tryFunctions to core and include tests

c7b97e2

telemtry: implement telemetry for amazonQLSP

5284763

Hweinstock force-pushed the telemetry/setup branch from 7ed2bb7 to 5284763 Compare January 24, 2025 18:19

Hweinstock added 5 commits January 27, 2025 11:17

refactor: move telemetry work to more general location

9e708fd

fix: change outdated reference

0b4da9d

merge: bring in HttpResourceFetcher changes

c37ef4a

refactor: rename stage -> setupStage

9cee3e7

fix: add workaround for windows tests

05f3a2b

Hweinstock force-pushed the telemetry/setup branch from 6e3b972 to 05f3a2b Compare January 27, 2025 17:34

Hweinstock added 6 commits January 27, 2025 12:56

test: disable lspResolver test on nonmac, add techdebt test

80fa5d6

jscpd: ignore verbose telemetry checks

8ebcc97

refactor: avoid duplicate import line

3cf254a

merge: handle tsUtils conflicts

2c60608

deps: bump telemetry version and remove local changes

9b9443b

refactor: cleanup minor details

077e402

Hweinstock changed the title ~~telemetry(lsp): Integrate language server/manifest resolver telemetry (WIP)~~ telemetry(lsp): Integrate language server/manifest resolver telemetry Jan 28, 2025

Hweinstock marked this pull request as ready for review January 28, 2025 16:31

Hweinstock requested review from a team as code owners January 28, 2025 16:31

jpinkney-aws mentioned this pull request Jan 29, 2025

feat(amazonq): Add language server support for codewhisperer #6462

Open

Hweinstock added 2 commits January 29, 2025 13:22

merge: resolve conflicts

0ef9f0d

refactor: fix spelling mistake

b0a37f3

jpinkney-aws reviewed Feb 3, 2025

View reviewed changes

Hweinstock added 3 commits February 3, 2025 13:54

fix: remove accidental space

42ed745

Merge branch 'feature/amazonqLSP' into telemetry/setup

ea4e35a

test: add telemetry to e2e tests

d286070

jpinkney-aws approved these changes Feb 4, 2025

View reviewed changes

Hweinstock added 2 commits February 4, 2025 16:05

test: adjust telemetry to mirror CI reality

3888ac5

test: adjust results to mirror ci

4819d49

jpinkney-aws reviewed Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

telemetry(lsp): Integrate language server/manifest resolver telemetry #6385

telemetry(lsp): Integrate language server/manifest resolver telemetry #6385

Hweinstock commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025

jpinkney-aws Feb 3, 2025

Hweinstock Feb 3, 2025 •

edited

Loading

jpinkney-aws Feb 3, 2025

Hweinstock Feb 3, 2025

jpinkney-aws Feb 3, 2025

jpinkney-aws Feb 3, 2025

Hweinstock Feb 3, 2025

jpinkney-aws Feb 3, 2025

Hweinstock Feb 4, 2025

jpinkney-aws Feb 4, 2025

Hweinstock commented Feb 4, 2025 •

edited

Loading

jpinkney-aws commented Feb 4, 2025

Hweinstock commented Feb 4, 2025 •

edited

Loading

jpinkney-aws Feb 4, 2025

jpinkney-aws Feb 4, 2025

telemetry(lsp): Integrate language server/manifest resolver telemetry #6385

Are you sure you want to change the base?

telemetry(lsp): Integrate language server/manifest resolver telemetry #6385

Conversation

Hweinstock commented Jan 16, 2025 • edited Loading

Problem

Solution

Refactoring

Metric Behavior

Testing

Future Work

github-actions bot commented Jan 16, 2025

Choose a reason for hiding this comment

Hweinstock Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hweinstock commented Feb 4, 2025 • edited Loading

jpinkney-aws commented Feb 4, 2025

Hweinstock commented Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hweinstock commented Jan 16, 2025 •

edited

Loading

Hweinstock Feb 3, 2025 •

edited

Loading

Hweinstock commented Feb 4, 2025 •

edited

Loading

Hweinstock commented Feb 4, 2025 •

edited

Loading