AzureServiceBus ensure client reuse #1974

TomMalow · 2023-07-31T14:39:36Z

What this PR does / why we need it:
HealthChecks.AzureServiceBus did not reuse created clients clients when possible.
Using multiple health check to queues and subscription within the same ServiceBus would resolve in multiple clients being created. Thereby experience a long first time execution of the health check and unnecessary open connections.

Which issue(s) this PR fixes:
#1890

Special notes for your reviewer:
I moved the code over to use the shared ClientCache instead of an internal cache. The internal cache did not allow for caching across health check instances.
The HealthChecks.AzureServiceBus did not have any unit test before. I have added for most of the health check except EventBus health check as it was not modified by this PR. This is due to the EventBus health check not face the same problem.

Does this PR introduce a user-facing change?:
Yes:

New constructors are added to all ServiceBus Health Checks to allow for injecting customServiceBus client . Mainly for testing purpose.
A previous Prefix field as been made obsolete and superseded by the existing ConnectionKey

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Unit tests passing
End-to-end tests passing
Extended the documentation - No prior documentation
Provided sample for the feature - No prior samples

…g/endpoint ConnectionKey should point to a service bus in order to reuse clients instead of pointing to a unique queue or subscription Added additional queue/subscription key for reusing reciever clients

…thCheck

TomMalow · 2023-07-31T14:44:11Z

@dotnet-policy-service agree company="Delegate A/S"

…entCache" This reverts commit 60be56e.

...lthChecks.AzureServiceBus/DependencyInjection/AzureServiceBusHealthCheckBuilderExtensions.cs

src/HealthChecks.AzureServiceBus/AzureServiceBusHealthCheck.cs

Previous commit introduced a new ServiceBusClientProvider which was added to all ServiceBus Health Check constructors. But this would introduce a breaking change. This commit would re-add constructors with 1 argument to allow for backward compatability.

…ctly instead

src/HealthChecks.AzureServiceBus/AzureServiceBusHealthCheck.cs

test/HealthChecks.AzureServiceBus.Tests/HealthChecks.AzureServiceBus.approved.txt

…iceBus.approved.txt

src/HealthChecks.AzureServiceBus/AzureServiceBusQueueHealthCheck.cs

src/HealthChecks.AzureServiceBus/AzureServiceBusSubscriptionHealthCheck.cs

...lthChecks.AzureServiceBus.Tests/AzureServiceBusQueueMessageCountThresholdHealthCheckTests.cs

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusSubscriptionHealthCheckTests.cs

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusTopicHealthCheckTests.cs

sungam3r · 2023-08-02T06:23:46Z

@TomMalow I'm fine to merge after fixing all copy-paste issues. Thank you very much for exhaustive tests.

also collect all values used to define a queue key together Change to field to avoid recalculation of the value each time

TomMalow · 2023-08-03T07:29:17Z

@sungam3r It should be ready now. Found some other similar mistakes that have been correct.
Thanks for being so thorough!

sungam3r · 2023-08-06T06:05:41Z

Please bump <HealthCheckAzureServiceBus>7.0.0</HealthCheckAzureServiceBus>.

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusQueueHealthCheckTests.cs

…althCheckTests.cs

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusQueueHealthCheckTests.cs

…althCheckTests.cs

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusQueueHealthCheckTests.cs

…althCheckTests.cs

sungam3r · 2023-08-06T06:12:33Z

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusQueueHealthCheckTests.cs

+            FullyQualifiedNamespace = FullyQualifiedName,
+            Credential = _tokenCredential,
+        };
+        var otherHealthCheck = new AzureServiceBusQueueHealthCheck(options, _clientProvider);


Suggested change

var otherHealthCheck = new AzureServiceBusQueueHealthCheck(options, _clientProvider);

var otherHealthCheck = new AzureServiceBusQueueHealthCheck(otherOptions, _clientProvider);

sungam3r · 2023-08-06T06:12:47Z

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusQueueHealthCheckTests.cs

+        var otherHealthCheck = new AzureServiceBusQueueHealthCheck(options, _clientProvider);
+        var otherContext = new HealthCheckContext
+        {
+            Registration = new HealthCheckRegistration(HEALTH_CHECK_NAME, healthCheck, HealthStatus.Unhealthy, null)


Suggested change

Registration = new HealthCheckRegistration(HEALTH_CHECK_NAME, healthCheck, HealthStatus.Unhealthy, null)

Registration = new HealthCheckRegistration(HEALTH_CHECK_NAME, otherHealthCheck, HealthStatus.Unhealthy, null)

sungam3r · 2023-08-06T06:13:16Z

test/HealthChecks.AzureServiceBus.Tests/AzureServiceBusQueueHealthCheckTests.cs

+            Registration = new HealthCheckRegistration(HEALTH_CHECK_NAME, healthCheck, HealthStatus.Unhealthy, null)
+        };
+
+        var otherActual = await healthCheck


