Add additional error handling to CosmosHealthCheck #4781
Conversation
mikaelweave
added
Bug
mikaelweave
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/Microsoft.Health.Fhir.CosmosDb.UnitTests/Features/Health/CosmosHealthCheckTests.cs
Fixed
Show resolved
Hide resolved
| ex, | ||
| "Failed to connect to the data store. Request has timed out."); | ||
|
|
||
| return HealthCheckResult.Degraded( |
There was a problem hiding this comment.
Should timeout requests to CosmosDB result in Degraded or ServiceUnavailable? 408 status code can mean the database is overloaded from client requests.
There was a problem hiding this comment.
Discussed with @fhibf - degraded is the proper behavior here.
| VerifyErrorInResult(result.Data, "Error", FhirHealthErrorCode.Error408.ToString()); | ||
| } | ||
|
|
||
| private void VerifyErrorInResult(IReadOnlyDictionary<string, object> dictionary, string key, string expectedMessage) |
There was a problem hiding this comment.
Added this helper due to code scanning errors.
| // Reference: https://learn.microsoft.com/azure/cosmos-db/nosql/conceptual-resilient-sdk-applications#should-my-application-retry-on-errors | ||
| static bool IsRetryableException(Exception ex) => | ||
| ex is CosmosOperationCanceledException || | ||
| (ex is CosmosException cex && (cex.StatusCode == HttpStatusCode.ServiceUnavailable || cex.StatusCode == (HttpStatusCode)449)); |
There was a problem hiding this comment.
Should we include HttpStatusCode 408 as well?
There was a problem hiding this comment.
Reading the code now I see that HTTP408 is treated differently. That's fine.
|
|
||
| void LogAdditionalRetryableExceptionDetails(Exception exception) | ||
| { | ||
| if (exception is CosmosException cosmosException && cosmosException.StatusCode == HttpStatusCode.ServiceUnavailable) |
There was a problem hiding this comment.
Should we also add the logginc for HTTP449 and HTTP408?
There was a problem hiding this comment.
Actually this can be removed because it's added when useful to the exception message in the CosmosSDK.
Description
Related issues
AB#137391
Testing
Unit testing.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)