Azure-sdk-for-net: [FEATURE REQ] Implement pipelines for Azure Service Bus (track 2) with support for custom plugins

Created on 11 May 2020  路  40Comments  路  Source: Azure/azure-sdk-for-net

Library or service name.
Azure.Messaging.ServiceBus

Description
The preview should contain an equivalent to the incoming/outgoing pipelines with support for custom plugins, similar to what Track 1 had. The pipelines allow Open-Close Principle by exposing customization points for incoming and outgoing messages w/o the need to change the SDK.

Client Service Bus customer-reported feature-request needs-team-attention

All 40 comments

Will investigate this as part of July sprint which will be focusing on Preview 4.

Sounds good, @JoshLove-msft.
I've raised the issue as Preview 3 had "Finalize API changes" and this change could cause some APIs to shift.

Yeah we would like to finalize what we have so far at least. We can also do a quick investigation to make sure that plugins wouldn't drastically change things from Preview 3.

I'd vote for a quick investigation to remove any major concerns. Started thinking about potential woes it could cause when looking at the ServiceBusReceivedMessage that would need to be modified when is going through an incoming pipeline.

Good point regarding the incoming pipeline and ServiceBusReceivedMessage. I think what we could do is have both the BeforeMessageSend and AfterMessageReceive methods work with a ServiceBusMessage. This would provide users that opt in to plugins with a means of modifying the received message before it comes back in its final ServiceBusReceivedMessage form.

I think what we could do is have both the BeforeMessageSend and AfterMessageReceive methods work with a ServiceBusMessage. This would provide users that opt in to plugins with a means of modifying the received message before it comes back in its final ServiceBusReceivedMessage form.

In theory that should work. It will be awkward having ServiceBusMessage for the outgoing messages and ServiceBusReceivedMessage for the incoming messages on the SDK level but use ServiceBusMessage for both the incoming and the outgoing messages when working with a pipeline.

The other potential concern is having no access to the properties assigned by the broker upon message receive at the AMQP level (DeliveryCount, LockedUntil, LockToken, etc) that plugins otherwise could access for their logic.

Yeah, we would need to make those system properties available in the method. Another idea would be to pass the ServiceBusReceivedMessage but to expose set methods that live on the Plugin class.
/cc @jsquire as he was thinking about similar things regarding AMQP message formatting.

Another idea would be to pass the ServiceBusReceivedMessage but to expose set methods that live on the Plugin class.

Wouldn't those be getters/get methods more than anything else? Custom plugins usually require broker information to act but not change what only the broker can assign. At least in my experience.

As for the proposal, somewhat awkward as it will be the limitation of internals worked around by using a base class that has access to those members. But that would be a possibility, yes.

Another idea would be to pass the ServiceBusReceivedMessage but to expose set methods that live on the Plugin class.

Wouldn't those be getters/get methods more than anything else? Custom plugins usually require broker information to act but not change what only the broker can assign. At least in my experience.

As for the proposal, somewhat awkward as it will be the limitation of internals worked around by using a base class that has access to those members. But that would be a possibility, yes.

Yes we would only expose as settable the properties that should be settable by the user. Broker properties would not be settable.

Because Josh alluded to it earlier, in the larger context, I think we're looking at two different features with distinct responsibilities. To sketch out my thoughts in a rough manner:

Plugin _(application-level)_

  • Would come in two flavors: those interested in outgoing messages and those interested in incoming messages
  • Would exist in an ordered set; for an operation, the first plugin in the sequence is given access to a message and may choose to modify it. The same message (with any modifications) would be passed to the next plugin and so-on. Once the plugins have completed, the remainder of the Service Bus operation would take place with the "final" version of the message.

  • Sender plugins would be given access to a ServiceBusMessage and have the ability to work against that message before it was sent. Any field on the ServiceBusMessage that is mutable could be adjusted by the plugin; those that are immutable would not be able to be altered.

  • Receiver plugins would be given access to a ReceivedServiceBusMessage and have the ability to work against that message before it was returned to the calling application. Any field on the ReceivedServiceBusMessage that is mutable could be adjusted by the plugin; those that are immutable would not be able to be altered.

