arcus-azure / arcus.messaging

Messaging with Microsoft Azure in a breeze.
https://messaging.arcus-azure.net
MIT License
20 stars 11 forks source link

Not using a Fallbackhandler should be a valid use case and not throw an exception. #424

Closed Roeland54 closed 3 weeks ago

Roeland54 commented 7 months ago

Is your feature request related to a problem? Please describe. In our implementation of arcus messaging we do not want to use a fallback handler if an exception is thrown in the specified handler for the message. We now see 2 exceptions about a missing fallback handler that are covering up the initial exception thrown in the handler. Which is polluting our application insights.

Describe the solution you'd like Not throwing an exception on missing fallback handler if not using a fallback handler is a choice.

Describe alternatives you've considered Maybe make the exception of a missing fallback handler configurable and log a debug / trace message instead.

fgheysels commented 7 months ago

Thanks for reporting this @Roeland54

I think the approach here should indeed be that Arcus logs a Debug trace when no Fallback-messagehandler is registered and should never throw an exception for this.

@stijnmoreels can you have a look at this please ?

stijnmoreels commented 7 months ago

'we do not want to use a fallback handler if an exception is thrown in the specified handler for the message'

This feels a bit off as it would mean that the handler would not be able to handle the message. How is this a scenario that is expected? Even with a trace/debug, you would still pollute the Application Insights - and rightly, so - bc the system is used the wrong way. Yes, the fallback handler is a choice to use, but that places the responsibility purely on the message handlers to handle the message. Not sure I agree if we should not throw in this case as it is a clear indication that the system is not configured correctly.

Roeland54 commented 7 months ago

The idea here is that if a transient error happens in the handler this causes the handler to fail and makes the ServiceBus try to redeliver the message for handling.

fgheysels commented 7 months ago

'we do not want to use a fallback handler if an exception is thrown in the specified handler for the message'

This feels a bit off as it would mean that the handler would not be able to handle the message. How is this a scenario that is expected? Even with a trace/debug, you would still pollute the Application Insights - and rightly, so - bc the system is used the wrong way. Yes, the fallback handler is a choice to use, but that places the responsibility purely on the message handlers to handle the message. Not sure I agree if we should not throw in this case as it is a clear indication that the system is not configured correctly.

If it is a choice for the developer to register a FallbackHandler or not, then it makes no sense that an exception is thrown in case there's no FallbackHandler present. It doesn't mean that the system is not configured correctly, it means that the developer (intentionally) didn't want to use a FallbackHandler. Rather, it can indicate that the system that has developed contains a bug, or that a dependent service that is called in the handler is not functioning correctly. (To cope with this, we'll actually use the circuit-breaker functionality that is under construction).

It makes sense to log this as a debug-level trace however, but the logs should not be 'polluted' with an additional exception where the user is not interested in.

stijnmoreels commented 7 months ago

Will be discussed separately to come to a way of working, as this is in conflict with the core-system and intent of the Messaging system.

fgheysels commented 7 months ago

We discussed this offline.

It seems that the Message Pump tries to handle the message with other suitable messagehandlers if an exception as thrown in a messagehandler. Hence the idea that this change request has a bigger impact. Although this is not an ideal approach imo, I think we can currently stick with this way of working.

However, this issue is merely about the fact that an additional exception is being thrown / logged when none of the registered message-handlers was able to correctly handle the (servicebus)message. This is something that can be improved: instead of throwing an exception and logging that exception as an exception, Arcus should log this as a 'Debug Trace''.

fgheysels commented 4 months ago

Coming back to this, this is what we currently have in our project for the past 5 days, that are a lot of exceptions / traces that just are not necessary

image

This unnecessary logging is something that must be avoided.

fgheysels commented 1 month ago

I would like to re-open this issue, because this is not entirely as it should be. We're now using Arcus.Messaging 2.0.0 and we're still seeing a lot of InvalidOperationException registrations in our logs.

The message that is logged by these exceptions is :

