add methods for health checks #41
Conversation
wi-y
commented
Mar 23, 2020
wi-y
commented
- added methods for health checks
- a developer can add CheckHealth() to Functions.cs wrapped in a try/catch to check health of fhir server
src/lib/Microsoft.Health.Fhir.Ingest/Service/FhirImportService.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Overall I like the design and where you have the code. Before completion I would like to see HealthCheck data class moved to the data namespace, adding a cancellation token & make async, and some simple unit tests for the different exceptions you service is responding too. Other comments are optional. Let me know if you would like to discuss further, Thanks!
| { | ||
| public abstract class FhirHealthService | ||
| { | ||
| public abstract Task<FhirHealthCheckStatus> CheckHealth(); |
There was a problem hiding this comment.
This should probably be updated to take a cancellation token since this will be asynchronous. You can set it to default if you would like but it should be an option.
src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirHealthService.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirHealthService.cs
Outdated
Show resolved
Hide resolved
| catch (Exception ex) | ||
| { | ||
| return Task.FromResult(new FhirHealthCheckStatus(ex.Message, 500)); | ||
| throw; |
There was a problem hiding this comment.
Do we want to throw here? I would think we want to record the issue and be done. Was there a specific case you wanted to throw for?
There was a problem hiding this comment.
Not really, I only added it originally because of the linter. I went ahead and removed the throw
|
|
||
| namespace Microsoft.Health.Fhir.Ingest.Service | ||
| { | ||
| public class R4FhirHealthService : |
There was a problem hiding this comment.
Can we have unit tests with mocks of the exceptions to test the various outputs?
|
|
||
| namespace Microsoft.Health.Fhir.Ingest.Service | ||
| { | ||
| public abstract class FhirHealthService |
There was a problem hiding this comment.
Thoughts on the public method just returning Task and taking an ILogger for telemetry recording? In this instance you would still have version of CheckHealth returning the FhirHealthCheckStatus but it would be a protected abstract member that the implementer needs to specify. Then the public method of Task Name(ILogger, CancelationToken) would record the result in a standard way in the logger so the upstream implementer doesn't need to. Only thing you need in the function definition is the instance of the Health Check & the ILogger which you can get with DI on the function signature. This also makes the use of the abstract class more appropriate. Right now the abstract class doesn't have any implementation outside a contract and could just be an interface.
There was a problem hiding this comment.
That makes sense, but I'm not sure I would like to address this at this moment
There was a problem hiding this comment.
Okay, it can be part of the next step.
| sp => | ||
| { | ||
| var fhirClient = sp.GetRequiredService<IFhirClient>(); | ||
| var resourceIdentityOptions = sp.GetRequiredService<IOptions<ResourceIdentityOptions>>(); | ||
| return ResourceIdentityServiceFactory.Instance.Create(resourceIdentityOptions.Value, fhirClient); | ||
| }); | ||
| builder.Services.AddSingleton<IMemoryCache>(sp => new MemoryCache(Options.Create<MemoryCacheOptions>(new MemoryCacheOptions { SizeLimit = 5000 }))); | ||
| builder.Services.AddSingleton<FhirImportService, R4FhirImportService>(); | ||
| builder.Services.TryAddSingleton<IMemoryCache>(sp => new MemoryCache(Options.Create<MemoryCacheOptions>(new MemoryCacheOptions { SizeLimit = 5000 }))); |
There was a problem hiding this comment.
Looks good, thanks for making this change!
|
|
||
| namespace Microsoft.Health.Fhir.Ingest.Service | ||
| { | ||
| public abstract class FhirHealthService |
There was a problem hiding this comment.
Okay, it can be part of the next step.
| _fhirHealthService = EnsureArg.IsNotNull(fhirHealthService, nameof(fhirHealthService)); | ||
| } | ||
|
|
||
| public async Task<FhirHealthCheckStatus> CheckHealth(CancellationToken token = default) |
There was a problem hiding this comment.
standard convention is to have an Async suffix on Async methods.
