Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add back request telemetry in CLaSP queue #71157

Merged
merged 10 commits into from
Dec 11, 2023

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Dec 7, 2023

Modify CLaSP's QueueItem to explicitly send details about timing and success. This better mimics the previous RequestMetrics usage that was removed when Ryan moved clasp over.

Also, added AbstractLspLogger to help in the (eventual) transition away from the ILspLogger interface.

Move some work from AbstractRequestScope to RequestTelemetryScope to allow for usage of SharedStopwatch
Remove RequestMetrics class
@ToddGrun ToddGrun marked this pull request as ready for review December 8, 2023 17:22
@ToddGrun ToddGrun requested a review from a team as a code owner December 8, 2023 17:22
Comment on lines +65 to +67
var telemetryService = lspServices.GetRequiredServices<AbstractTelemetryService>().FirstOrDefault();

_requestTelemetryScope = telemetryService?.CreateRequestScope(methodName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetRequiredServices says to me that the service is expected to always exist, but the FirstOrDefault and null check operator says to me that it isn't. Which one is it?

(and i'm hoping that the GetRequiredServices bit is wrong, or this will presuambly stop Razor and Web Tools from working once this is inserted)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetRequiredServices is poorly named (the whole ILspServices implementation is a bit wonky). It doesn't throw if there aren't any items, but rather returns an empty enumeration.

I went ahead and used this as it gave me what I need and I didn't want to change the interface, and really didn't want to change the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, happy to hear its not actually required 😁

Change RequestTelemetryLogger to use TelemetryLogging apis
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Dec 9, 2023

Closing a solution in VS (with C# LSP enabled) after a couple edits to a file led to the following telemetry events:

image

where a requestcounter entry looks like

image

and a requestduration entry looks like

image

and a timeinqueue entry looks like

image

@davidwengier
Copy link
Contributor

Should remember to add language as another property once Marco's PR is in.

@ToddGrun ToddGrun merged commit 6e5a343 into dotnet:main Dec 11, 2023
21 of 24 checks passed
@ghost ghost added this to the Next milestone Dec 11, 2023
@Cosifne Cosifne removed this from the Next milestone Jan 9, 2024
@Cosifne Cosifne added this to the 17.9 P3 milestone Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants