Don't treat 409 response to conditional request as an error #20064
Conversation
ghost
added
the
| @@ -90,6 +90,10 @@ private async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPip | |||
|
|
|||
| activity.AddTag("http.status_code", message.Response.Status.ToString(CultureInfo.InvariantCulture)); | |||
| activity.AddTag("serviceRequestId", message.Response.Headers.RequestId); | |||
| if (message.ResponseClassifier.IsErrorResponse(message)) | |||
| { | |||
| activity.AddTag("otel.status_code", "ERROR"); | |||
There was a problem hiding this comment.
This allows us to remove the status calculation logic from the AppInsights. PR to follow.
|
I talked with @tg-msft and we agreed with a more targeted ResponseClassifier implementation in storage instead of changing the global one. |
pakrym
requested review from
amishra-dev,
amnguye,
kasobol-msft and
seanmcc-msft
as code owners
|
@kasobol-msft this change would reduce amount of error spans customers see in AppInsights/logs when making CreateIfNotExist calls. |
sdk/storage/Azure.Storage.Common/src/Shared/StorageResponseClassifier.cs
Outdated
Show resolved
Hide resolved
| header.Contains(HttpHeader.Names.IfNoneMatch) || | ||
| header.Contains(HttpHeader.Names.IfModifiedSince) || | ||
| header.Contains(HttpHeader.Names.IfUnmodifiedSince); | ||
| return !isConditional; |
There was a problem hiding this comment.
Help new devs wondering what this is for with a comment like:
// We're not considering Container/BlobAlreadyExists as errors when the request has conditional headers.
// Convenience methods like BlobContainerClient.CreateIfNotExists will cause a lot of these responses and
// we don't want them polluting AppInsights with noise. See RequestActivityPolicy for how this is applied.
| @@ -50,7 +50,12 @@ public override bool IsErrorResponse(HttpMessage message) | |||
| switch (message.Response.Status) | |||
| { | |||
| case 409: | |||
There was a problem hiding this comment.
Should we do 412 here as well? We've had requests around that as well, since we expect preconditions to fail during concurrent operations; it also isn't an exceptional situation.
There was a problem hiding this comment.
Looking at https://docs.microsoft.com/en-us/rest/api/storageservices/blob-service-error-codes I don't see anything I want to eagerly add for now.
…onal-request-as-an-error # Conflicts: # sdk/core/Azure.Core/CHANGELOG.md # sdk/core/Azure.Core/src/Azure.Core.csproj
pakrym
deleted the
pakrym/Don-t-treat-409-response-to-conditional-request-as-an-error
branch
|
Sorry if this is a stupid question but when do these changes get released? How can I tell if they are? Do I need to update anything on my end? Because I am still getting these errors in my ApplicationInsights for BlobBaseClient.Exists() |
| public void IsError_409_ConditionalResponse(string code, string header, bool isError) | ||
| { | ||
| var mockRequest = new MockRequest(); | ||
| mockRequest.Headers.Add("x-ms-error-code", code); |
There was a problem hiding this comment.
This test adds the error code to the request, when it should be on the response.
Contributes to: #19982