Message Formatter _(protocol-level)_

  • The message formatter would be part of the service communication pipeline, intended to influence how a message from the Service Bus library is represented in the underlying protocol. For example, which of the AMQP body fields holds the payload.
  • Registered as a single instance for a given client and which is expected to be compatible with the ServiceBusTransportType used for the connection.

  • Is presented with a low-level representation of the message that represents the data in a manner that better reflects its network transport but does not leak concrete types from any protocol library directly. _(this is still in early design; opinions and suggestions on what that looks like are welcome)_

  • Has the ability to modify any data associated with the message, including items owned by the broker which would otherwise be immutable.

  • For an outgoing message, the transformer serves as the last step for preparing a message for transport. After the formatter, the data is converted to the final format for the protocol and sent to the service.

  • For an incoming message, the transformer serves as the first step for preparing data received from the broker. After the formatter, the data is converted into a ReceivedServiceBusMessage.

@jsquire, looks good with a few points:

Would come in two flavors: those interested in outgoing messages and those interested in incoming messages

Plugins can also be interested in both. For example, the attachment plugin intercepts both, incoming and outgoing messages to achieve what it needs to accomplish. A plugin is a logical unit. Which can opt into registering for incoming and/or outgoing message pipelines.

Receiver plugins would be given access to a ReceivedServiceBusMessage and have the ability to work against that message before it was returned to the calling application. Any field on the ReceivedServiceBusMessage that is mutable could be adjusted by the plugin; those that are immutable would not be able to be altered.

Plugins in the incoming pipeline should be able to mutate the body and custom properties of the incoming message.

Plugins can also be interested in both. For example, the attachment plugin intercepts both, incoming and outgoing messages to achieve what it needs to accomplish. A plugin is a logical unit. Which can opt into registering for incoming and/or outgoing message pipelines.

Fair enough; it wasn't my intent to limit them to one-or-the-other exclusively, just to highlight the separate cases.

Plugins in the incoming pipeline should be able to mutate the body and custom properties of the incoming message.

Agreed, and apologies. I overlooked ServiceBusReceivedMessage being fully immutable when making that statement. As you and Josh have been discussing, we'll have to figure a direction that allows for things not explicitly broker-owned System Properties to be mutable for the pipeline. I don't see a way forward without trade-offs; personally, I'd like to consider a subclass similar to:

public class WritableServiceBusReceivedMessage : ServiceBusReceivedMessage
{
    public new ReadOnlyMemory<byte> Body
    {
        get => base.SentMessage.Body;
        set => base.SentMessage.Body = value;
    }

    // ... and so on, shadowing properties that should be writable.
}

I see the trade-off here as some duplication and the need to keep this class up to date should there be new properties added to the base that we want to shadow. It also does introduce a new type that only plugins would see, but the shape mirrors the ServiceBusReceivedMessage, so it would be at least familiar.

Great, so we are aligned in general.

@JoshLove-msft had an idea of exposing ServiceBusReceivedMessage immutable properties for mutation via plugin methods. That way WritableServiceBusReceivedMessage or any other publicly visible types for the workaround don't have to be created/exposed.

```c#
public abstract class ServiceBusPlugin
{
public abstract async Task ProcessIncomingMessage(ServiceBusReceivedMessage message);
//...

public void SetBody(ServiceBusReceivedMessage message, ReadOnlyMemory body)
{
message.Body = body;
}

public void SetHeader(ServiceBusReceivedMessage message, string key, object value)
{
message.Properties[key] = value;
}
//...
}

That could eliminate any need in additional types.

```c#
public class CustomPlugin : ServiceBusPlugin
{
  override async Task ProcessIncomingMessage(ServiceBusReceivedMessage message)
  {
    var newBody = SomeOperation(...);
    base.SetBody(message, newBody);

    base.SetHeader(message, "header", "value");
  }
}

Great, so we are aligned in general.

@JoshLove-msft had an idea of exposing ServiceBusReceivedMessage immutable properties for mutation via plugin methods. That way WritableServiceBusReceivedMessage or any other publicly visible types for the workaround don't have to be created/exposed.

public abstract class ServiceBusPlugin
{
  public abstract async Task ProcessIncomingMessage(ServiceBusReceivedMessage message);
  //...

  public void SetBody(ServiceBusReceivedMessage message, ReadOnlyMemory<byte> body)
  {
    message.Body = body;
  }

