Validate that EnableCrossEntityTransactions is configured when SendsAtomicWithReceive is enabled

[andreasohlund]

This makes sure that the ServiceBusClient is configured correctly to support cross entity transactions.

Note that this will only catch issues when our attribute is used (which should be most of the use cases. We could add a runtime check that validates on first invocation but I'm not sure its worth it?

[andreasohlund]

FYI @SeanFeldman

[odelljl]

Looking at this doc (it is for java - could not find the .net equivalent) - there seems to be performance implications - so this setting will need to be surfaced to the developer level. If an endpoint does not need cross entity transactions we would not want to set it, right?

https://docs.microsoft.com/en-us/java/api/com.azure.messaging.servicebus.servicebusclientbuilder.enablecrossentitytransactions?view=azure-java-stable

It may just be a documentation update - but future devs would not want to enable it when unnecessary.

[andreasohlund]

That is correct, hopefully we can set it when the AtomicSendsWithReceive property is set to true on our attribute. Will take a deeper look on Monday

…

On Saturday, January 15, 2022, Jeff Odell ***@***.***> wrote: Looking at this doc (it is for java - could not find the .net equivalent) - there seems to be performance implications - so this setting will need to be surfaced to the developer level. If an endpoint does *not* need cross entity transactions we would not want to set it, right? https://docs.microsoft.com/en-us/java/api/com.azure.messaging.servicebus. servicebusclientbuilder.enablecrossentitytransactions? view=azure-java-stable — Reply to this email directly, view it on GitHub <#402 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA6QZAJKJPWZEUK6Q2BAR3UWFZWBANCNFSM5MAXC32A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were assigned.Message ID: <Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/ ***@***.***>

[odelljl]

I'm not sure it is quite that simple - we may still want to transact when we are only using a single queue right? In that case, we don't need cross-entity but do want transactions. I'm on the road and will be back Tuesday and can confirm the setting then. Thanks for the quick responses.

That is correct, hopefully we can set it when the AtomicSendsWithReceive property is set to true on our attribute. Will take a deeper look on Monday
…
On Saturday, January 15, 2022, Jeff Odell @.> wrote: Looking at this doc (it is for java - could not find the .net equivalent) - there seems to be performance implications - so this setting will need to be surfaced to the developer level. If an endpoint does not need cross entity transactions we would not want to set it, right? https://docs.microsoft.com/en-us/java/api/com.azure.messaging.servicebus. servicebusclientbuilder.enablecrossentitytransactions? view=azure-java-stable — Reply to this email directly, view it on GitHub <#402 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA6QZAJKJPWZEUK6Q2BAR3UWFZWBANCNFSM5MAXC32A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were assigned.Message ID: <Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/ @.>

[andreasohlund]

You always get transactions, our flag only controls if sends should be atomic with the receive operation.

In short you get either ReceiveOnly or AtomicSendsWithReceive https://docs.particular.net/transports/transactions

[andreasohlund]

This isn't strictly needed since our transport isn't receiving messages but I figured that this would be more future proof if we for some reason add some logic in the outgoing pipeline that makes decisions based on the transaction mode

[andreasohlund]

@bording like we discussed it tried to instead enable the option when we detect this. I used

 if (!serviceBusOptions.EnableCrossEntityTransactions)
{
      serviceBusOptions.EnableCrossEntityTransactions = true;
}

but that didn't work so either:

1. Its not supported to set values like this
2. There is some kind of ordering issue that causes the function host to already have read the value before we can set it here

Do you have some ideas on what else we can try or do we have to give with this approach to force the users to set it?

[bording]

Have you tried playing around with the setting in simplified azure functions project that doesn't have NSB in it, ensuring you can do something with it at the earliest point?

Other than that, only other thing that comes to mind would be to open an issue with Microsoft to ask how it's supposed to be used, and if it can't be used, then why does it have a setter defined?

[andreasohlund]

I've tried to set it directly in the functions startup using

[assembly: FunctionsStartup(typeof(Startup))]
[assembly: NServiceBusTriggerFunction("InProcess-HostV4", SendsAtomicWithReceive = true)]
public class Startup : FunctionsStartup
{
    public override void Configure(IFunctionsHostBuilder builder)
    {
        var configuration = builder.GetContext().Configuration;

        var serviceBusOptions = configuration.GetSection("AzureFunctionsJobHost:extensions:ServiceBus")
            .Get<ServiceBusOptions>();

        if (!serviceBusOptions.EnableCrossEntityTransactions)
        {
            serviceBusOptions.EnableCrossEntityTransactions = true;
        }

        builder.UseNServiceBus();
    }
}

but that still didn't work. I'll try to raise something with MS about this but for now, I think we'll have to go with the current approach to throw an exception asking the customer to set the value to true

[andreasohlund]

Raised Particular/docs.particular.net#5613 to document the current config changes neeed

[danielmarbach]

Doesn't this hugely negatively affect the function startup time? Wouldn't it be better to rethink our current approach of hosting endpoints within functions to avoid having to scan assemblies?

[andreasohlund]

I agree that the design needs a rethink but we are already doing the reflection for most user already since we pick the endpoint name from the attribute here https://github.com/Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/blob/master/src/NServiceBus.AzureFunctions.InProcess.ServiceBus/FunctionsHostBuilderExtensions.cs#L24

Perhaps we can make this more efficient somehow?

[danielmarbach]

So by looking at the code you posted, there is an assumption this attribute is placed into the calling assembly. So, why don't we try to get that attribute for now in all those extensions once and then piggyback the information from there into the configuration? Is there a legacy that would prevent us from doing that?

[danielmarbach]

Is there a code first API that would allow not to go over the specific configuration section that could backfire on us here?

[andreasohlund]

I couldn't find anything when I did the research and GetSection only accepts a string https://docs.microsoft.com/en-us/dotnet/api/system.configuration.configurationmanager.getsection?view=dotnet-plat-ext-6.0

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.