Move the project telemetry collection out of process.#42383
Conversation
|
Tagging @tmat @jasonmalinowski |
| /// along to be worked on. Rounds of processing happen serially, only starting up after a | ||
| /// previous round has completed. | ||
| /// </summary> | ||
| internal class AsyncBatchingWorkQueue<TItem> |
There was a problem hiding this comment.
i extracted this into its own type as it's something i've replicated in a few OOP prs at this point. Might as well have a simple and common impl they can all use.
dibarbet
left a comment
There was a problem hiding this comment.
Had one or two questions, but mainly looks good! Would recommend another pair of eyes though.
...VisualStudio/Core/Def/Implementation/ProjectTelemetry/VisualStudioProjectTelemetryService.cs
Show resolved
Hide resolved
...VisualStudio/Core/Def/Implementation/ProjectTelemetry/VisualStudioProjectTelemetryService.cs
Show resolved
Hide resolved
...tudio/Core/Def/Implementation/ProjectTelemetry/VisualStudioProjectTelemetryServiceFactory.cs
Show resolved
Hide resolved
...VisualStudio/Core/Def/Implementation/ProjectTelemetry/VisualStudioProjectTelemetryService.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectTelemetry/IProjectTelemetryService.cs
Show resolved
Hide resolved
...ces/Remote/ServiceHub/Services/ProjectTelemetry/RemoteProjectTelemetryIncrementalAnalyzer.cs
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncBatchingWorkQueue.cs
Show resolved
Hide resolved
...sualStudio/Core/Def/Implementation/DesignerAttribute/VisualStudioDesignerAttributeService.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.Implementation.ProjectTelemetry | ||
| { | ||
| internal class VisualStudioProjectTelemetryService |
There was a problem hiding this comment.
Are there any semantic changes in this file, or is it just moving existing code around?
There was a problem hiding this comment.
Ah seems like mix of new code + moved code.
There was a problem hiding this comment.
there are semantic changes. this now calls into the OOP system and batches responses from it.
| private const string TelemetryProjectReferencesCountName = PropertyPrefix + "ProjectReferences.Count"; | ||
| private const string TelemetryMetadataReferencesCountName = PropertyPrefix + "MetadataReferences.Count"; | ||
| private const string TelemetryDocumentsCountName = PropertyPrefix + "Documents.Count"; | ||
| private const string TelemetryAdditionalDocumentsCountName = PropertyPrefix + "AdditionalDocuments.Count"; |
There was a problem hiding this comment.
Should we also track AnalyzerConfigDocuments.Count?
There was a problem hiding this comment.
Sure. i can add that. (just preserving existing telemetry code). That won't break anything, right?
There was a problem hiding this comment.
Hmm I am not sure - should be fine to do this in a follow-up PR as you are preserving existing semantics.
...sualStudio/Core/Def/Implementation/DesignerAttribute/VisualStudioDesignerAttributeService.cs
Show resolved
Hide resolved
| } | ||
| catch (Exception e) | ||
| { | ||
| // The telemetry service itself can throw. |
There was a problem hiding this comment.
Interesting, is this the recommended pattern everywhere we log exceptions from telemetry logging?
There was a problem hiding this comment.
no idea. this part was entirely copy paste of the original logic. i didn't want to touch it.
|
|
||
| namespace Microsoft.CodeAnalysis.ProjectTelemetry | ||
| { | ||
| internal struct ProjectTelemetryInfo |
There was a problem hiding this comment.
IEquatable<ProjectTelemetryInfo>?
There was a problem hiding this comment.
No real point. This is just a basic serialization type.
|
|
||
| private async Task StartAsync(CancellationToken cancellationToken) | ||
| { | ||
| _workQueue = new AsyncBatchingWorkQueue<DesignerInfo>( |
There was a problem hiding this comment.
_workQueue [](start = 12, length = 10)
Add Contract.ThrowIfNotNull(_qorkQueue)
| } | ||
| } | ||
|
|
||
| _workQueue.AddWork(attributeInfos); |
There was a problem hiding this comment.
_workQueue [](start = 12, length = 10)
Contract.ThrowIfNull(_workQueue)
| private bool _taskInFlight = false; | ||
|
|
||
| #endregion | ||
| private AsyncBatchingWorkQueue<DesignerInfo> _workQueue = null!; |
There was a problem hiding this comment.
I'd prefer not cheating here and keeping this nullable. Then add Contract.ThrowIfNull in entry points that need this to be initialized.
| /// <summary> | ||
| /// Queue where we enqueue the information we get from OOP to process in batch in the future. | ||
| /// </summary> | ||
| private AsyncBatchingWorkQueue<ProjectTelemetryInfo> _workQueue = null!; |
There was a problem hiding this comment.
ProjectTelemetryInfo [](start = 39, length = 20)
The same comment as in designer attribute impl.
No description provided.