made NFW to bucket things better (#22865)

* made NFW to bucket things better

NFW API allows caller to supply extra info to make bucketting better.

this takes advantage of that ability for certain exception types which we know inner or remote exception is more important than the generic outter exception.

* added more comments

src/VisualStudio/CSharp/Test/Watson/WatsonTests.cs

+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Generic;
+using Microsoft.CodeAnalysis.ErrorReporting;
+using Microsoft.VisualStudio.Telemetry;
+using StreamJsonRpc;
+using Xunit;
+
+namespace Roslyn.VisualStudio.CSharp.UnitTests.Watson
+{
+    public class WatsonTests
+    {
+        [Fact]
+        public void TestRegularException()
+        {
+            try
+            {
+                // throw one
+                throw new Exception("test");
+            }
+            catch (Exception exception)
+            {
+                var mockFault = new MockFault();
+                mockFault.SetExtraParameters(exception);
+
+                // there should be no extra bucket info
+                // for regular exception
+                Assert.False(mockFault.Map.ContainsKey(8));
+            }
+        }
+
+        [Fact]
+        public void TestRegularWithInnerexception()
+        {
+            try
+            {
+                try
+                {
+                    // throw one
+                    throw new Exception("inner");
+                }
+                catch (Exception inner)
+                {
+                    throw new Exception("outter", inner);
+                }
+            }
+            catch (Exception exception)
+            {
+                var mockFault = new MockFault();
+                mockFault.SetExtraParameters(exception);
+
+                Assert.Equal(exception.InnerException.GetParameterString(), mockFault.Map[8]);
+            }
+        }
+
+        [Fact]
+        public void TestRemoteInvocationException()
+        {
+            var mockFault = new MockFault();
+
+            var exception = new RemoteInvocationException("test", "remoteCallstack", "remoteErrorCode");
+            mockFault.SetExtraParameters(exception);
+
+            Assert.Equal(exception.GetParameterString(), mockFault.Map[8]);
+        }
+
+        [Fact]
+        public void TestRemoteInvocationExceptionNull()
+        {
+            var mockFault = new MockFault();
+
+            var exception = new RemoteInvocationException(message: null, remoteStack: null, remoteCode: null);
+            mockFault.SetExtraParameters(exception);
+
+            Assert.Equal(exception.GetParameterString(), mockFault.Map[8]);
+        }
+
+        [Fact]
+        public void TestAggregateException()
+        {
+            try
+            {
+                // throw one
+                throw new AggregateException("no inner");
+            }
+            catch (Exception exception)
+            {
+                var mockFault = new MockFault();
+                mockFault.SetExtraParameters(exception);
+
+                // there should be no extra bucket info
+                // for regular exception
+                Assert.False(mockFault.Map.ContainsKey(8));
+            }
+        }
+
+        [Fact]
+        public void TestAggregateWithInnerexception()
+        {
+            try
+            {
+                try
+                {
+                    // throw one
+                    throw new Exception("inner");
+                }
+                catch (Exception inner)
+                {
+                    throw new AggregateException(inner);
+                }
+            }
+            catch (Exception exception)
+            {
+                var mockFault = new MockFault();
+                mockFault.SetExtraParameters(exception);
+
+                Assert.Equal(exception.GetParameterString(), mockFault.Map[8]);
+            }
+        }
+
+        [Fact]
+        public void TestAggregateWithMultipleInnerexceptions()
+        {
+            try
+            {
+                List<Exception> inners = new List<Exception>();
+
+                try
+                {
+                    // throw one
+                    throw new Exception("inner1");
+                }
+                catch (Exception inner)
+                {
+                    inners.Add(inner);
+                }
+
+                try
+                {
+                    // throw one
+                    throw new Exception("inner2");
+                }
+                catch (Exception inner)
+                {
+                    inners.Add(inner);
+                }
+
+                throw new AggregateException(inners);
+            }
+            catch (AggregateException exception)
+            {
+                var mockFault = new MockFault();
+                mockFault.SetExtraParameters(exception);
+
+                var flatten = exception.Flatten();
+                Assert.Equal(flatten.CalculateHash(), mockFault.Map[7]);
+                Assert.Equal(flatten.InnerException.GetParameterString(), mockFault.Map[8]);
+            }
+        }
+
+        public class MockFault : IFaultUtility
+        {
+            public readonly Dictionary<int, string> Map = new Dictionary<int, string>();
+
+            public void SetBucketParameter(int bucketNumber, string value)
+            {
+                Map.Add(bucketNumber, value);
+            }
+
+            #region not used
+            public void AddErrorInformation(string information)
+            {
+                throw new NotImplementedException();
+            }
+
+            public void AddFile(string fullpathname)
+            {
+                throw new NotImplementedException();
+            }
+
+            public void AddProcessDump(int pid)
+            {
+                throw new NotImplementedException();
+            }
+
+            public string GetBucketParameter(int bucketNumber)
+            {
+                throw new NotImplementedException();
+            }
+            #endregion
+        }
+    }
+}

src/VisualStudio/Core/Def/Implementation/Watson/WatsonExtensions.cs

+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System;
+using Microsoft.VisualStudio.Telemetry;
+using Roslyn.Utilities;
+using StreamJsonRpc;
+
+namespace Microsoft.CodeAnalysis.ErrorReporting
+{
+    internal static class WatsonExtensions
+    {
+        // NFW API let caller to customize watson report to make them better bucketed by
+        // putting custom string in reserved slots. normally those 2 slots will be empty.
+        private const int Reserved1 = 8;
+        private const int Reserved2 = 7;
+
+        /// <summary>
+        /// This sets extra watson bucket parameters to make bucketting better
+        /// in non fatal watson report
+        /// </summary>
+        public static void SetExtraParameters(this IFaultUtility fault, Exception exception)
+        {
+            switch (exception)
+            {
+                case RemoteInvocationException remote:
+                    fault.SetBucketParameter(Reserved1, remote.GetParameterString());
+                    return;
+                case AggregateException aggregate:
+                    if (aggregate.InnerException == null)
+                    {
+                        return;
+                    }
+                    else if (aggregate.InnerExceptions.Count == 1)
+                    {
+                        fault.SetBucketParameter(Reserved1, aggregate.GetParameterString());
+                        return;
+                    }
+                    else
+                    {
+                        var flatten = aggregate.Flatten();
+
+                        fault.SetBucketParameter(Reserved1, flatten.InnerException.GetParameterString());
+                        fault.SetBucketParameter(Reserved2, flatten.CalculateHash());
+
+                        return;
+                    }
+                default:
+                    if (exception.InnerException == null)
+                    {
+                        return;
+                    }
+
+                    fault.SetBucketParameter(Reserved1, exception.InnerException.GetParameterString());
+                    return;
+            }
+        }
+
+        public static string CalculateHash(this AggregateException exception)
+        {
+            var hash = 1;
+            foreach (var inner in exception.InnerExceptions)
+            {
+                var parameterString = inner.GetParameterString();
+                hash = Hash.Combine(parameterString, hash);
+            }
+
+            return hash.ToString();
+        }
+
+        public static string GetParameterString(this Exception exception)
+        {
+            switch (exception)
+            {
+                case RemoteInvocationException remote:
+                    return $"{remote.RemoteErrorCode} {remote.RemoteStackTrace ?? exception.Message}";
+                case AggregateException aggregate when aggregate.InnerException != null:
+                    // get first exception that is not aggregated exception
+                    return GetParameterString(aggregate.InnerException);
+                default:
+                    return $"{exception.GetType().ToString()} {(exception.StackTrace ?? exception.ToString())}";
+            }
+        }
+    }
+}

src/VisualStudio/Core/Def/Implementation/Watson/WatsonReporter.cs

@@ -81,7 +81,7 @@ public static void Report(string description, Exception exception, Func<IFaultUt
             // bail out.
             // I think this should be actually done by PostFault itself and I talked to them about it.
             // but until they do something, we will do very simple throuttle ourselves.
-            var currentExceptionString = $"{exception.GetType().ToString()} {(exception.StackTrace ?? exception.ToString())}";
+            var currentExceptionString = exception.GetParameterString();
             var currentException = currentExceptionString.GetHashCode();
             if (s_lastExceptionReported == currentException)
             {
@@ -102,6 +102,10 @@ public static void Report(string description, Exception exception, Func<IFaultUt
                 {
                     // always add current processes dump
                     arg.AddProcessDump(System.Diagnostics.Process.GetCurrentProcess().Id);
+
+                    // add extra bucket parameters to bucket better in NFW
+                    arg.SetExtraParameters(exception);
+
                     return callback(arg);
                 });
 

src/Workspaces/Remote/ServiceHub/ServiceHub.csproj

@@ -38,6 +38,7 @@
     <PublicAPI Include="PublicAPI.Unshipped.txt" />
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="..\..\..\VisualStudio\Core\Def\Implementation\Watson\WatsonExtensions.cs" Link="Telemetry\WatsonExtensions.cs" />
     <Compile Include="..\..\..\VisualStudio\Core\Def\Telemetry\VSTelemetryCache.cs">
       <Link>Telemetry\VSTelemetryCache.cs</Link>
     </Compile>

src/Workspaces/Remote/ServiceHub/Telemetry/WatsonReporter.cs

@@ -74,6 +74,10 @@ public static void Report(string description, Exception exception, Func<IFaultUt
                 {
                     // always add current processes dump
                     arg.AddProcessDump(System.Diagnostics.Process.GetCurrentProcess().Id);
+
+                    // add extra bucket parameters to bucket better in NFW
+                    arg.SetExtraParameters(exception);
+
                     return callback(arg);
                 });
         }