Azure Service Bus message router cannot correctly process the message in the 'AzureServiceBusMessageContext' because none of the registered 'IAzureServiceBusMessageHandler<,>' implementations in the dependency injection container matches the incoming message type and context; and no 'IFallbackMessageHandler' or 'IAzureServiceBusFallbackMessageHandler' was registered to fall back to.Make sure you call the correct '.WithServiceBusMessageHandler' extension on the IServiceCollection during the registration of the message pump or message router to register a message handler

And this originates from the AzureServiceBusMessageRouter that throws this exception:

System.InvalidOperationException:
   at Arcus.Messaging.Abstractions.ServiceBus.MessageHandling.AzureServiceBusMessageRouter.EnsureFallbackMessageHandlerAvailable (Arcus.Messaging.Abstractions.ServiceBus, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null)
   at Arcus.Messaging.Abstractions.ServiceBus.MessageHandling.AzureServiceBusMessageRouter+<TryRouteMessageWithPotentialFallbackAsync>d__13.MoveNext (Arcus.Messaging.Abstractions.ServiceBus, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at Arcus.Messaging.Abstractions.ServiceBus.MessageHandling.AzureServiceBusMessageRouter+<RouteMessageWithPotentialFallbackAsync>d__12.MoveNext (Arcus.Messaging.Abstractions.ServiceBus, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at Arcus.Messaging.Abstractions.ServiceBus.MessageHandling.AzureServiceBusMessageRouter+<RouteMessageAsync>d__11.MoveNext (Arcus.Messaging.Abstractions.ServiceBus, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null)

It would be very nice if this exception could be avoided and if a Debug trace is logged instead. On our DEV environment, we have more than 6.500 of these exceptions logged in the last 24 hours. This clutters our logs and also has an impact on our Azure consumption cost.

stijnmoreels commented 1 month ago

Can you check that you are indeed using Messaging v2.0 everywhere, as you can see from the linked PR, the exception message you're talking about is now only used in a LogDebug statement. The other InvalidOperationException that is being thrown only happens when no message handlers are registered.

Also note: that the fix was made in a general abstraction package, if that one is referenced directly, it might also cause this.

fgheysels commented 1 month ago

We're using Messaging v2.0 everywhere: image

The exception that we're seeing in AppInsights has another message then the one that has now been changed to a LogDebug message :)

Azure Service Bus message router cannot correctly process the message in the 'AzureServiceBusMessageContext' because none of the registered 'IAzureServiceBusMessageHandler<,>' implementations in the dependency injection container matches the incoming message type and context; and no 'IFallbackMessageHandler' or 'IAzureServiceBusFallbackMessageHandler' was registered to fall back to.Make sure you call the correct '.WithServiceBusMessageHandler' extension on the IServiceCollection during the registration of the message pump or message router to register a message handler

The stacktrace also points to the exception that is being thrown by the AzureServiceBusMessageRouter ( here )

System.InvalidOperationException:
   at Arcus.Messaging.Abstractions.ServiceBus.MessageHandling.AzureServiceBusMessageRouter.EnsureFallbackMessageHandlerAvailable (Arcus.Messaging.Abstractions.ServiceBus, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null)
   at Arcus.Messaging.Abstractions.ServiceBus.MessageHandling.AzureServiceBusMessageRouter+<TryRouteMessageWithPotentialFallbackAsync>d__13.MoveNext (Arcus.Messaging.Abstractions.ServiceBus, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null)

The exception is thrown by Arcus.Messaging when the MessageHandler that processes the message throws an unexpected error. (Which can always occur) In that case, we want the message to be returned to the queue, so that it can be reprocessed later. Having this additional exception that is thrown by Arcus is actually not necessary (and unwanted) in this case.

stijnmoreels commented 1 month ago

The link you provided to the line where it throws shows a different exception message then the one you pasted here.

The link you provided points to a place where the message router checks if there are any message handlers available to try - before the message handlers are even run. In that case, it is correct that we throw, as the system was invalidly set up.