  public void SetHeader(ServiceBusReceivedMessage message, string key, object value)
  {
    message.Properties[key] = value;
  }
  //...
}

That could eliminate any need in additional types.

public class CustomPlugin : ServiceBusPlugin
{
  override async Task ProcessIncomingMessage(ServiceBusReceivedMessage message)
  {
    var newBody = SomeOperation(...);
    base.SetBody(message, newBody);

    base.SetHeader(message, "header", "value");
  }
}

Would we still need to expose other properties as settable, such as MessageId, Label, etc. These are settable on the ServiceBusMessage. I was thinking we could even have something like:
c# public void SetProperty(ServiceBusReceivedMessage message, string propertyName, object value) { }
Otherwise, we could just expose Set methods for each of the other properties.

I do generally prefer doing this in the plugin as we avoid adding a new message type.

I'm not opposed to Josh's suggestion - it's a good option. It feels a bit less natural to me than manipulating an object directly, but that may not be a bad thing, as it will make clear that you're about to modify an otherwise immutable instance.

I'm not a fan of the generalized SetProperty method for a couple of reasons:

  • You'd have to implement via reflection to do it directly, which would potentially bottleneck throughput. You could do some setup work to build a dictionary of strongly-typed delegates for each property, which may help a bit, but we'd still take a perf hit.
  • There's no clear indication for callers what properties are settable and which are off-limits due to being owned by the broker. Usability concerns aside, you'd have to either track an allow list or a deny list for member names, so you're back to trying to keep things in-sync. We could offset that last part by decorating settable properties with an attribute and then discovering them.

Assuming that we go down the explicit property route, I think we're looking at the same general trade-off for maintenance cost that we would be with a specialized type. We're trading a little bit of discoverability for reducing the complexity for tracking another type. Given that plugins aren't something that would be on the "Hello World" path, I'd be good with that.

I'm not a fan of the generalized SetProperty method for a couple of reasons:

Second that. And I'll add also this - Service Bus message properties are a small set and do not change that often, if at all. Avoiding reflection and having explicit methods would be my preference as well.

Any updates on this front?

No updates at present. The current milestone closes out this week and planning for the next isn't final yet, but plugins seem to be tentatively on the schedule. _(ref: #12384)_

Yes, we definitely want to get this in for Preview 4.

I'm looking at the Track 1 API, and I noticed that BeforeMessageSend and AfterMessageReceive return Task<Message>. I'm wondering if we can get away with just returning a Task so that it is clear the plugin should be mutating the passed in instance, rather than potentially creating a new instance. Do we know of use cases where creating a new instance is needed?
@nemakam @SeanFeldman

Also interested in what everyone's thoughts are on splitting into separate Sender/Receiver plugins to align with the rest of the SDK. The only potential issue I can think of would arise if someone is trying to use the same instance for both a sender and a receiver in order to maintain state in the plugin. This seems like an odd use case as state would be more appropriately set in the message properties rather than in the plugin.

I'm looking at the Track 1 API, and I noticed that BeforeMessageSend and AfterMessageReceive return Task<Message>. I'm wondering if we can get away with just returning a Task so that it is clear the plugin should be mutating the passed in instance, rather than potentially creating a new instance. Do we know of use cases where creating a new instance is needed?

Plugins always operate on the message passed in. Wherever they will replace the entire Message, parts of it, or nothing, depends on the plugin itself. I can't see an issue with the return just a Task _if_ the message can be modified or entirely replaced.

Also interested in what everyone's thoughts are on splitting into separate Sender/Receiver plugins to align with the rest of the SDK. The only potential issue I can think of would arise if someone is trying to use the same instance for both a sender and a receiver in order to maintain state in the plugin. This seems like an odd use case as state would be more appropriately set in the message properties rather than in the plugin.

I will strongly advocate against it. Often times a plugin operates on both directions. You'll be forcing both the authors of the plugin and the users to register both. Which is counterproductive. Take encryption as an example. The logic to encrypt and decrypt belongs to the same plugin. Configuration for the two is shared. Two different plugins add unnecessary complications. I have stated multiple types that coherence with the HTTP based SDKs is not always something to strive for when working with this SDK. Plugins themselves don't have a state, but they often logically connected operations: encrypt/decrypt, compress/decompress, etc. Splitting that logic for the sake of consistency with the other SDKs is a wrong move. If the pipeline model used for the rest of the HTTP based SDKs could be used for ASB that'd be a different discussion. But it can't. Therefore forcing the same model, unless it can be proven as beneficial for users and plugin authors, is not beneficial.

