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

richardpark-msft · 2020-11-04T23:19:49Z

No description provided.

jsquire · 2020-11-05T15:12:30Z

_discussion_reason_codes.md

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


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?

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.

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

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.

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

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

@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).

@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?

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.

jsquire · 2020-11-05T15:20:15Z

_discussion_reason_codes.md

+
+## Additional questions
+
+- [ ] Are users expected to react to these errors? If so, what other flags/information might we need?


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.

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?

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

We do have the retryable flag for that! :)

Retryable = IsTransient

Don't you mean Retriable? 😉

richardpark-msft · 2020-11-05T21:16:36Z

@chradek - any conflicts you see with what Event Hubs does? We're basically being additive with the reason code (and even then, this is only currently in service bus). But I'm guessing we'd want to potentially expose this there as well.

chradek · 2020-11-06T00:04:15Z

@richardpark-msft
Not specific to event hubs, but core-amqp uses code instead of reason to distinguish MessagingErrors. I think we chose code partially because many errors thrown from the node.js runtime have a code field that users can check against.

I can't help but notice that many of the errors listed in the enum here match what we have in core-amqp, and as a user I'd probably be confused if I sometimes saw MessageLockLostError in code and sometimes in reason depending whether it was core-amqp or service-bus that threw the error :)

Other than that, looks reasonable to me!

richardpark-msft · 2020-11-11T21:32:13Z

@richardpark-msft
Not specific to event hubs, but core-amqp uses code instead of reason to distinguish MessagingErrors. I think we chose code partially because many errors thrown from the node.js runtime have a code field that users can check against.

I can't help but notice that many of the errors listed in the enum here match what we have in core-amqp, and as a user I'd probably be confused if I sometimes saw MessageLockLostError in code and sometimes in reason depending whether it was core-amqp or service-bus that threw the error :)

Other than that, looks reasonable to me!

Yeah, it's definitely a reasonable concern.

In the documentation the only spot they can really see a list of checkable codes will be on the reason field (the '.code' field is just a string) but if they were just inspecting the actual data that's passed to them there definitely could be that confusion.

We do demonstrate how to do error checking in our receiveMessagesStreaming sample so I think we'll be okay.

ramya-rao-a · 2020-11-12T08:14:59Z

Update:

JS SDK is now updated to use the same set of reasons in its ServiceBusError class with the difference that it would use the field code instead of reason
[Service Bus] Introduce ServiceBusException with limited set of error condition/code/reason azure-sdk-for-java#17500 is the proposal to do something similar in Java
In Python, we have sub classes for a few category of errors, but they dont have counterparts for each of the reasons above. Similarly, other languages do not have a counterpart to each of the sub classes in Python

Adding default values to fix correctness issue (Azure#12286) * Adding default values to fix correctness issue * revoke default value for allowedOrigins

richardpark-msft · 2021-02-12T00:30:53Z

The discussion on this has long since passed and we shipped the result of it.

adding in initial discussion fodder

2a1bad0

richardpark-msft added the design-discussion An area of design currently under discussion and open to team and community feedback. label Nov 4, 2020

richardpark-msft requested review from hemanttanwar, JoshLove-msft, yunhaoling, KieranBrantnerMagee, conniey, jsquire, MiYanni, ramya-rao-a, chradek and HarshaNalluru November 4, 2020 23:21

richardpark-msft changed the title ~~[service-bus] Design discussion~~ [service-bus] Design discussion for error reporting Nov 4, 2020

jsquire reviewed Nov 5, 2020

View reviewed changes

richardpark-msft closed this Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

richardpark-msft commented Nov 4, 2020

jsquire Nov 5, 2020

JoshLove-msft Nov 5, 2020

jsquire Nov 5, 2020

JoshLove-msft Nov 5, 2020 •

edited

Loading

jsquire Nov 5, 2020

JoshLove-msft Nov 5, 2020

richardpark-msft Nov 5, 2020

richardpark-msft Nov 5, 2020

JoshLove-msft Nov 5, 2020

jsquire Nov 5, 2020 •

edited

Loading

HarshaNalluru Nov 5, 2020

jsquire Nov 5, 2020

HarshaNalluru Nov 5, 2020

JoshLove-msft Nov 5, 2020

MiYanni Nov 5, 2020

richardpark-msft commented Nov 5, 2020

chradek commented Nov 6, 2020

richardpark-msft commented Nov 11, 2020

ramya-rao-a commented Nov 12, 2020 •

edited

Loading

richardpark-msft commented Feb 12, 2021


		## Additional questions

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

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

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

Conversation

richardpark-msft commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshLove-msft Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquire Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpark-msft commented Nov 5, 2020

chradek commented Nov 6, 2020

richardpark-msft commented Nov 11, 2020

ramya-rao-a commented Nov 12, 2020 • edited Loading

richardpark-msft commented Feb 12, 2021

JoshLove-msft Nov 5, 2020 •

edited

Loading

jsquire Nov 5, 2020 •

edited

Loading

ramya-rao-a commented Nov 12, 2020 •

edited

Loading