Skip to content

Conversation

@maryamariyan
Copy link
Contributor

Adds telemetry for LSP calls implementing AbstractRazorDelegatingEndpoint

@maryamariyan maryamariyan requested a review from a team as a code owner June 26, 2023 17:28
TResponse? delegatedRequest;
try
{
using var _ = _telemetryReporter?.TrackLspRequest(nameof(HandleRequestAsync), LspName, positionInfo.LanguageKind == RazorLanguageKind.CSharp ? "Razor C# Language Server Client" : "HtmlDelegationLanguageServerClient", correlationId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO, move strings "Razor C# Language Server Client" and "HtmlDelegationLanguageServerClient" elsewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

They already are:

public const string HtmlLanguageServerName = "HtmlDelegationLanguageServerClient";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RazorLSPConstants seemed inaccessible here. So I figured I had to create a new class

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, because it's LanguageServerClient. I wonder if we should move that?

Copy link
Member

Choose a reason for hiding this comment

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

I think this call is just in the wrong place. The thing that receives this request is not the delegated servers, but our code in the LSP client. This call should love to DefauktRazorLanguageServerCustomMessageTarget, where the calls to the delegated servers happen, and then those constants will be available (and probably unnecessary anyway)

TResponse? delegatedRequest;
try
{
using var _ = _telemetryReporter?.TrackLspRequest(nameof(HandleRequestAsync), LspName, positionInfo.LanguageKind == RazorLanguageKind.CSharp ? "Razor C# Language Server Client" : "HtmlDelegationLanguageServerClient", correlationId);
Copy link
Contributor

Choose a reason for hiding this comment

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

They already are:

public const string HtmlLanguageServerName = "HtmlDelegationLanguageServerClient";

}

var correlationId = Guid.NewGuid();
using var __ = _telemetryReporter?.TrackLspRequest(LspTarget, LanguageServerConstants.RazorLanguageServerName, correlationId);
Copy link
Member

Choose a reason for hiding this comment

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

I thought we wanted this to be opt-in, or we'll be reporting too much telemetry?

Also, should the call be moved to under the guard clauses? Seems like we'd be reporting telemetry for requests we didn't do anything with, which would skew our data.

@maryamariyan
Copy link
Contributor Author

maryamariyan commented Jun 27, 2023

I created a standalone PR for adding telemetry for CodeAction here: #8873

I would rather have that merged before this one. FYI, after #8873 gets merged, there will be a slight merge conflict here on the TrackLspRequest signature because of the shared logic.

@maryamariyan maryamariyan marked this pull request as draft June 28, 2023 00:25
@maryamariyan maryamariyan deleted the sub-lsp-others branch April 1, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants