Customized FHIR service configuration exceptions #150
Conversation
src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientFactory.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientFactory.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientFactory.cs
Outdated
Show resolved
Hide resolved
...ib/Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceExceptionProcessor.cs
Show resolved
Hide resolved
...ib/Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceExceptionProcessor.cs
Outdated
Show resolved
Hide resolved
| case MsalServiceException _: | ||
| string errorCode = ((MsalServiceException)exception).ErrorCode; | ||
| message = FhirResources.FhirServiceMsalServiceError; | ||
| errorName = $"{nameof(FhirServiceErrorCode.ConfigurationError)}_{errorCode}"; |
There was a problem hiding this comment.
Should this be auth error instead?
There was a problem hiding this comment.
From my testing, no, because MsalServiceException was given for invalid FHIR service URLs such as https://clai1015.azurehealthcareapis.com/metadata, https://clai1015.azurehealthcareapis.com///, https://sdfsldfkwr389lksnldsf.com
In commit 9ab3837, I've scoped MsalServiceException down by checking the ErrorCode property.
src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs
Outdated
Show resolved
Hide resolved
…ames and updated redundant string interpolations.
| try | ||
| { | ||
| client = new FhirClient(url, fhirClientSettings, new BearerTokenAuthorizationMessageHandler(uri, tokenCredential, logger)); | ||
| FhirServiceValidator.ValidateFhirService(client, logger); |
There was a problem hiding this comment.
So currently if this fails, we just log the exception and return false, so what happens in the rest of the execution flow in that case?
There was a problem hiding this comment.
In this case (when ValidateFhirService catches an exception, gets the exception logged, and returns false), FhirClient is returned (Line 90), as before these code changes.
There was a problem hiding this comment.
Sure but I was trying to understand that if this validation failed and we still returned the client, then what happens in the ingestion flow? Do we encounter other exceptions that block the ingestion?
There was a problem hiding this comment.
As discussed offline, what happens when an invalid client is returned is something to further investigate later. For now, the previous behaviour is still retained: a client, whether valid or not, gets returned.
| { | ||
| _ = await SaveObservationAsync(config, grp, ids).ConfigureAwait(false); | ||
| FhirServiceExceptionProcessor.ProcessException(ex, _logger); |
There was a problem hiding this comment.
I think we should retain the previous behaviour and not eat the exception?
There was a problem hiding this comment.
Just clarifying - do you mean to wrap only the line with ResolveResourceIdentitiesAsync in the try-block, so that the other lines are still executed if we do get an exception thrown (for an authorization error) from ResolveResourceIdentitiesAsync?
There was a problem hiding this comment.
To retain the previous behaviour where this function would throw if we encountered an exception, we can just throw it back after processing the exception. Unless we feel like throwing it back doesn't make sense anymore? I'm worried that if we were previously retrying on this exception, then by eating the exception now, we will not be and if that is desired.
There was a problem hiding this comment.
Oh I see what you mean, thanks for the details. I think you bring up a good point; I'll re-throw while keeping the stack trace.
| m.Dimensions[DimensionNames.Operation].Equals(ConnectorOperation.FHIRConversion) && | ||
| m.Dimensions[DimensionNames.Category].Equals(Category.Errors) && | ||
| m.Dimensions[DimensionNames.ErrorType].Equals(ErrorType.FHIRServiceError) && | ||
| m.Dimensions[DimensionNames.ErrorSeverity].Equals(ErrorSeverity.Critical) && |
There was a problem hiding this comment.
nit: Maybe this can go into a separate method like ValidateFHIRServiceErrorProperteies() similar to a recent change you made in the other test file.
Added custom exceptions for the following scenarios:
Also renamed "FhirServerError" to "FhirServiceError" for consistency with product name "FHIR service".