Improve TryCreateContext by martinrrm · Pull Request #4762 · NuGet/NuGet.Client

martinrrm · 2022-08-18T01:26:59Z

Bug

Fixes: NuGet/Home#11918

Regression? Last working version:

Description

Improvements for TryCreateContext, this is the number 3 fault in PRISM

PRISM data https://prism.vsdata.io/failure/?query=ch%3Drelease%20sev!%3DDiagnostic&eventType=fault&failureType=hits&failureHash=17a271a2-70a5-7dee-8a92-c7db5367242c

Related bug: NuGet/Home#5089

PR Checklist

martinrrm · 2022-08-18T20:31:21Z

Conversations in Teams suggested removing throws in this function, since it is a Try___. This will affect other teams; I will start that conversation with them once I have a good improvement in this code path. Please add your suggestions and feedback.

donnie-msft

I thought we were going to change TryCreateContext (Logs then throws) to instead Log and return false (this is how Try____ methods typically work).

Can you let me know if my understanding is accurate, and how that improves the high number of faults?:

Before this PR:
TryCreateContext (Logs then throws) -> CreatePathContextAsync (Logs then throws) -> GetPathContextFromAssetsFileAsync (Throws without logging)

After this PR:
TryCreateContext (Throws without logging) -> CreatePathContextAsync (Throws without logging) -> GetPathContextFromAssetsFileAsync (Logs and swallows exception)

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

donnie-msft · 2022-08-19T03:02:37Z

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

+                    context = GetSolutionPathContext();
                }
            }
-            catch (Exception e) when (e is KeyNotFoundException || e is InvalidOperationException)


Again, this is removing the logging, but still throwing. Can you explain the impact on fault telemetry?

I'm moving the logging to the function where the throw was being raised. I think we can avoid this try catch if we do that, I dont think Telemetry changes

nkolev92 · 2022-08-19T18:54:25Z

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

