Revive PR - Extend Service Bus with Active and Deadletter-Queue thresholds

[cieciurm]

What this PR does / why we need it:
This PR revives #822

• Fixed build
• Fixed unit tests (and moved to specific AzureServiceBus project)
• Fixed registration

Which issue(s) this PR fixes:

Please reference the issue this PR will close: #821

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
No.

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
• Provided sample for the feature

[cieciurm]

Hi,
Thanks for your feedback.
Changes applied.

cc @sungam3r

[cieciurm]

Thanks @sungam3r!

I have merged into a single health check - the only down-side is I changed from struct to class because I wanted to use Guard clause.

Let me know what do you think now :)

Also I'm wondering if we want to add registration method which supports two Func to configure both active and dead letter messages

[sungam3r]

from struct to class because I wanted to use Guard clause.

😕 For struct Guard clause is useless since structs always non-null, but I'm fine either class or struct.

[cieciurm]

Thanks for your review @sungam3r 😄

I decided to stick with struct - let me know what do you think now 🕵🏻
I have also fixed unit tests.

Speaking of IHealthChecksBuilder extensions currently we have 4:

• 2 for Active Messages (authentication via connection string and endpoint/TokenCredential)
• 2 for Dead Letter Messages (authentication via connection string and endpoint/TokenCredential)

I'm wondering if I should add 2 more:

• 2 for Active and Dead Letter Messages (authentication via connection string and endpoint/TokenCredential)

Or not add it at all.
Then in theory, client cannot specify to validate both active and dead letters.
But client could specify 2 separate health checks - so maybe it's fine.

[sungam3r]

When HC provides options object client can specify any configuration in action delegate so no need to provide 100500 extension methods from our side. Imagine you have options object with 10 optional properties inside. Would you provide all combinations for setting those properties? Obviously no.

[sungam3r]

A lot of extensions were added historically before introducing options pattern. I personally dislike those bunch overloads but I did not remove already written ones. For new health checks I do not bother with many extension methods leaning to provide only required minimum - one with Action<BlaBlaOptions>. Anyway - eventually the number grows because of requirements to access to things from DI, so new overloads appear - with IServiceProvider, etc.

[codecov-commenter]

Codecov Report

Merging #1861 (e9f5999) into master (c5b71a4) will decrease coverage by 0.21%.
The diff coverage is 49.39%.

❗ Current head e9f5999 differs from pull request most recent head 19fe25e. Consider uploading reports for the commit 19fe25e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
- Coverage   68.44%   68.24%   -0.21%     
==========================================
  Files         214      217       +3     
  Lines        7762     7845      +83     
  Branches      530      539       +9     
==========================================
+ Hits         5313     5354      +41     
- Misses       2309     2349      +40     
- Partials      140      142       +2     

Flag	Coverage Δ
AzureServiceBus	55.27% <49.39%> (-0.88%)	⬇️

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

Impacted Files	Coverage Δ
...tion/AzureServiceBusQueueMessagesCountThreshold.cs	0.00% <0.00%> (ø)
...ServiceBusQueueMessageCountThresholdHealthCheck.cs	14.28% <14.28%> (ø)
...ion/AzureServiceBusHealthCheckBuilderExtensions.cs	97.50% <93.75%> (-0.49%)	⬇️
...usQueueMessagesCountThresholdHealthCheckOptions.cs	100.00% <100.00%> (ø)

[cieciurm]

Thanks for the explanation, @sungam3r.

I got fixated to pass Action<Threshold> instead of Action<Options> to the extension method, and that is why I thought that I need extension method per threshold type.

Now, as you mentioned, it is as free/open to use as possible, because we expose Action<Options>.

There are two extension methods in total because there are two ways to authenticate:

• using Connection String
• using Endpoint and Token Credential

Tests are fixed as well :)

[sungam3r]

There are two extension methods in total because there are two ways to authenticate

OK, I'm fine to provide 2, but again - these auth combinations are in options. What if you have 5 ways of auth? Would you provide 5 extension methods to connect your HC? What prompted you to choose a method of authentication as a criterion for the number of overloads? All these settings are optional and can generate combinations, especially with further evolution. Subsequently, this can create a design problem when the previously chosen criterion will no longer be so obvious and will have to change the public API.

[sungam3r]

In other words string queueName is good parameter for extension method because it's a required one to build AzureServiceBusQueueMessagesCountThresholdHealthCheckOptions (and then pass it to delegate back to the caller). All other parameters are not so good to enumerate in extension methods.

[sungam3r]

@cieciurm I'll wait for your decision on the number of overloads until tomorrow.

[cieciurm]

I am no expert, but I think my pattern of thinking, as well as original PR author's, were inspired by the shape of existing extension methods for Queues.

If you see the current overloads of AddAzureServicBusQueue extension methods, they are embracing two different ways of authentication. The auth parameters are passed explicitly as parameters, and not thru Action<Options>.

I would rely on your expertise and experience in shaping the public API of a library - if you think it is just causing noise, then let's remove it and have single method.

Just let me know what you think and I will adjust the PR tomorrow morning :)

[sungam3r]

I merge as is. I still think that the library (almost each HC project) has more extensions to add health checks than necessary, but now is not the time to change the design removing that "noise".