.Net: [Error handling] [Part7] Removing IsCriticalException extension method by SergeyMenshykh · Pull Request #2516 · microsoft/semantic-kernel

SergeyMenshykh · 2023-08-21T17:37:40Z

Motivation and Context

This PR is one of the follow-up PRs aimed at gradually replacing SK custom exceptions with SKException to provide a simple, consistent, and extensible exception model. You can find more details in the ADR discussing error handling.

Description

This PR removes the IsCriticalException extension method and updates all the code that used it. In a few cases, the change triggers the CA1031 - Do not catch general exception types warning, which is temporarily suppressed to keep this PR focused on removing the IsCriticalException extension method. The proper fix for these cases will be applied by one of the following PRs dedicated to the fix.

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

…r handling code.

lemillermicrosoft

Why are we removing IsCriticalException? The ADR only mentions it in that it will be updated to include StackOverflowException and OutOfMemoryException.

dmytrostruk

As mentioned by @lemillermicrosoft , would be nice to see updates in ADR with details why IsCriticalException is removed/replaced.

SergeyMenshykh · 2023-08-22T09:18:59Z

Why are we removing IsCriticalException? The ADR only mentions it in that it will be updated to include StackOverflowException and OutOfMemoryException.

Yes, right, it's not part of ADR. It popped up later as a comment in one of the 'Erro handling' PRs where we (@rogerbarreto , @stephentoub and me) agreed that there's no value in having it.

Link - https://github.com/microsoft/semantic-kernel/pull/2263/files/02abc1c868a259e081bb144f869cf09fcecd8416#diff-160896cef4bb4f4f3256d65cadf09f8d0e223b06a1b1c1184647780e8d319a37

Let's discuss this on parking lot. CC: @shawncal

rogerbarreto

By removing the function IsCriticalException we are pratically considering any exceptions to be "critical" (should throw), I would expect the change to be either:

Remove all the catch block
Keep the catch just to log and throw after

rogerbarreto · 2023-08-22T15:12:13Z

            return completionResponse.ConvertAll(c => new TextCompletionStreamingResult(c));
        }
-        catch (Exception e) when (e is not AIException && !e.IsCriticalException())
+        catch (Exception e) when (e is not AIException)


MSIL for catch + when is way more costly than using the below. Apply the same for the others.

Suggested change

catch (Exception e) when (e is not AIException)

catch (AIException aiex)

{

throw;

}

catch (Exception ex)

rogerbarreto · 2023-08-22T15:15:39Z

+                    catch (Exception ex)
                    {
                        nextStep.Observation = $"Error invoking action {nextStep.Action} : {ex.Message}";
                        this._logger?.LogWarning(ex, "Error invoking action {Action}", nextStep.Action);


The exception is being swallowed and no log data is being stored about it.

LogWarning should be LogError or at least include the exception details.

I will tag @lemillermicrosoft for input here.

The exception is being swallowed

As far as I know, this is expected behavior for StepwisePlanner, when it receives an exception, it will put details in Observation field and proceed to the next step.

LogWarning should be LogError

In case this is correct behavior, we shouldn't log it as error, because it doesn't break the application flow, instead it's just transient error. Previously we used Debug level here, and I updated it to Warning, and it's still not clear whether it was right choice.

or at least include the exception details.

As far as I can see, we include ex instance as first parameter to LogWarning method.

Yes thank you @dmytrostruk . I think LogDebug is probably best in this scenario (I was gonna change in another PR).

rogerbarreto · 2023-08-22T15:18:35Z

+        catch (Exception e)
        {
            this._logger?.LogError(e, "Something went wrong in system step: {0}.{1}. Error: {2}", targetFunction.SkillName, targetFunction.Name, e.Message);
            return $"Something went wrong in system step: {targetFunction.SkillName}.{targetFunction.Name}. Error: {e.Message} {e.InnerException.Message}";


Suggested change

return $"Something went wrong in system step: {targetFunction.SkillName}.{targetFunction.Name}. Error: {e.Message} {e.InnerException.Message}";

return $"Something went wrong in system step: {targetFunction.SkillName}.{targetFunction.Name}. Error: {e.Message} {e.InnerException?.Message}";

rogerbarreto · 2023-08-22T15:21:42Z

            return await this._function(null, settings, context, cancellationToken).ConfigureAwait(false);
        }
-        catch (Exception e) when (!e.IsCriticalException())
+        catch (Exception e)


By removing the function IsCriticalException we are pratically considering any exceptions to be "critical" (should throw).

Two options:

Remove all the catch block

Keep the catch just to log and throw after

rogerbarreto · 2023-08-22T15:21:54Z

                }
            }
