Skip to content

Conversation

@maryamariyan
Copy link
Contributor

@maryamariyan maryamariyan commented Jun 1, 2023

Summary of the changes

  • Includes a correlation ID, a Guid we generate for the sub LSP call to help correlate between the calls that are for the same main Diagnostics LSP request (As with diagnostics we may send sub LSP call for both html and C# LSP server and this way we can associate them together using this id).

/cc @ryzngard @davidwengier

Related to #8539

@maryamariyan maryamariyan requested a review from a team as a code owner June 1, 2023 18:56
@maryamariyan maryamariyan force-pushed the activityid branch 2 times, most recently from 4db01d8 to 42adc21 Compare June 1, 2023 19:13
@maryamariyan
Copy link
Contributor Author

maryamariyan commented Jun 2, 2023

I'm looking at adding tests too. But existing feedbacks seem addressed and the functionality seems to be working fine

Note: Ignore the actual ms timing because this was done in debug mode and hence not the best representation, but the ratios are probably more meaningful.

- TelemetryReporter.TelemetryScope.Dispose(): Took 165ms with correlationId {a993e40f-d6a3-4854-a0a7-fcca28c518c8} with "HtmlDelegationLanguageServerClient" during "textdocument/_vs_diagnostic"
- TelemetryReporter.TelemetryScope.Dispose(): Took 1585ms with correlationId {a993e40f-d6a3-4854-a0a7-fcca28c518c8} with "Razor C# Language Server Client" during "textdocument/_vs_diagnostic"
- TelemetryReporter.TelemetryScope.Dispose(): Took 2076ms with correlationId {a993e40f-d6a3-4854-a0a7-fcca28c518c8} with "Razor Language Server Client" during "textdocument/_vs_diagnostic"

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for considering all of the feedback.

@maryamariyan
Copy link
Contributor Author

Filed #8801 to track the need for integration testing to verify the telemetry events are fired when expected.

I tested manually and they work as expected, merging.

@maryamariyan maryamariyan merged commit 485cd41 into main Jun 2, 2023
@maryamariyan maryamariyan deleted the activityid branch June 2, 2023 20:55
maryamariyan added a commit that referenced this pull request Jun 2, 2023
Use CorrelationId to track Sub-LSP call duration for Diagnostics
@maryamariyan maryamariyan mentioned this pull request Jun 2, 2023
maryamariyan added a commit that referenced this pull request Jun 5, 2023
@maryamariyan maryamariyan self-assigned this Jun 28, 2023
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.

5 participants