Log certain exceptions so we don't lose their stacktraces (Azure#23139)

GetClientException is used to wrap the input exceptions in certain cases. In those cases where it doesn't and the caller throws the exception, the stacktrace gets truncated at the throw-site. Therefore, we will log the input exception so that there is a record of it.
AME-Redmond · Sep 8, 2021 · ce07ea5 · ce07ea5
1 parent ab37d39
commit ce07ea5
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 22 deletions.
diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpExceptionHelper.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpExceptionHelper.cs
@@ -175,7 +175,10 @@ static Exception ToMessagingContractException(string condition, string message,
             return new ServiceBusException(true, message);
         }
 
-        public static Exception GetClientException(Exception exception, string referenceId = null, Exception innerException = null, bool connectionError = false)
+        // In some cases, we don't store the input exception as an inner exception of the returned exception.  This means we will lose the input exception's stacktrace.  One fix is to always store it.
+        // However, this changes the type of exception we throw, and could break clients who were depending on the exception type.  Instead, we will log the input exception's stacktrace.
+        // This behavior will be controlled by the flag 'logExceptionIfNotWrapped', since some callers already log the input exception.
+        public static Exception GetClientException(Exception exception, bool logExceptionIfNotWrapped, string referenceId = null, Exception innerException = null, bool connectionError = false)
         {
             var stringBuilder = new StringBuilder();
             stringBuilder.AppendFormat(CultureInfo.InvariantCulture, exception.Message);
@@ -227,6 +230,11 @@ public static Exception GetClientException(Exception exception, string reference
                 return new ServiceBusCommunicationException(message, aggregateException);
             }
 
+            if (aggregateException == exception && logExceptionIfNotWrapped)
+            {
+                MessagingEventSource.Log.Error($"{message}: {aggregateException}");
+            }
+
             return aggregateException;
         }
 
@@ -264,7 +272,7 @@ public static Exception GetInnerException(this AmqpObject amqpObject)
                     return null;
             }
 
-            return innerException == null ? null : GetClientException(innerException, null, null, connectionError);
+            return innerException == null ? null : GetClientException(innerException, true, null, null, connectionError);
         }
     }
 }
diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpLinkCreator.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpLinkCreator.cs
@@ -75,7 +75,7 @@ public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
             {
                 MessagingEventSource.Log.AmqpSessionCreationException(this.entityPath, amqpConnection, exception);
                 session?.Abort();
-                throw AmqpExceptionHelper.GetClientException(exception, null, session.GetInnerException(), amqpConnection.IsClosing());
+                throw AmqpExceptionHelper.GetClientException(exception, false, null, session.GetInnerException(), amqpConnection.IsClosing());
             }
 
             AmqpObject link = null;
@@ -95,7 +95,7 @@ public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
                     exception);
 
                 session.SafeClose(exception);
-                throw AmqpExceptionHelper.GetClientException(exception, null, link?.GetInnerException(), amqpConnection.IsClosing());
+                throw AmqpExceptionHelper.GetClientException(exception, false, null, link?.GetInnerException(), amqpConnection.IsClosing());
             }
         }
 

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpSubscriptionClient.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpSubscriptionClient.cs
@@ -129,7 +129,7 @@ public async Task OnAddRuleAsync(RuleDescription description)
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 
@@ -153,7 +153,7 @@ public async Task OnRemoveRuleAsync(string ruleName)
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 
@@ -194,7 +194,7 @@ public async Task<IList<RuleDescription>> OnGetRulesAsync(int top, int skip)
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
     }

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpTransactionEnlistment.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpTransactionEnlistment.cs
@@ -73,7 +73,7 @@ async Task SinglePhaseCommitAsync(SinglePhaseEnlistment singlePhaseEnlistment)
             }
             catch (Exception e)
             {
-                Exception exception = AmqpExceptionHelper.GetClientException(e, null);
+                Exception exception = AmqpExceptionHelper.GetClientException(e, true, null);
                 MessagingEventSource.Log.AmqpTransactionDischargeException(this.transactionId, this.AmqpTransactionId, exception);
                 singlePhaseEnlistment.InDoubt(exception);
             }
