[PM-17562] Refactor event integration methods / declarations in ServiceCollectionExtensions

[brant-livefront]

🎟️ Tracking

PM-17562

📔 Objective

On a previous PR, @withinfocus pointed me to Adopt TryAdd Dependency Injection Overloads. I wanted to circle back and take a look at adopting more TryAdd methods in the setup of event integrations. I also wanted to clean up some of the methods and try consolidate and clarify setup.

To that end, it's probably not helpful to look at these changes in the diff viewer on GitHub. I've moved all of the private methods to the bottom of the file and rearranged some other pieces, so it will likely confuse the diff view. If you view it as the raw file, hopefully you agree it's more readable now.

I also updated our README documentation to reflect the change of where to put supporting service (like SlackService HttpClient) declarations. Having these in one place should be an improvement (no need to define things like cache twice or introduce bugs with slightly different declarations).

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[withinfocus]

Definitely hard to read in a code review. Tests are passing but we'll have to see how this does once deployed just to ensure no runtime issues.

[github-actions]

Checkmarx One – Scan Summary & Details – 4ab213fd-f10e-469a-a76f-a1cc38d02171

Great job! No new security vulnerabilities introduced in this pull request

[justindbaur]

Definitely a big improvement, just some thoughts on how we can maybe solve the IHostedService problem but I am also not right up against this code so very sorry if it sends you into a wild goose chase.

To me, it seems like the problem is that you want to send in some level of dynamic primitives into the constructors of AzureServiceBusEventListenerService and AzureServiceBusIntegrationListenerService. To fix that we could instead employ the named options pattern first to limit the amount of dynamic primitives needed to just a single string. The constructor of AzureServiceBusIntegrationListenerService would change to:

public class ServiceBusIntegrationListenerOptions
{
    public int MaxRetries { get; set; }
    public string TopicName { get; set; }
    public string SubscriptionName { get; set; }
}

public AzureServiceBusIntegrationListenerService<T> : BackgroundService
    where T : IIntegrationTypeAccessor
{

    public AzureServiceBusIntegrationListenerService(IIntegrationHandler handler,
        IOptionsMonitor<ServiceBusIntegrationListenerOptions> options,
        IAzureServiceBusService serviceBusService,
        ILogger<AzureServiceBusIntegrationListenerService> logger)
    {
        _handler = handler;
        _logger = logger;
        var options = options.Get(T.Type.ToString());
        _maxRetries = options.MaxRetries;
        _serviceBusService = serviceBusService;

        _processor = _serviceBusService.CreateProcessor(options.TopicName, options.SubscriptionName, new ServiceBusProcessorOptions());
    }
    // ...rest of class...
}

The new generic is going to need to look like:

public interface IIntegrationTypeAccessor
{
    static abstract IntegrationType Type { get; }
}

and will have to be implemented for each integration type, returning something unique for each one.

Now when you register the service you should be able to do something like this:

```c#
private static IServiceCollection AddAzureServiceBusIntegration<TConfig, TIntegration>(
    this IServiceCollection services,
    string eventSubscriptionName,
    string integrationSubscriptionName,
    IntegrationType integrationType,
    GlobalSettings globalSettings)
    where TConfig : class
    where TIntegration : IIntegrationTypeAccessor
{
    services.AddOptions<ServiceBusIntegrationListenerOptions>(TIntegration.Type.ToString())
        .Configure(options =>
        {
            options.SubscriptionName = eventSubscriptionName;
            // ... others ...
        });

    services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService, AzureServiceBusIntegrationListenerService<TIntegration>>();
}

Now you have a concrete type along with the hosted service to make sure it doesn't get added multiple times but you also have different versions of it per integration type still.

[codecov]

Codecov Report

❌ Patch coverage is 15.78947% with 176 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.59%. Comparing base (abfb3a2) to head (959f17c).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...SharedWeb/Utilities/ServiceCollectionExtensions.cs	5.84%	144 Missing and 1 partial ⚠️
...Data/EventIntegrations/HecListenerConfiguration.cs	0.00%	7 Missing ⚠️
...ls/Data/EventIntegrations/ListenerConfiguration.cs	0.00%	7 Missing ⚠️
...ta/EventIntegrations/SlackListenerConfiguration.cs	0.00%	7 Missing ⚠️
.../EventIntegrations/WebhookListenerConfiguration.cs	0.00%	7 Missing ⚠️
...entIntegrations/RepositoryListenerConfiguration.cs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6118      +/-   ##
==========================================
- Coverage   48.61%   48.59%   -0.02%     
==========================================
  Files        1742     1748       +6     
  Lines       77343    77363      +20     
  Branches     6914     6917       +3     
==========================================
- Hits        37600    37597       -3     
- Misses      38230    38253      +23     
  Partials     1513     1513              

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[brant-livefront]

@justindbaur Thanks for the comment on this! While I did end up on a bit of a wild goose chase, I think it turned out profitable and all of the services are now TryAdd. 👍 😄

I couldn't type the listeners directly to the integration for a couple of reasons:

• The Repository listener at the Event level doesn't map to an integration, but rather writes all events to storage.
• The listener / handler pattern was intended to decouple listeners from the handler. A listener should just know which queue to listen to and DI gives it a handler to send the events to. So making it aware of integration felt like defeating some of that decoupling.

So instead, I've build a new set of configuration classes that we can use as the types to make sure each listener is uniquely typed. This also has the virtue of getting rid of a lot of the boilerplate in the ServiceCollectionExtensions. And best of all, it means that a lot of the ASB vs. RabbitMQ code can be consolidated and cleaned up. Most of the setup is now done in one place, which makes it easier to read and less likely that we setup ASB differently than RabbitMQ.

cc @withinfocus

[withinfocus]

Suggested change

	`IntegrationListenerConfiguration`. This class provides a way of accessing the previously configured
	`IIntegrationListenerConfiguration`. This class provides a way of accessing the previously configured

Right?

[brant-livefront]

🤦 yep. Good catch 👍

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
7.8% Duplication on New Code

See analysis details on SonarQube Cloud

[justindbaur]

Looks great, thank you for trying out the pattern!

Side feedback that isn't new to this change but since you have the same class added multiple times using the generic ILogger<T> will have them under the same category. It might be a good idea to instead use a manual category name and throw the name of the integration to the end. That way when you have issues you will quickly know which one is causing the problem.

Here is a simple program showing how even different generics don't affect the name. https://dotnetfiddle.net/v5dnlt

I would recommend a name like loggerFactory.CreateLogger($"Bit.Core.Services.AzureServiceBusIntegrationListenerService.{configuration.IntegrationType}") and this can easily be done in a follow-up PR doesn't have to be done on this one.