Allow transport implementation to lookup publisher(s) for a given eventtype

[yvesgoeleven]

The new core has static and dynamic rules that allow to specify how to subscribe to events, in addition to message endpoint mappings from the past.

Example from a test:

var publisher = new EndpointName("DistributingAnEvent.Publisher");
c.Pubishers().AddStatic(publisher, typeof(MyEvent));
c.Pubishers().AddDynamic(rule);
c.Routing().EndpointInstances.AddStatic(publisher, new EndpointInstance(publisher, null, null));

This routing information about publishers is however internal to the core and not passed to the transports.

In order to support existing customers on the ASB transport, we would need to have access to the publisher information for a given eventtype when (un)subscribing their endpoints. Please provide an API that allows us to look up the logicaladdress(es?) for a given event type.

I noticed there is a Publishers instance in the settings, with no public members, that might be a good place to expose this API?

/cc @Particular/nservicebus

[yvesgoeleven]

Looks like https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core/Routing/MessageDrivenSubscriptions/SubscriptionRouter.cs#L19 has the lookup capability that we need

[yvesgoeleven]

I tested to see if SubscriptionRouter has the correct functionality, and it seems to do, all tests green now!.

As all of it's parts were also available in settings, I managed to gobble the object together in the following way:

var publishers = settings.Get<Publishers>();
var endpointInstances = settings.Get<EndpointInstances>();
var transportAddresses = settings.Get<TransportAddresses>();

var subscriptionRouterType = Type.GetType("NServiceBus.SubscriptionRouter, NServiceBus.Core");
 var subscriptionRouter = Activator.CreateInstance(subscriptionRouterType, new object[]
 {
                publishers,
                endpointInstances,
                transportAddresses
});
var getAddressesMethod = subscriptionRouterType.GetMethod("GetAddressesForEventType",BindingFlags.Instance | BindingFlags.Public);
var addresses = ((Task<IEnumerable<string>>) getAddressesMethod.Invoke(subscriptionRouter, new object[]
 {
                eventType
 })).GetAwaiter().GetResult();

[udidahan]

So, is this a bug or a feature?

[yvesgoeleven]

@udidahan good question, I couldn't make up my mind. It's a new feature from core perspective, but lack of this information leads to bugs at the transport end.

[udidahan]

To me it sounds like a refactoring.

[SeanFeldman]

@SzymonPobiega would the PlatformDev issue https://github.com/Particular/NServiceBus/issues/3269 address this or it's a completely different issue?

[SzymonPobiega]

@SeanFeldman I think you pasted wrong issue link

[SeanFeldman]

my bad @SzymonPobiega, please ignore

[SzymonPobiega]

@yvesgoeleven would making GetAddressesForEventType public work?

[yvesgoeleven]

@SzymonPobiega yes, that capability available publicly would allow us to just use it, but SubscriptionRouter itself is not public either and is in no way passed to the transport, so just making the method public won't be enough, some way it should also be made available.

[SzymonPobiega]

@yvesgoeleven this to me seems like a bigger discussion about the public API surface. I have no strong opinions here but I like @andreasohlund 's idea that the transport should only depend on the official transport seam (IDispatchMessages) and not fiddle with internals of NSB (e.g. via behaviors).

The list of publishers is a routing concern used for unicast routing's message-driven pub sub feature so in theory far away from the transport seam. I understand that you need this for backwards compat since the naming convention uses the endpoint name but maybe there is some other way to address the backwards compat issue without polluting the new transport implementation with internals of NSB.

[yvesgoeleven]

@SzymonPobiega I'm not intending to fiddle with nsb internals, the above is just a temporary hack to not be blocked by it. I feel routing information like this are just user settings, that should be passed into the transport via settings and could be used for decission making inside the transport (but not should be used, it really depends on the needs of the transport itself f.e. for backward compat)

[SeanFeldman]

maybe there is some other way to address the backwards compat issue without polluting the new transport implementation with internals of NSB.

What would be that @SzymonPobiega?

[SzymonPobiega]

@SeanFeldman @yvesgoeleven let me go through the routing API and try to come up with a conherent public API.

[SzymonPobiega]

OK, went through the new ASB code to understand better what ASB needs. Let me try to validate what I understand:

• ASB transport has native pub sub (no subscribe messages)
• ASB publisher creates a topic with a name that combines endpoint name and event name in some way (assume endpoint-event for the sake of simplicity)
• ASB subscriber previously used MessageEndpointMappings to figure out the topic name. This is a different behavior from the other native pub/sub transport (Rabbit) which ignores the MEM with regards to subscriptions

Now the question is: would it be enough if the implementation of IManageSubscriptions got the name of the endpoint in the Subscribe and Unsubscribe calls? This is the smallest possible API surface area we can have to convey that info. Question is, if that would not be too late. My rationale (from point of view of core) is try to keep tell, don't ask principle in the transport seam. I'd rather expose what's necessary when core calls to the transport and not have transport querying for stuff.

[SeanFeldman]

@SzymonPobiega you're correct about your understanding. One comment, endpoint name (event publisher) is only needed for one of the topologies to maintain backwards compatibility.

If IManageSubscriptions is given an endpoint name on Subscribe/Unsubscribe call, that would work since we're building subscription entities at that point by calling TopologySectionManager.

@Particular/azure-maintainers can you poke any holes in this logic?

[SzymonPobiega]

From routing point of view it would be consistent. One might even argue that we should have been passing that info from the beginning because there is no place we say that if you use native pub-sub you don't get the information specified in the Publishers collection.

[yvesgoeleven]

@SzymonPobiega @SeanFeldman Passing in the publisher endpoint name on subscribe will work for this topology

But I disagree on the preference of adding everything explicitly to the interface. Not every transport will be the same, they may all need different info to get to the same behavior. It would be better if transports could look stuff up from well known locations (in settings) instead of asking for another breaking change of the core for a little piece of information to be added that is only relevant in some of the transports.

[SzymonPobiega]

@yvesgoeleven the problem with that is that the transport seam would not be bounded and there is now way we can test transports in isolation. I would still like to make some of the routing APIs to be public for the users to be able to replace our current routing behaviors but that is different story.

In short, I believe a transport should only use a small well-defined seam but if you write a plugin that contains behaviors, you can do much more. That plug-in can be in the same assembly but I think there is value in distinguishing these two things. The value comes from the fact that we're using transports in various contexts, not only in the context of dispatch pipeline.

[yvesgoeleven]

@SzymonPobiega I thought transports are no longer allowed to plug in behaviors

[SzymonPobiega]

@yvesgoeleven that is what I am aiming at. Pluging in behaviors seems to me similar (in terms of broadening the transport seam) as using the routing (or any other core feature) APIs.

[danielmarbach]

@Particular/transport-maintainers Any thoughts on this? Can we close it, should we remove the milestone?

[yvesgoeleven]

@danielmarbach we have implemented something in the transport, as far as I am concerned this issue can be closed (http://docs.particular.net/nservicebus/azure-service-bus/publisher-names-configuration)