Looks like that was at least five bucks and not just 0.02 馃槂

I'm looking at the Track 1 API, and I noticed that BeforeMessageSend and AfterMessageReceive return Task<Message>. I'm wondering if we can get away with just returning a Task so that it is clear the plugin should be mutating the passed in instance, rather than potentially creating a new instance. Do we know of use cases where creating a new instance is needed?

Plugins always operate on the message passed in. Wherever they will replace the entire Message, parts of it, or nothing, depends on the plugin itself. I can't see an issue with the return just a Task _if_ the message can be modified or entirely replaced.

Also interested in what everyone's thoughts are on splitting into separate Sender/Receiver plugins to align with the rest of the SDK. The only potential issue I can think of would arise if someone is trying to use the same instance for both a sender and a receiver in order to maintain state in the plugin. This seems like an odd use case as state would be more appropriately set in the message properties rather than in the plugin.

I will strongly advocate against it. Often times a plugin operates on both directions. You'll be forcing both the authors of the plugin and the users to register both. Which is counterproductive. Take encryption as an example. The logic to encrypt and decrypt belongs to the same plugin. Configuration for the two is shared. Two different plugins add unnecessary complications. I have stated multiple types that coherence with the HTTP based SDKs is not always something to strive for when working with this SDK. Plugins themselves don't have a state, but they often logically connected operations: encrypt/decrypt, compress/decompress, etc. Splitting that logic for the sake of consistency with the other SDKs is a wrong move. If the pipeline model used for the rest of the HTTP based SDKs could be used for ASB that'd be a different discussion. But it can't. Therefore forcing the same model, unless it can be proven as beneficial for users and plugin authors, is not beneficial.

Looks like that was at least five bucks and not just 0.02 馃槂

I was actually talking about consistency with the rest of the Service Bus SDK for Track 2. We have things split along sender/receiver lines so I was thinking of following suit here. In Track 1, even though the send and receive methods are both on the same plugin, you would need to register both your sender and receiver with this plugin. I suppose on the QueueClient this wouldn't be the case as this can send and receive. But in Track 2, we have no such type that is capable of both sending and receiving. We could allow the plugin to be specified in the ServiceBusClient, but this seems like it might be too high level, as it encapsulates the entire connection as opposed to a particular queue. Otherwise, we would face the scenario where users would need to register a plugin that has both send/receive methods on both the sender and the receiver, which raises the obvious question - why do we allow specifying receive plugin behavior on a sender, and vice versa.

Plugins always operate on the message passed in. Wherever they will replace the entire Message, parts of it, or nothing, depends on the plugin itself. I can't see an issue with the return just a Task if the message can be modified or entirely replaced.

