Skip to content

Conversation

@pakrym
Copy link
Contributor

@pakrym pakrym commented Jun 23, 2021

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

@ghost ghost added the Azure.Core label Jun 23, 2021
@pakrym pakrym requested a review from christothes June 24, 2021 16:06
}
namespace Azure.Core
{
public sealed partial class AzureError
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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*.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

{
internal ResponseError() { }
public string? Code { get { throw null; } }
public System.Collections.Generic.IReadOnlyList<Azure.Core.ResponseError> Details { get { throw null; } }
Copy link
Member

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.

Copy link
Member

@tg-msft tg-msft Jun 24, 2021

Choose a reason for hiding this comment

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

@pakrym pakrym merged commit dfe0462 into Azure:main Jun 24, 2021
@pakrym pakrym deleted the pakrym/Expose-error-details-in-AMQ-APIs branch June 24, 2021 21:37
@scottaddie scottaddie added Monitor Monitor, Monitor Ingestion, Monitor Query and removed Monitor - Log labels Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core Monitor Monitor, Monitor Ingestion, Monitor Query

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants