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

Move AsyncBatchingWorkQueue usage in telemetry to TelemetryLogging level #73485

Merged
merged 3 commits into from
May 17, 2024

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented May 15, 2024

Reapply "Move AsyncBatchingWorkQueue usage in telemetry to TelemetryLogging"

This reverts commit 4bbcd1d (which was a reversion of 21181a7).
Original PR which introduced most of these changes: #73287

Additionally:

  1. Switch from AsyncBatchingWorkQueue to just a simple task based fire and forget mechanism. The first PR changed introduced the problem that we added work to the ABWQ immediately when the prior work completed. Thus the IAsynchronousOperationListener system would never see the queue as empty. As adding the work immediately avoided other issues, I wanted to keep that concept in this fixup. So, I instead moved away from ABWQ and just do a simple fire and forget to queue up the next collection.
  2. Cleanup _findDocumentResults and _usedForkedSolutionCounter after their data has been flushed.

ToddGrun added 2 commits May 14, 2024 18:47
…e and forget mechanism

2) Cleanup _findDocumentResults and _usedForkedSolutionCounter after their data has been flushed.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 15, 2024
@ToddGrun ToddGrun changed the title Dev/toddgrun/reapply telemetry changes Move AsyncBatchingWorkQueue usage in telemetry to TelemetryLogging level May 15, 2024
@ToddGrun ToddGrun marked this pull request as ready for review May 15, 2024 13:12
@ToddGrun ToddGrun requested a review from a team as a code owner May 15, 2024 13:12
@ToddGrun ToddGrun requested a review from dibarbet May 15, 2024 13:13
@ToddGrun
Copy link
Contributor Author

ToddGrun commented May 15, 2024

Test insertion (as I broke insertions with the previous iteration of this) didn't hit the same issues as the previous PR was causing.

https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/551162

@CyrusNajmabadi
Copy link
Member

What was different this time around,(will review when I get in the office)

@ToddGrun
Copy link
Contributor Author

What was different this time around,(will review when I get in the office)

The first commit is strictly the revert. The second commit is the changes this time around. The issue was as you had originally thought, we were pushing an item into the ABWQ immediately after finishing the prior one (the code used to wait until a telemetry event was fired, but that didn't fit well with how things moved around). By pushing immediately into the ABWQ upon processing an item from it, the IAsynchronousOperationListener processing code would never complete.

// Create a fire and forget task to handle the next collection. This doesn't use IAsynchronousOperationListener
// to track this work as no-one needs to ensure this is sent, and the create a new item of work
// upon previous completion doesn't fit well in that model.
_ = PostCollectedTelemetryAsync(CancellationToken.None);
Copy link
Member

Choose a reason for hiding this comment

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

consider wrapping with reportnonfatal (so we'll have non fatals if something goes wrong)

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