telemetry(lsp): standardize setup metric and types #961
Conversation
Hweinstock
force-pushed
the
hkobew/lspSetupTelemetry
branch
from
bd211fd to
911286f
Compare
| { | ||
| "name": "languageServer_setup", | ||
| "description": "LSP setup event", | ||
| "metadata": [ |
There was a problem hiding this comment.
This is a passive event because users do not take an actual action for this to occur, please indicate the same. In addition fields of duration and result is missing from this shape
There was a problem hiding this comment.
I thought these were global arguments so they would be automatically populated?
Is it still required to add them to the shape as well?Also found in the VS generator I think? https://github.com/aws/aws-toolkit-common/blob/ab466029f241193ab93a8abb3a5ca8dde5f56c94/telemetry/csharp/AwsToolkit.Telemetry.Events/Core/BaseTelemetryEvent.cs#L90C24-L90C32
There was a problem hiding this comment.
Duration is implicit with the C# generator, but we don't have support for result, so this field needs to be added to the metadata.
There was a problem hiding this comment.
but we don't have support for
result, so this field needs to be added to the metadata.
Can that be prioritized? Adding result explicitly to every metric confuses the messaging for partner teams.
Co-authored-by: Shruti Sinha <[email protected]>
Hweinstock
changed the title
| "description": "The location of the manifest" | ||
| }, | ||
| { | ||
| "name": "manifestSchemaVersion", |
There was a problem hiding this comment.
Nice change, this definitely is less ambiguous.
…#6385) ## 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: aws/aws-toolkit-common#961 - bumped commons version to [1.0.296](aws/aws-toolkit-common@8df7a87) 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](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Problem
Relevant PR: aws/aws-toolkit-vscode#6385
Solution
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.