-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Expose error details in AMQ APIs #22114
Expose error details in AMQ APIs #22114
Conversation
sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs
Outdated
Show resolved
Hide resolved
@@ -18,6 +18,15 @@ public enum ResponseStatusOption | |||
} | |||
namespace Azure.Core | |||
{ | |||
public sealed partial class AzureError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this concept specific to Azure? i.e. should this be called ServiceError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was called ServiceError
in the first iteration. This exact shape is declared Azure REST guidenline as is Azure-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to call it ServiceError
because that'll cause a lot of confusion for anyone trying to search: https://docs.microsoft.com/en-us/dotnet/api/?term=serviceerror
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that'll cause a lot of confusion for anyone trying to search
Good point. Thought, I would still like to avoid using "Azure" in general purpose APIs. Maybe ServiceResponseError? or just ResponseError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs.microsoft.com/en-us/dotnet/api/?term=serviceerror
I don't think there are that many search results. For example, https://docs.microsoft.com/en-us/dotnet/api/?term=Response didn't stop us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like either RequestError
to tie it to RequestFailedException
or ResponseError
to tie it to Response*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestError
sounds like it's a validation error (the request is wrong).
ErrorResponse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ErrorResponse implies that it's a subtype of Response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the ServiceError
the most. It describes exactly what the type is about.
{ | ||
get | ||
{ | ||
_body.HasFailed = Status >= 400; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the value of HasFailed
ever be false when the service operation succeeded? If so, why wouldn't we throw RequestFailedException
rather than making someone interrogate this property? Is this just used for batches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for the batch response were some queries can succeed and some fail. For the single query scenario we always throw on failure.
sdk/monitor/Azure.Monitor.Query/src/Models/LogsBatchQueryResults.cs
Outdated
Show resolved
Hide resolved
…ts.cs Co-authored-by: Christopher Scott <chriscott@hotmail.com>
{ | ||
internal ResponseError() { } | ||
public string? Code { get { throw null; } } | ||
public System.Collections.Generic.IReadOnlyList<Azure.Core.ResponseError> Details { get { throw null; } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit strange that details of X is a collection of X.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's even stranger that InnerError is a different type... but https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#7102-error-condition-responses
Both batched and non-batched queries can partially succeed. Expose the error type and define it in Azure.Core.Experemental because it follows Azure-wide error definition from https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#errorresponse--object