New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new health check for improper behavior #3818

Merged

LTA-Thinking merged 7 commits into main from personal/rojo/fix-search-parameter-initialization

Apr 25, 2024

Collaborator

LTA-Thinking commented Apr 18, 2024

Description

Add new health check for improper behavior.

Related issues

Addresses Bug 119273: Sort tests failing in Cosmos DB

Testing

Describe how this change was tested.

FHIR Team Checklist

Update the title of the PR to be succinct and less than 65 characters
Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
CI is green before merge
Review squash-merge requirements

Semver Change (docs)

Patch


          Add new health check for improper behavior

5f4ba40

LTA-Thinking requested a review from a team as a code owner

April 18, 2024 18:25

LTA-Thinking added Bug Azure API for FHIR labels

LTA-Thinking modified the milestones: S138, S139

github-advanced-security bot found potential problems

View reviewed changes

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs Fixed Show fixed Hide fixed

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs Fixed Show fixed Hide fixed


          Add exception

bae3441

fhibf reviewed

View reviewed changes

src/Microsoft.Health.Fhir.Api/Features/Health/ImproperBehaviorHealthCheck.cs Outdated Show resolved Hide resolved

fhibf reviewed

View reviewed changes

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs Outdated Show resolved Hide resolved

fhibf reviewed

View reviewed changes

src/Microsoft.Health.Fhir.Core/Features/Health/ImproperBehaviorNotification.cs Outdated Show resolved Hide resolved


          Address PR comments

ed5fa5b

fhibf reviewed

View reviewed changes

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs Outdated

                       }
                       public async Task Handle(StorageInitializedNotification notification, CancellationToken cancellationToken)
                       {
-                          _logger.LogInformation("SearchParameterDefinitionManager: Storage initialized");
-                          await EnsureInitializedAsync(cancellationToken);
+                          try

Contributor

fhibf Apr 19, 2024

Should we try adding a retry logic?
Maybe a 3 times retry, otherwise set it as unhealthy?


          Increase initial throughput and fix startup

fhibf previously approved these changes

View reviewed changes

github-advanced-security bot found potential problems

View reviewed changes

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs

Comment on lines +96 to +99

+                          catch
+                          {
+                              _initialized = false;
+                          }

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs

Comment on lines +254 to +258

+                              catch (Exception ex)
+                              {
+                                  _logger.LogError(ex, $"Error calculating search parameter hash. Retry {retry}");
+                                  retry++;
+                              }

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.

src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs

Comment on lines +275 to +279

+                              catch (Exception ex)
+                              {
+                                  _logger.LogError(ex, $"Error initializing search parameters. Retry {retry}");
+                                  retry++;
+                              }

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.


          Fix integration tests and startup ordering

8e1c127

LTA-Thinking dismissed fhibf’s stale review via

8e1c127

April 22, 2024 15:29

github-advanced-security bot found potential problems

View reviewed changes

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Outdated

-                      private readonly IMediator _mediator = Substitute.For<IMediator>();
-                      private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor = Substitute.For<RequestContextAccessor<IFhirRequestContext>>();
-                      private readonly SqlQueueClient _sqlQueueClient;
+                      private int _maximumSupportedSchemaVersion;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_maximumSupportedSchemaVersion' can be 'readonly'.

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Outdated

-                      private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor = Substitute.For<RequestContextAccessor<IFhirRequestContext>>();
-                      private readonly SqlQueueClient _sqlQueueClient;
+                      private int _maximumSupportedSchemaVersion;
+                      private string _databaseName;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_databaseName' can be 'readonly'.

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Outdated

+                      private SearchParameterDefinitionManager _searchParameterDefinitionManager;
+                      private SupportedSearchParameterDefinitionManager _supportedSearchParameterDefinitionManager;
+                      private SearchParameterStatusManager _searchParameterStatusManager;
+                      private IMediator _mediator = Substitute.For<IMediator>();

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_mediator' can be 'readonly'.

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Outdated

+                      private SupportedSearchParameterDefinitionManager _supportedSearchParameterDefinitionManager;
+                      private SearchParameterStatusManager _searchParameterStatusManager;
+                      private IMediator _mediator = Substitute.For<IMediator>();
+                      private RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor = Substitute.For<RequestContextAccessor<IFhirRequestContext>>();

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_fhirRequestContextAccessor' can be 'readonly'.

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Outdated

+                      private IMediator _mediator = Substitute.For<IMediator>();
+                      private RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor = Substitute.For<RequestContextAccessor<IFhirRequestContext>>();
+                      private SqlQueueClient _sqlQueueClient;
+                      private string _initialConnectionString;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_initialConnectionString' can be 'readonly'.

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Outdated

+                      private RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor = Substitute.For<RequestContextAccessor<IFhirRequestContext>>();
+                      private SqlQueueClient _sqlQueueClient;
+                      private string _initialConnectionString;
+                      private IOptions<CoreFeatureConfiguration> _options;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_options' can be 'readonly'.

...crosoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs Dismissed Show dismissed Hide dismissed

Robert Johnson added 2 commits

April 22, 2024 14:40


          Fix unit tests

d3537c4


          Address comments

0864ad8

fhibf approved these changes

View reviewed changes

fhibf reviewed

View reviewed changes

src/Microsoft.Health.Fhir.Core/Exceptions/InitializationException.cs

+              namespace Microsoft.Health.Fhir.Core.Exceptions
+              {
+                  public class InitializationException : Exception

Contributor

fhibf Apr 23, 2024

Should this inherit from FhirException?

Collaborator Author

LTA-Thinking Apr 23, 2024

I don't think so because it isn't related to FHIR, it is related to the server startup.

Collaborator Author

LTA-Thinking commented Apr 23, 2024

/azp run

azure-pipelines bot commented Apr 23, 2024

Azure Pipelines successfully started running 1 pipeline(s).

LTA-Thinking merged commit 6e4612e into main

43 checks passed

LTA-Thinking deleted the personal/rojo/fix-search-parameter-initialization branch

April 25, 2024 18:45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure API for FHIR Bug