Skip to content

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
Signed-off-by: Vlad Rudenko <vladislav.rudenko@gmail.com>
@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
onionhammer pushed a commit to onionhammer/dotnet-sdk that referenced this pull request Aug 22, 2023
…#1014)

Signed-off-by: Vlad Rudenko <vladislav.rudenko@gmail.com>
Signed-off-by: Erik O'Leary <erik.m.oleary@gmail.com>
divzi-p pushed a commit to divzi-p/dotnet-sdk that referenced this pull request Dec 10, 2024
…#1014)

Signed-off-by: Vlad Rudenko <vladislav.rudenko@gmail.com>
Signed-off-by: Divya Perumal <diperuma@microsoft.com>
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