Won't we start reporting ConnectionLostExceptions (that are wrapped in OperationCanceledException) if this is merged? These are expected exceptions that occur on shutdown and they may get wrapped in OperationCanceledException and get reported if cancellation token is not signaled when ReportAndPropagateUnlessCanceled is called with cancellation token.

This is assuming all of the cancellation exceptions getting reported are wrapping a ConnectionLostException. Also, it'd be fine to skip reporting for ConnectionLostException when the current process is the ServiceHub process (ConnectionLostException is a more targeted exclusion than OperationCanceledException).

The motivation for adding OperationCanceledNotMatchingCancellationTokenException that is wrapping ConnectionLostException or ObjectDisposedException in #61792 is to distinguish OCEs that should not be reported in NFEs from regular OCEs.

Are there any other cases when we are wrapping exceptions to OCE?

The motivation for adding OperationCanceledNotMatchingCancellationTokenException that is wrapping ConnectionLostException or ObjectDisposedException in #61792 is to distinguish OCEs that should not be reported in NFEs from regular OCEs.

Are there any other cases when we are wrapping exceptions to OCE?

If we want to avoid reporting specific exceptions as faults we should filter there instead of FatalError. Filtering at that level would also result in it being removed from ETW traces and other logs right?

The goal of this is specifically around fault reporting. OCE is resulting in millions of events of unactionable data and bucketed in ways we can't root cause. This will, at the very least, give us a clearer picture of root causing.

OCE is resulting in millions of events of unactionable data

Yes, some of these (maybe most?) because we are wrapping ConnectionLostException in OCE today and then reporting it form FatalError.ReportUnlessCancelled(Exception, CancellationToken) because the cancellation token is not signaled in this case.

If we just unwrap this OCE and do nothing else we will continue having these unactionable NFEs, just with ConnectionLostException instead of OCE

Once #61792 is merged I think we shouldn't be seeing any OCE wrapping another exception being reported via NFE.

Once #61792 is merged I think we shouldn't be seeing any OCE wrapping another exception being reported via NFE.

I think we should still take this change though. Having the inner exception unwrapped for OCE is more useful than ever reporting OCE for faults

Maybe, but there are no other cases where we wrap an exception to OCE in Roslyn.

Maybe, but there are no other cases where we wrap an exception to OCE in Roslyn.

yet :)

/azp run