Suggested change

var otherActual = await healthCheck

var otherActual = await otherHealthCheck

Please, look through test files one more time...

Some test was setup incorrectly but were not caught by in any assertion. Added som additional assert to ensure that healthchecks were executing as intended

HealthCheck was always called after setting it up in helper methods so to reduce boiler plate the call to health check is moved intothe helper method. Only a few had a substitute setup inbetween, but these could be moved to earlier in the respective tests

TomMalow · 2023-08-10T06:34:08Z

Got tired of embarrassing myself, so ended up refactoring the tests to reduce the potential for me making, or overseeing, any mistakes. The test now has reduced boilerplate around setting up and executing the HealthChecks. This also resulted in the tests being much easier to read.

sungam3r · 2023-08-15T08:01:47Z

@TomMalow I believe everything is OK now though I did not deep in each line this time.

sungam3r · 2023-08-15T08:04:33Z

test/HealthChecks.AzureServiceBus.Tests/HealthChecks.AzureServiceBus.approved.txt

@@ -8,40 +8,46 @@ namespace HealthChecks.AzureServiceBus
    public abstract class AzureServiceBusHealthCheck<TOptions>
        where TOptions : HealthChecks.AzureServiceBus.Configuration.AzureServiceBusHealthCheckOptions
    {
-        protected static readonly System.Collections.Concurrent.ConcurrentDictionary<string, Azure.Messaging.ServiceBus.ServiceBusClient> ClientConnections;


Techically breaking changes, but I think v7.1 is OK.

codecov-commenter · 2024-09-13T22:52:29Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.24390% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.45%. Comparing base (4259ef3) to head (ea6e21c).
Report is 99 commits behind head on master.

Files with missing lines	Patch %	Lines
...Checks.AzureServiceBus/ServiceBusClientProvider.cs	0.00%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1974      +/-   ##
==========================================
+ Coverage   66.35%   67.45%   +1.09%     
==========================================
  Files         251      252       +1     
  Lines        8692     8729      +37     
  Branches      622      624       +2     
==========================================
+ Hits         5768     5888     +120     
+ Misses       2762     2677      -85     
- Partials      162      164       +2

Flag	Coverage Δ
AzureServiceBus	`72.56% <90.24%> (+8.76%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TomMalow and others added 8 commits July 31, 2023 16:23

chor: add Microsoft.Bcl.AsyncInterfaces to be able to use ClientCache

60be56e

refactor: use ClientCatch instead of internal concurrent dictionaries

af2c725

refactor: Change ConnectionKey to point to ServiceBus connectionStrin…

6bcbd79

…g/endpoint ConnectionKey should point to a service bus in order to reuse clients instead of pointing to a unique queue or subscription Added additional queue/subscription key for reusing reciever clients

feat: add Substitutable ServiceBusClientProvider to allow unit testing

b012102

feat: add unit test for AzureServiceBusQueueHealthCheck

c36ff09

feat: add unit test for AzureServiceBusSubscriptionHealthCheck

1ca452a

feat: add unit test for AzureServiceBusTopicHealthCheck

66a9864

feat: add unit test for AzureServiceBusQueueMessageCountThresholdHeal…

bed2127

…thCheck

github-actions bot added test azure labels Jul 31, 2023

TomMalow added 2 commits July 31, 2023 17:15

Revert "chor: add Microsoft.Bcl.AsyncInterfaces to be able to use Cli…

2b4f6ce

…entCache" This reverts commit 60be56e.

refactor: fix format

c57e94c

sungam3r reviewed Jul 31, 2023

View reviewed changes

...lthChecks.AzureServiceBus/DependencyInjection/AzureServiceBusHealthCheckBuilderExtensions.cs Outdated Show resolved Hide resolved

sungam3r reviewed Jul 31, 2023

View reviewed changes

src/HealthChecks.AzureServiceBus/AzureServiceBusHealthCheck.cs Outdated Show resolved Hide resolved

TomMalow added 2 commits August 1, 2023 08:20

refactor!: Change Prefix field to obsolete and use ConnectionKey dire…

f96d7cf

…ctly instead