Don't treat 409 response to conditional request as an error#20064
Conversation
| 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. |
|
@kasobol-msft this change would reduce amount of error spans customers see in AppInsights/logs when making CreateIfNotExist calls. |
| 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
|
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