+            outputPathContext = NuGetUIThreadHelper.JoinableTaskFactory.Run(
+                async () =>
+                {
+                    var nuGetProject = await CreateNuGetProjectAsync(projectUniqueName);


Do we need a generic try/catch here to ensure we never throw?

martinrrm · 2022-08-24T21:10:42Z

@donnie-msft @nkolev92 I reverted changes and I'm only removing the throw and return a null. I'm having trouble trying to improve this code path more, do you have any suggestions? Thanks!

nkolev92 · 2022-08-25T20:50:20Z

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

            {
                _telemetryProvider.PostFault(exception, typeof(VsPathContextProvider).FullName);
-                throw;
+                outputPathContext = null;


We're still posting a fault so we'd still get noticed in the tracking.

I think we should remove the fault post.

@zivkan any thoughts on whether failure results are any useful here?

We should remove the fault reporting only from targeted exception handlers, not the generic catch (Exception e) handler. If we know there is a scenario that any fault telemetry is reported (in theory even 1, but in practise we'll probably only investigate when we have a large count) and we think it's a scenario that the customers (other VS extensions) should handle, not our own code, then we need to find a way to catch it in a non-generic exception handler.

The point is, if we add a regression in the future (for example, side-effect from more refactoring of VsSolutionManager, or CpsNuGetProject), we want to get fault telemetry.

So you're suggesting we try to differentiate between an expected failure and one that isn't expected?

So catch InvalidOperationException or whatever the exception for a project not nominated does?

I think Nikolche suggestion is really good.

So, because a missing assets file is an error the customer should handle, we can remove the fault telemetry and remove the throw. And if the exception is a different than expected we should report the fault telemetry.

So you're suggesting we try to differentiate between an expected failure and one that isn't expected?

Yes, exactly. This is precisely why last year I added the ProjectNotNominatedException. I created this specific exception which is only used in a well-known scenario, so I can tell the difference between this and other unexpected InvalidOperationException.

I know some people aren't a fan of having hundreds of different exception types, but personally I think it's more important (see also this issue where NuGet.Protocol.dll using FatalProtocolException for many, many different reasons, without being able to programatically distinguish between the different scenarios, makes it harder for us to debug a customer issue).

donnie-msft

Looks good!

...nts.Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsPathContextProviderTests.cs

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Exceptions/AssetsFileMissingException.cs

I thought reviews were automatically dismissed with new commits, so I'm surprised to see my stale review here....

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

martinrrm · 2022-09-09T17:01:28Z

@zivkan @nkolev92 @donnie-msft Hi! Can you review the pr again? Thanks!

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

zivkan · 2022-09-09T22:23:57Z

...nts.Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsPathContextProviderTests.cs

-            var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => target.CreatePathContextAsync(project, CancellationToken.None));
-
+            // Assert
+            var exception = await Assert.ThrowsAsync<ProjectNotRestoredException>(() => target.CreatePathContextAsync(project, CancellationToken.None));


Given the XMLDoc says that the exception is InvalidOperationException, I think that the tests should assert the contract, not the implementation, since tests should be validating the customer experience.

Unless the XMLDocs for the methods change, it's not a breaking change from a customer point of view, if a future refactor changes the exception to something other than ProjectNotRestoredException, as long as it's still extending InvalidOperationException.

zivkan · 2022-09-09T22:24:43Z

...nts.Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsPathContextProviderTests.cs

-            var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => target.CreatePathContextAsync(project, CancellationToken.None));
-
+            // Assert
+            var exception = await Assert.ThrowsAsync<ProjectNotRestoredException>(() => target.CreatePathContextAsync(project, CancellationToken.None));


The goal of this PR is to stop reporting telemetry when this known error occurs. Therefore, you should have an assert that the INuGetTelemetryProvider's mock is not called.

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs

ghost · 2022-09-17T07:01:29Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

martinrrm · 2022-09-19T21:16:29Z

I tried adding tests to TryCreateContext but the usage of DTE make it untestable

Insert 6.4.0-rc.123 into rel/d17.4 on 11/07/2022 23:47:12 * tag '6.4.0.123': (60 commits) fix a logic error that caused AbandonedMutexException while executing migrations (release-6.4.x) (NuGet#4895) unblock source build failing due to fatal: transport 'file' not allowed error (NuGet#4867) (NuGet#4874) Signing: update to August 2022 CTL (NuGet#4791) (NuGet#4850) Merged PR 422933: Prefer BCL Directory create API over helper class (7.0.1xx-rc2) Fix empty combobox when package is not present in project file (NuGet#4844) (NuGet#4848) Fix component detection alert for microsoft.owin package (NuGet#4841) (NuGet#4845) Make release label RC, move to escrow mode Adds special case to include transitive origins in GetInstalledAndTransitivePackagesAsync API (NuGet#4824) Add longPathAware manifest to NuGet.Build.Tasks.Console (NuGet#4830) VsPackageInstallerServices should not post ProjectNotNominatedException faults (NuGet#4814) Skip test GetOrCreateAsync_WithUnhandledExceptionInPlugin_Throws (NuGet#4831) Improve OptProf pipeline job run names (NuGet#4825) Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio (NuGet#4798) Suppress CA2213 warnings to unblock dev branch (NuGet#4823) Ensure IsVsOfflineFeed is calculated correctly on 64-bit machines (NuGet#4817) Add better handling of AggregateExceptions in static graph-based restore (NuGet#4809) Add Component Detection task into each pipeline (NuGet#4813) Localizes nuget.exe with default, embedded resource assembly lookup (NuGet#4773) Removes BrowseObjectBase class in NuGet Solution Explorer (NuGet#4807) Improve TryCreateContext (NuGet#4762) ...

martinrrm marked this pull request as ready for review August 18, 2022 20:31

martinrrm requested a review from a team as a code owner August 18, 2022 20:31

donnie-msft previously requested changes Aug 19, 2022

View reviewed changes

martinrrm requested review from donnie-msft and nkolev92 August 19, 2022 17:14

nkolev92 reviewed Aug 19, 2022

View reviewed changes

martinrrm requested a review from nkolev92 August 24, 2022 21:08

nkolev92 reviewed Aug 25, 2022

View reviewed changes

martinrrm force-pushed the dev-martinrrm-try-create-context-improvements branch from 0c8d5c6 to d15887c Compare August 31, 2022 00:30

martinrrm requested review from nkolev92 and zivkan August 31, 2022 00:36

donnie-msft reviewed Aug 31, 2022

View reviewed changes

...nts.Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsPathContextProviderTests.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Exceptions/AssetsFileMissingException.cs Outdated Show resolved Hide resolved

zivkan reviewed Aug 31, 2022

View reviewed changes

martinrrm added 5 commits September 7, 2022 11:23

remove extra throw when the assets file is missing

382e7b5

remove throws

ebc054a

return null insead of throw

cd60884

created new assets file missing exception

fdaff1e

throw invalid operation to user, keep new exception internal

adf63f4

martinrrm force-pushed the dev-martinrrm-try-create-context-improvements branch from d15887c to adf63f4 Compare September 7, 2022 20:13

renamed exception

2abfbf2

martinrrm requested review from donnie-msft and zivkan September 7, 2022 22:19

donnie-msft approved these changes Sep 9, 2022

View reviewed changes

zivkan reviewed Sep 9, 2022

View reviewed changes

zivkan approved these changes Sep 10, 2022

View reviewed changes

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsPathContextProvider.cs Show resolved Hide resolved

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 17, 2022

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 19, 2022

martinrrm merged commit 3827ea2 into dev Sep 19, 2022

martinrrm deleted the dev-martinrrm-try-create-context-improvements branch September 19, 2022 21:17

Conversation

martinrrm commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

martinrrm commented Aug 18, 2022

Uh oh!

donnie-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinrrm commented Aug 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zivkan Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donnie-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martinrrm commented Sep 9, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Sep 17, 2022

Uh oh!

martinrrm commented Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martinrrm commented Aug 18, 2022 •

edited

Loading

zivkan Aug 25, 2022 •

edited

Loading