Add telemetry for 'bad' experiences based on tag helpers #10906
Conversation
...src/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/DocumentPullDiagnosticsEndpoint.cs
Outdated
Show resolved
Hide resolved
...src/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/DocumentPullDiagnosticsEndpoint.cs
Outdated
Show resolved
Hide resolved
...src/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/DocumentPullDiagnosticsEndpoint.cs
Outdated
Show resolved
Hide resolved
...src/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/DocumentPullDiagnosticsEndpoint.cs
Outdated
Show resolved
Hide resolved
| var document = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false); | ||
| var tagHelpers = document.GetTagHelpers(); | ||
|
|
||
| if (Interlocked.Exchange(ref _lastReportedTagHelperCount, tagHelpers.Count) != tagHelpers.Count) |
There was a problem hiding this comment.
❔ If the user is getting "RZ10012" errors, isn't it possible (maybe likely?) that the tag helper count is going to be 0? If so, is using the tag helper count as a way to reduce telemetry events going to skew the data so it doesn't look as significant a problem? Would using the number of relevant diagnostics be better?
...src/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/DocumentPullDiagnosticsEndpoint.cs
Outdated
Show resolved
Hide resolved
|
(Noting that I'm on my phone so might have missed something, and can't view the work item linked in the description) I'm a little unsure of the scenario this is trying to catch. The last tag helper count has a lifetime that matches the language server, but the number of tag helpers being stored in it comes from the document. That means it's the count of tag helpers available to that document based on usings etc. Seems like it would be trivial to trigger this telemetry just by switching tabs in an IDE. In fact, if the IDE request diagnostics from all open documents, I think a user wouldn't even have to switch. As they are typing a component tag, if they have multiple files open, this could just quietly report telemetry constantly in the background. If the intention is to find issues with tag helper discovery then I think the project tag helper count would be better, though even that won't stand up to multiple documents being open if they're from different projects. We recently saw telemetry that seemed to show project tag helper count fluctuating, but we could tell whether it was just two projects in a solution that had different component counts, which even the standard Blazor App template would exhibit, or an actual problem. Perhaps this telemetry should use a dictionary to keep last tag helper count per project? It might also be a good idea to report a hash of the project. Also worth noting I don't believe the pull diags endpoint will be hit in VS Code so this would be VS only telemetry, though I'm not 100% confident on that. |
|
Definitely agree this should be a per project count. I'll update the PR |
|
Merging before snap but feel free to add follow up comments and I'll address them |
| var document = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false); | ||
| var tagHelpers = document.GetTagHelpers(); | ||
| var tagHelpers = await documentContext.Project.GetTagHelpersAsync(cancellationToken).ConfigureAwait(false); | ||
| var tagHelperCount = tagHelpers.Count(); |
| new Property("RZ10012errors", relevantDiagnosticsCount)); | ||
| new("tagHelpers", tagHelperCount), | ||
| new("RZ10012errors", relevantDiagnosticsCount), | ||
| new("project", documentContext.Project.Key.Id)); |
There was a problem hiding this comment.
Isn't this PII? Do have PIIProperty?
There was a problem hiding this comment.
It gets labeled in the server to not depend on client side labeling and ensure correct handling
Helps add telemetry for cases like AB#2255138