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

Inroduce OnActorMethodFailedAsync virtual method to simplify error logging #1014

Merged
merged 6 commits into from
Aug 16, 2023

Conversation

vlardn
Copy link
Contributor

@vlardn vlardn commented Jan 24, 2023

Description

Currently, to catch exceptions thrown by any actor method (and log them somewhere) you need to add the try/catch block to ALL actor methods. For timer and reminder actor callbacks, this is almost mandatory, otherwise, exceptions will never appear anywhere. But all these try/catch blocks bloat the actors' code and make it less readable.

Given change introduces the OnActorMethodFailedAsync virtual method which can be overridden by the user the same way as the OnPreActorMethodAsync or OnPostActorMethodAsync methods and can be used (in most cases) to log exceptions thrown by any actor method as well as by OnPreActorMethodAsync or OnPostActorMethodAsync overridden methods. Overriding of the OnActorMethodFailedAsync method significantly simplifies actor error logging code for the user.

@vlardn vlardn requested review from a team as code owners January 24, 2023 10:15
@vlardn vlardn changed the title Inroduce OnActorMethodFailedAsync virtual method for overriding Inroduce OnActorMethodFailedAsync virtual method to simplify error logging Jan 24, 2023
@vlardn
Copy link
Contributor Author

vlardn commented Jul 7, 2023

Could someone either approve or at least comment on this? Half year is already passed :(
This change is very simple and safe but very important!

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #1014 (5a1eff5) into master (f4e02df) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   67.16%   67.12%   -0.04%     
==========================================
  Files         170      170              
  Lines        5695     5698       +3     
  Branches      607      607              
==========================================
  Hits         3825     3825              
- Misses       1727     1730       +3     
  Partials      143      143              
Flag Coverage Δ
net6 67.05% <0.00%> (-0.04%) ⬇️
net7 67.05% <0.00%> (-0.04%) ⬇️
netcoreapp3.1 67.09% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/Dapr.Actors/Runtime/Actor.cs 59.85% <0.00%> (-1.30%) ⬇️
src/Dapr.Actors/Runtime/ActorManager.cs 52.66% <0.00%> (ø)

@halspang halspang added this to the v1.12 milestone Aug 16, 2023
@halspang
Copy link
Contributor

Thanks for the contribution, sorry for the long delay!

@halspang halspang merged commit 667dcaf into dapr:master Aug 16, 2023
10 of 12 checks passed
onionhammer pushed a commit to onionhammer/dotnet-sdk that referenced this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants