Skip to content

Commit

Permalink
Update logging across solution AB#14264 (#6356)
Browse files Browse the repository at this point in the history
* Introduce Problem class and log HealthGatewayExceptions

The Problem class uses singletons to store data related to problem types.

* Update logging across solution

* Re-enable analyzer: documentation should end with period

* Remove unused method in V1 PatientService

* Update observability settings

* Fix communication caching bug
  • Loading branch information
MikeLyttle authored Oct 31, 2024
1 parent b65b2cd commit b406f72
Show file tree
Hide file tree
Showing 239 changed files with 2,266 additions and 2,921 deletions.
2 changes: 1 addition & 1 deletion .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ resharper_web_config_type_not_resolved_highlighting = warning
resharper_web_config_wrong_module_highlighting = warning

# SA1629: Documentation text should end with a period
dotnet_diagnostic.SA1629.severity = none
dotnet_diagnostic.SA1629.severity = warning

# IDE0058: Remove unnecessary expression value
dotnet_diagnostic.IDE0058.severity = none
Expand Down
12 changes: 11 additions & 1 deletion Apps/AccountDataAccess/src/Audit/AuditRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
namespace HealthGateway.AccountDataAccess.Audit
{
using System.Collections.Generic;
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;
using HealthGateway.Database.Delegates;
Expand All @@ -31,15 +32,24 @@ public class AuditRepository : IAuditRepository
/// <summary>
/// Initializes a new instance of the <see cref="AuditRepository"/> class.
/// </summary>
/// <param name="agentAuditDelegate">The inject agent audit delegate.</param>
/// <param name="agentAuditDelegate">The injected agent audit delegate.</param>
public AuditRepository(IAgentAuditDelegate agentAuditDelegate)
{
this.agentAuditDelegate = agentAuditDelegate;
}

private static ActivitySource ActivitySource { get; } = new(typeof(AuditRepository).FullName);

/// <inheritdoc/>
public async Task<IEnumerable<AgentAudit>> HandleAsync(AgentAuditQuery query, CancellationToken ct = default)
{
using Activity? activity = ActivitySource.StartActivity();
activity?.AddBaggage("AuditHdid", query.Hdid);
if (query.Group != null)
{
activity?.AddBaggage("AuditGroup", query.Group.ToString());
}

return await this.agentAuditDelegate.GetAgentAuditsAsync(query.Hdid, query.Group, ct);
}
}
Expand Down
2 changes: 1 addition & 1 deletion Apps/AccountDataAccess/src/Audit/IAuditRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public interface IAuditRepository
/// </summary>
/// <param name="query">The query.</param>
/// <param name="ct">The cancellation token.</param>
/// <returns>The list of agent audits..</returns>
/// <returns>The list of agent audits.</returns>
Task<IEnumerable<AgentAudit>> HandleAsync(AgentAuditQuery query, CancellationToken ct = default);
}

Expand Down
238 changes: 115 additions & 123 deletions Apps/AccountDataAccess/src/Patient/ClientRegistriesDelegate.cs

Large diffs are not rendered by default.

46 changes: 31 additions & 15 deletions Apps/AccountDataAccess/src/Patient/PatientRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace HealthGateway.AccountDataAccess.Patient
{
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
Expand Down Expand Up @@ -64,7 +65,7 @@ internal class PatientRepository : IPatientRepository
/// <param name="cacheProvider">The injected cache provider.</param>
/// <param name="configuration">The Configuration to use.</param>
/// <param name="patientQueryFactory">The injected patient query factory.</param>
/// <param name="messageSender">The change feed message sender</param>
/// <param name="messageSender">The change feed message sender.</param>
public PatientRepository(
ILogger<PatientRepository> logger,
IBlockedAccessDelegate blockedAccessDelegate,
Expand All @@ -85,12 +86,14 @@ public PatientRepository(
this.blockedDataSourcesChangeFeedEnabled = changeFeedConfiguration?.BlockedDataSources.Enabled ?? false;
}

private static ActivitySource ActivitySource { get; } = new(typeof(PatientRepository).FullName);

/// <inheritdoc/>
public async Task<PatientQueryResult> QueryAsync(PatientQuery query, CancellationToken ct = default)
{
return query switch
{
PatientDetailsQuery q => await this.HandleAsync(q, ct),
PatientDetailsQuery q => await this.HandlePatientDetailsQueryAsync(q, ct),

_ => throw new NotImplementedException($"{query.GetType().FullName}"),
};
Expand All @@ -99,9 +102,8 @@ public async Task<PatientQueryResult> QueryAsync(PatientQuery query, Cancellatio
/// <inheritdoc/>
public async Task<bool> CanAccessDataSourceAsync(string hdid, DataSource dataSource, CancellationToken ct = default)
{
this.logger.LogDebug("Checking if {DataSource} can be accessed by {Hdid}", dataSource, hdid);
IEnumerable<DataSource> blockedDataSources = await this.GetDataSourcesAsync(hdid, ct);
this.logger.LogDebug("Blocked data sources for hdid: {Hdid} - {DataSources}", hdid, blockedDataSources);

return !blockedDataSources.Contains(dataSource);
}

Expand Down Expand Up @@ -148,8 +150,9 @@ public async Task BlockAccessAsync(BlockAccessCommand command, CancellationToken

if (this.blockedDataSourcesChangeFeedEnabled)
{
this.logger.LogDebug("Sending change feed notification for blocked data sources");
IEnumerable<string> dataSourceValues = blockedAccess.DataSources.Select(ds => EnumUtility.ToEnumString(ds));
await this.messageSender.SendAsync(new[] { new MessageEnvelope(new DataSourcesBlockedEvent(command.Hdid, dataSourceValues), command.Hdid) }, ct);
await this.messageSender.SendAsync([new MessageEnvelope(new DataSourcesBlockedEvent(command.Hdid, dataSourceValues), command.Hdid)], ct);
}
}

Expand All @@ -163,12 +166,12 @@ public async Task BlockAccessAsync(BlockAccessCommand command, CancellationToken
[SuppressMessage("Performance", "CA1863:Use 'CompositeFormat'", Justification = "Team decision")]
public async Task<IEnumerable<DataSource>> GetDataSourcesAsync(string hdid, CancellationToken ct = default)
{
string blockedAccessCacheKey = string.Format(CultureInfo.InvariantCulture, ICacheProvider.BlockedAccessCachePrefixKey, hdid);
string message = $"Getting item from cache for key: {blockedAccessCacheKey}";
this.logger.LogDebug("{Message}", message);
using Activity? activity = ActivitySource.StartActivity();
activity?.AddBaggage("CacheIdentifier", hdid);
this.logger.LogDebug("Accessing blocked access cache");

IEnumerable<DataSource>? dataSources = await this.cacheProvider.GetOrSetAsync(
blockedAccessCacheKey,
string.Format(CultureInfo.InvariantCulture, ICacheProvider.BlockedAccessCachePrefixKey, hdid),
() => this.blockedAccessDelegate.GetDataSourcesAsync(hdid, ct),
TimeSpan.FromMinutes(this.blockedAccessCacheTtl),
ct);
Expand All @@ -178,19 +181,34 @@ public async Task<IEnumerable<DataSource>> GetDataSourcesAsync(string hdid, Canc

private static string GetStrategy(string? hdid, PatientDetailSource source)
{
string prefix = "HealthGateway.AccountDataAccess.Patient.Strategy.";
const string prefix = "HealthGateway.AccountDataAccess.Patient.Strategy.";
string type = hdid != null ? "Hdid" : "Phn";
const string suffix = "Strategy";
return $"{prefix}{type}{source}{suffix}";
}

private async Task<PatientQueryResult> HandleAsync(PatientDetailsQuery query, CancellationToken ct)
private async Task<PatientQueryResult> HandlePatientDetailsQueryAsync(PatientDetailsQuery query, CancellationToken ct)
{
this.logger.LogDebug("Patient details query source: {Source} - cache: {Cache}", query.Source, query.UseCache);
using Activity? activity = ActivitySource.StartActivity();

if (!string.IsNullOrEmpty(query.Hdid))
{
activity?.AddBaggage("PatientHdid", query.Hdid);
}

if (!string.IsNullOrEmpty(query.Phn))
{
activity?.AddBaggage("PatientPhn", query.Phn);
}

activity?.AddBaggage("UseCache", query.UseCache.ToString());
activity?.AddBaggage("Source", EnumUtility.ToEnumString(query.Source));

this.logger.LogDebug("Retrieving patient details");

if (ct.IsCancellationRequested)
{
throw new InvalidOperationException("cancellation was requested");
throw new InvalidOperationException("Cancellation was requested");
}

if (query.Hdid == null && query.Phn == null)
Expand All @@ -209,9 +227,7 @@ private async Task<PatientQueryResult> GetPatientAsync(PatientDetailsQuery query
disabledValidation);

string strategy = GetStrategy(query.Hdid, query.Source);
this.logger.LogDebug("Strategy: {Strategy}", strategy);

// Get the appropriate strategy from factory to query patient
PatientQueryStrategy patientQueryStrategy = this.patientQueryFactory.GetPatientQueryStrategy(strategy);
PatientQueryContext context = new(patientQueryStrategy);
PatientModel patient = await context.GetPatientAsync(patientRequest, ct);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,16 @@ public override async Task<PatientModel> GetPatientAsync(PatientRequest request,
}
catch (CommunicationException ce)
{
this.GetLogger().LogError(ce, "Will call PHSA for patient due to EMPI Communication Exception when trying to retrieve patient information: {Message}", ce.Message);
this.GetLogger().LogWarning(ce, "Error retrieving patient data from the Client Registry");

try
{
this.GetLogger().LogDebug("Retrieving patient from PHSA");
PatientIdentity result = await this.patientIdentityApi.GetPatientIdentityAsync(request.Identifier, ct);
patient = this.mapper.Map<PatientModel>(result);
}
catch (ApiException e) when (e.StatusCode == HttpStatusCode.NotFound)
{
this.GetLogger().LogInformation(e, "PHSA could not find patient identity for {Hdid}", request.Identifier);
throw new NotFoundException(ErrorMessages.PatientIdentityNotFound);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ public override async Task<PatientModel> GetPatientAsync(PatientRequest request,
{
try
{
this.GetLogger().LogDebug("Retrieving patient from PHSA");
PatientIdentity result = await this.patientIdentityApi.GetPatientIdentityAsync(request.Identifier, ct);
patient = this.mapper.Map<PatientModel>(result);
}
catch (ApiException e) when (e.StatusCode == HttpStatusCode.NotFound)
{
this.GetLogger().LogInformation(e, "PHSA could not find patient identity for {Hdid}", request.Identifier);
throw new NotFoundException(ErrorMessages.PatientIdentityNotFound);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,7 @@ public PatientQueryFactory(IServiceProvider serviceProvider)
/// <returns>Requested instance of <see cref="PatientQueryStrategy"/> class.</returns>
public PatientQueryStrategy GetPatientQueryStrategy(string strategy)
{
Type? type = Type.GetType(strategy);

if (type == null)
{
throw new ArgumentException($"Invalid strategy type: {strategy}");
}

Type type = Type.GetType(strategy) ?? throw new ArgumentException($"Invalid strategy type {strategy}");
return (PatientQueryStrategy)this.serviceProvider.GetRequiredService(type);
}
}
Expand Down
72 changes: 31 additions & 41 deletions Apps/AccountDataAccess/src/Patient/Strategy/PatientQueryStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ protected PatientQueryStrategy(
this.cacheTtl = configuration.GetSection("PatientService").GetValue("CacheTTL", 0);
}

private static ActivitySource Source { get; } = new(nameof(PatientQueryStrategy));
private static ActivitySource ActivitySource { get; } = new(typeof(PatientQueryStrategy).FullName);

/// <summary>
/// Returns patient model based on the implemented Strategy.
Expand Down Expand Up @@ -87,33 +87,28 @@ protected ILogger GetLogger()
/// <returns>The found Patient model or null.</returns>
protected async Task<PatientModel?> GetFromCacheAsync(string identifier, PatientIdentifierType identifierType, CancellationToken ct)
{
using Activity? activity = Source.StartActivity();
PatientModel? retPatient = null;
if (this.cacheTtl > 0)
if (this.cacheTtl == 0)
{
switch (identifierType)
{
case PatientIdentifierType.Hdid:
this.logger.LogDebug("Querying Patient Cache by HDID");
retPatient = await this.cacheProvider.GetItemAsync<PatientModel>($"{PatientCacheDomain}:HDID:{identifier}", ct);
break;

case PatientIdentifierType.Phn:
this.logger.LogDebug("Querying Patient Cache by PHN");
retPatient = await this.cacheProvider.GetItemAsync<PatientModel>($"{PatientCacheDomain}:PHN:{identifier}", ct);
break;
}

string message = $"Patient with identifier {identifier} was {(retPatient == null ? "not" : string.Empty)} found in cache";
this.logger.LogDebug("{Message}", message);
return null;
}

activity?.Stop();
using Activity? activity = ActivitySource.StartActivity();
activity?.AddBaggage("CacheIdentifier", identifier);
this.logger.LogDebug("Accessing patient cache via {CacheIdentifierType}", identifierType);

PatientModel? retPatient = identifierType switch
{
PatientIdentifierType.Hdid => await this.cacheProvider.GetItemAsync<PatientModel>($"{PatientCacheDomain}:HDID:{identifier}", ct),
PatientIdentifierType.Phn => await this.cacheProvider.GetItemAsync<PatientModel>($"{PatientCacheDomain}:PHN:{identifier}", ct),
_ => null,
};

this.logger.LogDebug("Patient cache access outcome: {CacheResult}", retPatient == null ? "miss" : "hit");
return retPatient;
}

/// <summary>
/// Caches the Patient model if patient is not null and validation is enabled.
/// Caches a patient model if it's not null, validation is enabled, and the cache is enabled.
/// </summary>
/// <param name="patient">The patient to cache.</param>
/// <param name="disabledValidation">bool indicating if disabledValidation was set.</param>
Expand All @@ -122,42 +117,37 @@ protected ILogger GetLogger()
protected async Task CachePatientAsync(PatientModel? patient, bool disabledValidation, CancellationToken ct)
{
// Only cache if validation is enabled (as some clients could get invalid data) and when successful.
if (patient != null && !disabledValidation)
if (patient != null && !disabledValidation && this.cacheTtl > 0)
{
await this.CachePatientAsync(patient, ct);
}
}

/// <summary>
/// Caches the Patient model if enabled.
/// Caches a patient model.
/// </summary>
/// <param name="patientModel">The patient to cache.</param>
/// <param name="ct">The cancellation token.</param>
/// <returns>A <see cref="Task"/> representing the result of the asynchronous operation.</returns>
private async Task CachePatientAsync(PatientModel patientModel, CancellationToken ct)
{
using Activity? activity = Source.StartActivity();
string hdid = patientModel.Hdid;
if (this.cacheTtl > 0)
using Activity? activity = ActivitySource.StartActivity();

TimeSpan expiry = TimeSpan.FromMinutes(this.cacheTtl);

if (!string.IsNullOrEmpty(patientModel.Hdid))
{
this.logger.LogDebug("Attempting to cache patient: {Hdid}", hdid);
TimeSpan expiry = TimeSpan.FromMinutes(this.cacheTtl);
if (!string.IsNullOrEmpty(patientModel.Hdid))
{
await this.cacheProvider.AddItemAsync($"{PatientCacheDomain}:HDID:{patientModel.Hdid}", patientModel, expiry, ct);
}

if (!string.IsNullOrEmpty(patientModel.Phn))
{
await this.cacheProvider.AddItemAsync($"{PatientCacheDomain}:PHN:{patientModel.Phn}", patientModel, expiry, ct);
}
activity?.AddBaggage("CacheIdentifier", patientModel.Hdid);
this.logger.LogDebug("Storing patient in cache via {CacheIdentifierType}", PatientIdentifierType.Hdid);
await this.cacheProvider.AddItemAsync($"{PatientCacheDomain}:HDID:{patientModel.Hdid}", patientModel, expiry, ct);
}
else

if (!string.IsNullOrEmpty(patientModel.Phn))
{
this.logger.LogDebug("Patient caching is disabled will not cache patient: {Hdid}", hdid);
activity?.AddBaggage("CacheIdentifier", patientModel.Phn);
this.logger.LogDebug("Storing patient in cache via {CacheIdentifierType}", PatientIdentifierType.Phn);
await this.cacheProvider.AddItemAsync($"{PatientCacheDomain}:PHN:{patientModel.Phn}", patientModel, expiry, ct);
}

activity?.Stop();
}
}

Expand Down
2 changes: 1 addition & 1 deletion Apps/AccountDataAccess/test/unit/PatientRepositoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public async Task QueryThrowsInvalidOperationException(string? hdid, string? phn
Source: PatientDetailSource.Empi,
UseCache: true);

string expected = ct.IsCancellationRequested ? "cancellation was requested" : "Must specify either Hdid or Phn to query patient details";
string expected = ct.IsCancellationRequested ? "Cancellation was requested" : "Must specify either Hdid or Phn to query patient details";

IPatientRepository patientRepository = SetupPatientRepositoryForQueryThrowsInvalidOperationException();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public static class RefitExceptionUtil
/// Creates a Refit exception with the given status code and reason phrase.
/// </summary>
/// <param name="statusCode">The desired exception status code.</param>
/// <returns>Refit ApiException</returns>
/// <returns>Refit ApiException.</returns>
public static async Task<ApiException> CreateApiException(HttpStatusCode statusCode)
{
RefitSettings rfSettings = new();
Expand Down
2 changes: 1 addition & 1 deletion Apps/Admin/Client/Services/DateConversionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace HealthGateway.Admin.Client.Services
using Microsoft.Extensions.Configuration;

/// <inheritdoc/>
/// <param name="configuration">Injected application settings</param>
/// <param name="configuration">The injected configuration.</param>
public class DateConversionService(IConfiguration configuration) : IDateConversionService
{
/// <inheritdoc/>
Expand Down
2 changes: 1 addition & 1 deletion Apps/Admin/Client/Services/IDateConversionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public interface IDateConversionService
/// <summary>
/// Converts a nullable UTC datetime values to the system configured timezone.
/// </summary>
/// <param name="utcDateTime">Nullable utcDateTime</param>
/// <param name="utcDateTime">Nullable utcDateTime.</param>
/// <param name="returnNowIfNull">If utcDateTime is null, this flag can be used to return now.</param>
/// <returns>DateTime converted or null if returnNowIfNull is false else will return now.</returns>
public DateTime? ConvertFromUtc(DateTime? utcDateTime, bool returnNowIfNull = false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,6 @@ namespace HealthGateway.Admin.Common.Models.AdminReports
/// Represents a patient with blocked access to data sources.
/// </summary>
/// <param name="Hdid">Patient's HDID.</param>
/// <param name="BlockedSources">List of <see cref="DataSource"/> that are blocked</param>
/// <param name="BlockedSources">List of <see cref="DataSource"/> that are blocked.</param>
public record BlockedAccessRecord(string Hdid, IList<DataSource> BlockedSources);
}
Loading

0 comments on commit b406f72

Please sign in to comment.