-            catch (Exception e) when (!e.IsCriticalException())
+            catch (Exception e)


By removing the function IsCriticalException we are pratically considering any exceptions to be "critical" (should throw).

Two options:

Remove all the catch block

Keep the catch just to log and throw after

rogerbarreto · 2023-08-22T15:24:07Z

-            catch (Exception e) when (!e.IsCriticalException())
+            catch (Exception e)
            {
                this.Logger.LogError(e, "Something went wrong in pipeline step {0}: {1}.{2}. Error: {3}",


Suggested change

this.Logger.LogError(e, "Something went wrong in pipeline step {0}: {1}.{2}. Error: {3}",

this._logger.LogError(e, "Something went wrong in pipeline step {0}: {1}.{2}. Error: {3}",

rogerbarreto · 2023-08-22T15:24:18Z

                        return parser(value, context.Culture);
                    }
-                    catch (Exception e) when (!e.IsCriticalException())
+                    catch (Exception e)


By removing the function IsCriticalException we are pratically considering any exceptions to be "critical" (should throw).

Two options:

Remove all the catch block

Keep the catch just to log and throw after

rogerbarreto · 2023-08-22T15:24:45Z

                        return converter.ConvertFromString(context: null, cultureInfo, input);
                    }
-                    catch (Exception e) when (!e.IsCriticalException() && cultureInfo != CultureInfo.InvariantCulture)
+                    catch (Exception) when (cultureInfo != CultureInfo.InvariantCulture)


By removing the function IsCriticalException we are pratically considering any exceptions to be "critical" (should throw).

Two options:

Remove all the catch block

Keep the catch just to log and throw after

SergeyMenshykh · 2023-08-23T09:56:55Z

Closing the PR due to a lack of consensus regarding the removal of the method. Additionally, it seems that the method can be useful in distinguishing between actionable and non-actionable exceptions.

Removing IsCriticalException the extension method to simplify SK erro…

856887e

…r handling code.

SergeyMenshykh requested a review from a team as a code owner August 21, 2023 17:37

SergeyMenshykh assigned msftgits Aug 21, 2023

shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Aug 21, 2023

SergeyMenshykh requested review from rogerbarreto and stephentoub August 21, 2023 17:37

SergeyMenshykh assigned SergeyMenshykh and unassigned msftgits Aug 21, 2023

SergeyMenshykh added PR: ready for review All feedback addressed, ready for reviews and removed kernel Issues or pull requests impacting the core kernel labels Aug 21, 2023

SergeyMenshykh requested a review from dmytrostruk August 21, 2023 17:42

lemillermicrosoft reviewed Aug 21, 2023

View reviewed changes

Comment thread dotnet/src/Extensions/Planning.StepwisePlanner/StepwisePlanner.cs Outdated

Changed jsutification for CA1031 warning for the stepwise planner.

cc2cd2c

shawncal added the kernel Issues or pull requests impacting the core kernel label Aug 22, 2023

dmytrostruk approved these changes Aug 22, 2023

View reviewed changes

rogerbarreto requested changes Aug 22, 2023

View reviewed changes

rogerbarreto added the PR: breaking change Pull requests that introduce breaking changes label Aug 22, 2023

SergeyMenshykh closed this Aug 23, 2023

	return $"Something went wrong in system step: {targetFunction.SkillName}.{targetFunction.Name}. Error: {e.Message} {e.InnerException.Message}";
	return $"Something went wrong in system step: {targetFunction.SkillName}.{targetFunction.Name}. Error: {e.Message} {e.InnerException?.Message}";

	this.Logger.LogError(e, "Something went wrong in pipeline step {0}: {1}.{2}. Error: {3}",
	this._logger.LogError(e, "Something went wrong in pipeline step {0}: {1}.{2}. Error: {3}",

Uh oh!

Conversation

SergeyMenshykh commented Aug 21, 2023

Motivation and Context

Description

Contribution Checklist

Uh oh!

lemillermicrosoft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmytrostruk left a comment

Choose a reason for hiding this comment

Uh oh!

SergeyMenshykh commented Aug 22, 2023

Uh oh!

rogerbarreto left a comment

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!

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!

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!

SergeyMenshykh commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants