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

[service-bus] Design discussion for error reporting #12286

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions _discussion_reason_codes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# Service Bus: reason codes or equivalent

## Proposal

Each language should provide:

- [ ] Aconceptual equivalent to a reason code (a programatically readable field describing the reason for the failure)
- [ ] A sample that shows how to use it.

## Current state

This is what we have:

| Language | Error class name | Has 'reason' | Samples link |
| ---------- | ----------------------------------------- | ----------------------------------------- | -------------------------------------------------------- |
| JavaScript | [ServiceBusError][js_exception] | yes | [receiveMessagesStreaming.ts][js_sample] |
| C# | [ServiceBusException][csharp_exception] | yes | [ServiceBusProcessor(maybe, but not yet)][csharp_sample] |
| Python | [ServiceBusError(base)][python_exception] | yes, via the type of the exception itself | |
| Java | [ServiceBusException][java_exception] | no | |

## Additional questions

- [ ] Are users expected to react to these errors? If so, what other flags/information might we need?
Copy link
Member

@jsquire jsquire Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attributes that I see as important are:

  • IsTransient : Offers a hint to users if retrying may resolve the issue; also offers a hint to understand if the client has already retried according to its policy.

  • Message: What in the wide, wide world of sports is a-goin' on around here? (This is driven by the service response, when triggered as part of communication)

One that I've heard asked for and discussed that I think we should avoid:

  • WhoseFault: An indication of "this was caused by something the user did" versus "this is just something that happened". While it is potentially helpful, the service does not give a clear indication of this and I do not believe the client should be attempting to interpret and classify the service response this way. If the service begins to return this, then yeah.... we should surface it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the isTransient, how would we know if it is a transient issue if we pass on the error as soon as it is encountered?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do not retry, it's not transient. If you do.... 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have the retryable flag for that! :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retryable = IsTransient

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you mean Retriable? 😉

- [ ] Is Java going with a base-class so users distinguish via types (like Python) or with an enum?
- [ ] Here's the current list of Reasons and their descriptions. Do we like these? Can we sync names in our code?

```csharp
public enum ServiceBusFailureReason
{
/// <summary>
/// The exception was the result of a general error within the client library.
/// </summary>
GeneralError,

/// <summary>
/// A Service Bus resource cannot be found by the Service Bus service.
/// </summary>
MessagingEntityNotFound,

/// <summary>
/// The lock on the message is lost. Callers should call attempt to receive and process the message again.
/// </summary>
MessageLockLost,

/// <summary>
/// The requested message was not found.
/// </summary>
MessageNotFound,

/// <summary>
/// A message is larger than the maximum size allowed for its transport.
/// </summary>
MessageSizeExceeded,

/// <summary>
/// The Messaging Entity is disabled. Enable the entity again using Portal.
/// </summary>
MessagingEntityDisabled,

/// <summary>
/// The quota applied to an Service Bus resource has been exceeded while interacting with the Azure Service Bus service.
/// </summary>
QuotaExceeded,

/// <summary>
/// The Azure Service Bus service reports that it is busy in response to a client request to perform an operation.
/// </summary>
ServiceBusy,

/// <summary>
/// An operation or other request timed out while interacting with the Azure Service Bus service.
/// </summary>
ServiceTimeout,

/// <summary>
/// There was a general communications error encountered when interacting with the Azure Service Bus service.
/// </summary>
ServiceCommunicationProblem,

/// <summary>
/// The requested session cannot be locked.
/// </summary>
SessionCannotBeLocked,

/// <summary>
/// The lock on the session has expired. Callers should request the session again.
/// </summary>
SessionLockLost,

/// <summary>
/// The user doesn't have access to the entity.
/// </summary>
Unauthorized,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be worthy of discussion. Feedback during Event Hubs was that we should use the natural exception in the language, if one applies. In this case, we were asked to use an UnauthorizedAccessException. which is also the behavior from the EH and SB clients in Track One.

Since we're using language types like ArgumentException, ArgumentOutOfRangeException, InvalidOperationException, and NotSupportedException rather than wrapping them in ServiceBusException, is there a reason that we don't want to do the same for the unauthorized scenario?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Track 1 uses a custom UnathorizedException that derives from ServiceBusException.
I think the distinction of when we use ServiceBusException is when the exception actually comes from the service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the other ones that I mentioned, which are in the AMQP mapping but go to standard language types. 😄

Copy link
Member

@JoshLove-msft JoshLove-msft Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point - I assumed you were referring to the client side validation, but you are right - sometimes we wrap service errors in standard language exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't feel strongly. Krzysztof did, at one point any may have guidance here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we should probably be consistent here for .NET.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KieranBrantnerMagee , I'm guessing that Python might have a similar concern but I don't know if there's a stdlib type exception that would cover this case (so you would have to make your own anyways).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshLove-msft - and for my own curiosity - are you saying you're going to swap over to using your native language exception here? I believe this change would only affect .net. @hemanttanwar - does Java have a similar built-in exception for this as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - there is no reason to differ from EH here, especially when we are already using native language exceptions for some service errors. If we were going to be consistent and always use SBException/EHException for service errors, it would be a different story.


/// <summary>
/// An entity with the same name exists under the same namespace.
/// </summary>
MessagingEntityAlreadyExists
}
```

[js_exception]: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/servicebus/service-bus/src/serviceBusError.ts
[js_sample]: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/servicebus/service-bus/samples/typescript/src/receiveMessagesStreaming.ts#L53
[csharp_failurereason]: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusFailureReason.cs#L9
[csharp_exception]: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusException.cs
[csharp_sample]: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/servicebus/Azure.Messaging.ServiceBus/samples/Sample04_Processor.md
[java_exception]: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverException.java
[python_exception]: https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/servicebus/azure-servicebus/azure/servicebus/exceptions.py#L213