Would the instance itself need to be replaced? All properties should be able to be modified, but if the instance needs to be replaced we would still need to return a ServiceBusMessage (we can't pass ref params in async method).

Would the instance itself need to be replaced? All properties should be able to be modified, but if the instance needs to be replaced we would still need to return a ServiceBusMessage (we can't pass ref params in async method).

I never had to replace the instance itself. Properties and body, yes.

We could allow the plugin to be specified in the ServiceBusClient, but this seems like it might be too high level, as it encapsulates the entire connection as opposed to a particular queue.

And why do you make an assumption it would be too high of a level?
Let's say the plugin is an audit plugin. You'd want to audit all the entities. If not, the plugin would take a configuration object to let filter out/allow only specific entities. It's not the SDK concern.
Let's take the attachment plugin. Customers use on all the entities as it's a namespace limitation, not an entity. Same with the compression plugin.

Plugins that are asymmetric, will pass through on the method they do not need.

.... I'm wondering if we can get away with just returning a Task so that it is clear the plugin should be mutating the passed in instance, rather than potentially creating a new instance. Do we know of use cases where creating a new instance is needed?

I agree with the sentiment, but you'd have to change the parameter to ref in order to allow the plugin to replace the message entirely rather than just manipulate it's properties. In this case, I think it is a good expression of the intent and makes the semantics clear. The only concern is that it's not an oft used modifier, in my experience, and may not be well understood. We probably want to run it by the language architect, but I'm in favor.

Also interested in what everyone's thoughts are on splitting into separate Sender/Receiver plugins to align with the rest of the SDK. The only potential issue I can think of would arise if someone is trying to use the same instance for both a sender and a receiver in order to maintain state in the plugin. This seems like an odd use case as state would be more appropriately set in the message properties rather than in the plugin.

I'm with Sean on this one. I think having both directions allows for a plugin to better represent a single concern. For example, a logging plugin that would observe messages and send information to a monitoring system. Regardless of incoming/outgoing direction, the logging of messages is a single area of concern. It would seem strange and arbitrary to have to break that into two types and would potentially add complexity for the author to factor out shared code.

I'm not sure that I understand the underlying rationale, but if we think there's a case to designate a plugin for a single incoming/outgoing direction, this may be a case to discuss having an interface representation so that a single plugin could simply implement both interfaces. Granted, there are good reasons to avoid that as well, so we'd definitely want to be sure that we have a solid case for the split before going down that path.

Also interested in what everyone's thoughts are on splitting into separate Sender/Receiver plugins to align with the rest of the SDK. The only potential issue I can think of would arise if someone is trying to use the same instance for both a sender and a receiver in order to maintain state in the plugin. This seems like an odd use case as state would be more appropriately set in the message properties rather than in the plugin.

I'm with Sean on this one. I think having both directions allows for a plugin to better represent a single concern. For example, a logging plugin that would observe messages and send information to a monitoring system. Regardless of incoming/outgoing direction, the logging of messages is a single area of concern. It would seem strange and arbitrary to have to break that into two types and would potentially add complexity for the author to factor out shared code.

I'm not sure that I understand the underlying rationale, but if we think there's a case to designate a plugin for a single incoming/outgoing direction, this may be a case to discuss having an interface representation so that a single plugin could simply implement both interfaces. Granted, there are good reasons to avoid that as well, so we'd definitely want to be sure that we have a solid case for the split before going down that path.

My main concern was the awkwardness with setting the bidirectional plugin on a sender/receiver. If we think it makes sense to have this only be settable on the ServiceBusClient, then I am in agreement.

.... I'm wondering if we can get away with just returning a Task so that it is clear the plugin should be mutating the passed in instance, rather than potentially creating a new instance. Do we know of use cases where creating a new instance is needed?

I agree with the sentiment, but you'd have to change the parameter to ref in order to allow the plugin to replace the message entirely rather than just manipulate it's properties. In this case, I think it is a good expression of the intent and makes the semantics clear. The only concern is that it's not an oft used modifier, in my experience, and may not be well understood. We probably want to run it by the language architect, but I'm in favor.

As mentioned in previous comment, we can't use ref in async method, so if replacing an instance is required, we would need to return the instance. I can't think of why this would be needed though if all properties can be updated.

@SeanFeldman what are your thoughts regarding immutability of plugins once the client is created?Track 1 allows the ability to register/unregister plugins after client creation. Is this a feature that you have seen a need for? I'm considering taking the plugins as part of the ServiceBusClientOptions, and having the list of plugins be immutable once the client is constructed.

@SeanFeldman what are your thoughts regarding immutability of plugins once the client is created?Track 1 allows the ability to register/unregister plugins after client creation. Is this a feature that you have seen a need for?

I have not seen scenarios where plugins would be modified at run-time. At least not for the use-cases I've been involved in.

I'm considering taking the plugins as part of the ServiceBusClientOptions, and having the list of plugins be immutable once the client is constructed.

I'd like to see that API pseudocode first. Would it be something like this?

```c#
var options = new ServiceBusClientOptions
{
Plugins = new List { new PluginA(...), new PluginB(...) },
// other SB client options
};

await using var client = new ServiceBusClient(connectionString, options);

If the answer is yes, this is forcing too much boilerplate code on the developers and forcing plugin creators to expose the constructors, which have to be carefully thought through to avoid breaking changes. Track 1 has a method to register plugins, `.RegisterPlugin(plugin-instance)` and was a cleaner option. I'd much rather see the API as the following:

```c#
await using var client = new ServiceBusClient(connectionString, options);
client.RegisterPlugin(plugin);

An additional point to consider is when plugins are created, authors can simplify plugins discoverability and registration by having extension methods for registration reside in the default/root namespace of the ASB client. The end-user experience then becomes

c# await using var client = new ServiceBusClient(connectionString, options); client.UseCustomPlugin(); // the plugin uses all the default settings // or client.UseCustomPlugin(new CustomPluginConfiguration());

If the answer is yes, this is forcing too much boilerplate code on the developers and forcing plugin creators to expose the constructors, which have to be carefully thought through to avoid breaking changes.

Yes it would be pretty much this, or maybe even having a nested PluginOptions type within ServiceBusClientOptions to allow for extensibility.
Don't the constructors of the plugins need to be exposed with RegisterPlugin anyway? How else do you create the plugin?
I agree that RegisterPlugin would mean less boilerplate, but it also breaks our convention of specifying configuration within the ServiceBusClientOptions.

Don't the constructors of the plugins need to be exposed with RegisterPlugin anyway?

No, they don't. From the DX point of view, it's much more convenient to invoke an extension method to register a plugin, optionally providing a configuration object if the defaults do not work out.

I agree that RegisterPlugin would mean less boilerplate, but it also breaks our convention of specifying configuration within the ServiceBusClientOptions

You're saying that following the conventions is more important than the customer experience?
Just for the comparison sake, layout the code that will be required to register a plugin and compare it to what I've presented.

Don't the constructors of the plugins need to be exposed with RegisterPlugin anyway?

No, they don't. From the DX point of view, it's much more convenient to invoke an extension method to register a plugin, optionally providing a configuration object if the defaults do not work out.

I agree that RegisterPlugin would mean less boilerplate, but it also breaks our convention of specifying configuration within the ServiceBusClientOptions

You're saying that following the conventions is more important than the customer experience?
Just for the comparison sake, layout the code that will be required to register a plugin and compare it to what I've presented.

Yeah no argument there in terms of there being more boilerplate, and this certainly will factor into the design. Your point about using extension methods to avoid exposing plugin ctors is good information as well.

```c#
var pluginOptions = new ServiceBusPluginOptions();
pluginOptions.RegisterPlugin(new TestPlugin());
var clientOptions = new ServiceBusClientOptions
{
PluginOptions = pluginOptions
};
var client = new ServiceBusClient(connStr, clientOptions);

```c#
var client = new ServiceBusClient(connStr, clientOptions);
client.RegisterPlugin(new TestPlugin());

I do worry about race conditions and such when we allow the client to be mutable in this way, but we can make sure to take a lock on the plugins when updating or reading from them to ensure determinism. I suppose we could just copy off the plugins at the time of receiver/sender/processor creation to avoid multithreading concerns.

If I'd rate developer experience, this is would be my rating:

Poor

```c#
var pluginOptions = new ServiceBusPluginOptions();
pluginOptions.RegisterPlugin(new TestPlugin());
var clientOptions = new ServiceBusClientOptions
{
PluginOptions = pluginOptions
};
var client = new ServiceBusClient(connStr, clientOptions);

**Optimal**

```c#
var client = new ServiceBusClient(connStr, clientOptions);
client.RegisterPlugin(new TestPlugin());

// alternatively, extension methods provided by the plugin author 
// to reduce implementation details and breaking change releases

client.UsePluginX(/*optional plugin configuration object*/);

To sum it up: I've spent some time on both sides of the equation and would strongly recommend considering adopting a friendlier approach.

I do worry about race conditions and such when we allow the client to be mutable in this way, but we can make sure to take a lock on the plugins when updating or reading from them to ensure determinism. I suppose we could just copy off the plugins at the time of receiver/sender/processor creation to avoid multithreading concerns.

Sounds reasonable.

Reopening this and moving to the backlog. We will investigate adding this feature after GA so that we can align across all languages.

Reopening this and moving to the backlog. We will investigate adding this feature after GA

This means there will be no feature parity between track 1 and track 2 when the latter gets a stable release. Is that the intention?

Yes, plugins will be looked at across all language SDKs as a post-GA feature.

Was this page helpful?
0 / 5 - 0 ratings