@@ -98,7 +98,7 @@ async Task RollbackAsync(SinglePhaseEnlistment singlePhaseEnlistment)
             }
             catch (Exception e)
             {
-                Exception exception = AmqpExceptionHelper.GetClientException(e, null);
+                Exception exception = AmqpExceptionHelper.GetClientException(e, true, null);
                 MessagingEventSource.Log.AmqpTransactionDischargeException(this.transactionId, this.AmqpTransactionId, exception);
                 singlePhaseEnlistment.Aborted(exception);
             }

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Core/MessageReceiver.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Core/MessageReceiver.cs
@@ -1124,7 +1124,7 @@ protected virtual async Task<IList<Message>> OnReceiveAsync(int maxMessageCount,
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception, receiveLink?.GetTrackingId(), null, receiveLink?.IsClosing() ?? false);
+                throw AmqpExceptionHelper.GetClientException(exception, true, receiveLink?.GetTrackingId(), null, receiveLink?.IsClosing() ?? false);
             }
         }
 
@@ -1183,7 +1183,7 @@ protected virtual async Task<IList<Message>> OnPeekAsync(long fromSequenceNumber
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 
@@ -1231,7 +1231,7 @@ protected virtual async Task<IList<Message>> OnReceiveDeferredMessageAsync(long[
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
 
             return messages;
@@ -1316,7 +1316,7 @@ protected virtual async Task<DateTime> OnRenewLockAsync(string lockToken)
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
 
             return lockedUntilUtc;
@@ -1489,7 +1489,7 @@ async Task DisposeMessagesAsync(IEnumerable<Guid> lockTokens, Outcome outcome)
                     throw new MessageLockLostException(Resources.MessageLockLost);
                 }
 
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 
@@ -1552,7 +1552,7 @@ async Task DisposeMessageRequestResponseAsync(Guid[] lockTokens, DispositionStat
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Core/MessageSender.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Core/MessageSender.cs
@@ -573,7 +573,7 @@ async Task OnSendAsync(IList<Message> messageList)
                 }
                 catch (Exception exception)
                 {
-                    throw AmqpExceptionHelper.GetClientException(exception, amqpLink?.GetTrackingId(), null, amqpLink?.Session.IsClosing() ?? false);
+                    throw AmqpExceptionHelper.GetClientException(exception, true, amqpLink?.GetTrackingId(), null, amqpLink?.Session.IsClosing() ?? false);
                 }
             }
         }
@@ -642,7 +642,7 @@ async Task<long> OnScheduleMessageAsync(Message message)
                 }
                 catch (Exception exception)
                 {
-                    throw AmqpExceptionHelper.GetClientException(exception, sendLink?.GetTrackingId(), null, sendLink?.Session.IsClosing() ?? false);
+                    throw AmqpExceptionHelper.GetClientException(exception, true, sendLink?.GetTrackingId(), null, sendLink?.Session.IsClosing() ?? false);
                 }
             }
         }
@@ -675,7 +675,7 @@ async Task OnCancelScheduledMessageAsync(long sequenceNumber)
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception, sendLink?.GetTrackingId(), null, sendLink?.Session.IsClosing() ?? false);
+                throw AmqpExceptionHelper.GetClientException(exception, true, sendLink?.GetTrackingId(), null, sendLink?.Session.IsClosing() ?? false);
             }
         }
 

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/MessageSession.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/MessageSession.cs
@@ -105,7 +105,7 @@ protected async Task<byte[]> OnGetStateAsync()
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 
@@ -140,7 +140,7 @@ protected async Task OnSetStateAsync(byte[] sessionState)
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 
@@ -170,7 +170,7 @@ protected async Task OnRenewSessionLockAsync()
             }
             catch (Exception exception)
             {
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, true);
             }
         }
 

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/MessagingEventSource.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/MessagingEventSource.cs
@@ -1416,6 +1416,15 @@ public void UnregisterSessionHandlerStop(string clientId)
                 this.WriteEvent(121, clientId);
             }
         }
+
+        [Event(122, Level = EventLevel.Error, Message = "Error occurred: {0}")]
+        public void Error(string errorMsg)
+        {
+            if (this.IsEnabled())
+            {
+                this.WriteEvent(122, errorMsg);
+            }
+        }
     }
 
     internal static class TraceHelper

diff --git a/sdk/servicebus/Microsoft.Azure.ServiceBus/src/SessionClient.cs b/sdk/servicebus/Microsoft.Azure.ServiceBus/src/SessionClient.cs
@@ -331,7 +331,7 @@ public async Task<IMessageSession> AcceptMessageSessionAsync(string sessionId, T
                     exception);
 
                 await session.CloseAsync().ConfigureAwait(false);
-                throw AmqpExceptionHelper.GetClientException(exception);
+                throw AmqpExceptionHelper.GetClientException(exception, false);
             }
             finally
             {