Azure / azure-functions-dotnet-worker

Azure Functions out-of-process .NET language worker
MIT License
408 stars 172 forks source link

Returning ServiceBusMessage doesn't work correctly #1819

Open SeanFeldman opened 12 months ago

SeanFeldman commented 12 months ago

When returning a ServiceBusMessage, the message posted contains incorrect information.

[Function(nameof(Run))]
[ServiceBusOutput("audit", Connection = "AzureServiceBus")]
public async Task<ServiceBusMessage> Run([ServiceBusTrigger("Notifications", Connection = "AzureServiceBus")] ServiceBusReceivedMessage message)
{
    // removed for brevity 

    return new ServiceBusMessage(rawMessage);
}

For an incoming message

image

The returned message body is

image

1 - missing original header 2 - incorrect body 3 - incorrect content type

Expected:

image

1 - the original header (EventType) is found 2 - original body 3 - correct content type

liliankasem commented 12 months ago

I would not expect that to work today, we are currently not supporting SDK-types for output bindings as we're pushing for folks to start using clients for output instead.

SeanFeldman commented 12 months ago

we're pushing for folks to start using clients for output instead

@liliankasem is there a sample you could refer me to?

Out of curiosity, why's the push? Is it documented somewhere to allow understanding the rationale? Thank you.

liliankasem commented 12 months ago

Should be documented here, and each extensions output page should have a comment about creating and using SDK types directly for output scenarios. Though I'm not sure that we documented the "why" - @mattchenderson

I don't have a sample, I think the idea is to create and use your own ServiceBusClient; or once the message settlement preview becomes available, using those types.

SeanFeldman commented 12 months ago

Appreciate the links and the details, @liliankasem.

each extensions output page should have a comment about creating and using SDK types directly for output scenarios

May I suggest making the note more standing out? It's easily dismissed when looking at the wall of text. It might be a small change in the docs that will be very appreciated.

image

I don't have a sample, I think the idea is to create and use your own ServiceBusClient; or once the message settlement preview becomes available, using those types.

Message settlement would be configured to use the client to receive the messages. The flexibility of the In-Proc model allowed having the output binding configured to use a different namespace. Using a client injected into the Function would be the only option.

Though I'm not sure that we documented the "why"

Given how simple the output binding with the In-Proc SDK was, I'd like to understand. Now, if you need to output messages, you need to add more code by registering the client and invoking it. Something that Functions was good at reducing. Also, by doing so, for scenarios where messages are received and sent to the same namespace, there will be 2 connections open to the ASB namespace per function instance instead of 1. At a scale, that will add up on the overall number of connections that have a limit.

I don't have a sample

The currently found sample under the ASB output binding looks like the sample contradicts the general guidance, as it shows ServiceBusOutput use despite the note "For other output scenarios, create and use types from Azure.Messaging.ServiceBus directly." 😀

mattchenderson commented 11 months ago

May I suggest making the note more standing out? It's easily dismissed when looking at the wall of text. It might be a small change in the docs that will be very appreciated.

Agreed. I'm actually trying to tee up a scenario pivot to these docs so there will be a whole section about writing to blobs (or whatever) that covers options and examples. Behind schedule there, and in the meantime, this could definitely use a boost in visibility of some kind.

Message settlement would be configured to use the client to receive the messages. The flexibility of the In-Proc model allowed having the output binding configured to use a different namespace. Using a client injected into the Function would be the only option.

Yes, and client injection is the best answer here right now. I added a section on that to our DI content to hopefully help: https://learn.microsoft.com/en-us/azure/azure-functions/dotnet-isolated-process-guide#register-azure-clients

Admittedly that was put together quickly, and I especially don't love leaning on that tip note. I'm hoping with a little iteration it will land in a good spot.

Given how simple the output binding with the In-Proc SDK was, I'd like to understand. Now, if you need to output messages, you need to add more code by registering the client and invoking it. Something that Functions was good at reducing. Also, by doing so, for scenarios where messages are received and sent to the same namespace, there will be 2 connections open to the ASB namespace per function instance instead of 1. At a scale, that will add up on the overall number of connections that have a limit.

Great point on the connections. I also agree there's friction to creating the registration. However, as part of scoping things, we made a call to assess output bindings later as we believed there to be a viable alternative using injection. These discussions around how viable that actually is are super valuable. There are a few research hypotheses we're exploring based on signals we've gotten about our previous approach to output bindings. I think there are alternative solutions to some of the backing problem statements.

The currently found sample under the ASB output binding looks like the sample contradicts the general guidance, as it shows ServiceBusOutput use despite the note "For other output scenarios, create and use types from Azure.Messaging.ServiceBus directly." 😀

I think that sample is in line with the guidance, though "other" is maybe doing a bit too much heavy lifting. But string is listed as a type supported for output bindings, and while I'd like to have more samples with more flexible options using clients, etc., returning a string is still a valid approach. The qualifier is there to address things ServiceBusMessage or ServiceBusClient for outputs, which the current model does not include.

I'm also a bit curious about how much coverage the client types provide on their own. If we assume it to be sufficiently easy to have the ServiceBusClient passed in, how critical then is the ability to return a ServiceBusMessage? Where does something like .CreateSender(topic).SendMessageAsync(msg) fall short? I could imagine simplifying the hypothetical: what if the function can just get the ServiceBusSender and then call .SendMessageAsync(msg)?

SeanFeldman commented 11 months ago

I could imagine simplifying the hypothetical: what if the function can just get the ServiceBusSender and then call .SendMessageAsync(msg)?

This might be more effective as the clients could be cached based on the connection and entity those are requested for.

With all that said, receiving a ServiceBusReceivedMessage and returning from function a ServiceBusMessage would be extremely simple and powerful. And would satisfy a lot of scenarios where an incoming message is generating an outgoing message. For multiple messages, I'd agree that a client is a better approach (IAsyncCollector in In-Proc SDK was a bit of a weird API). But for a single returned message, there's a value in considering returning just a ServiceBusMessage w/o the need for a client or sender.

mattchenderson commented 11 months ago

I think we're mostly on the same page, and to me it's primarily a priority/timing consideration. The existing model can work (it covers the scenarios functionally), but there's the extra boilerplate, and I think we can definitely cut a bunch of that back. I'm in favor, myself, at least.

The client is nicer for multiple messages, but once there's ServiceBusMessage support, there's also ServiceBusMessage[], and that would be an option if someone really wants it. That does start to feel like IAsyncCollector again. But our guidance would absolutely be to use a client in that scenario.

The client approach also affords timing of the send and general error handling, in addition to other controls on how the send occurs. The message can be sent while the function continues to do other things too, rather than at the end of the invocation and outside of any try/catch, which is what using ServiceBusMessage/ServiceBusMessage[] would give you. I'd probably want to still present the client approach as an option to consider when discussing the single message return in guidance.

SeanFeldman commented 11 months ago

I'd probably want to still present the client approach as an option to consider when discussing the single message return in guidance

+1 on that.

Also, I'd suggest showing the sample how to handle the incoming message from middleware. Much of the boilerplate code could be moved from functions into middleware (related to issue #1824) when implementing some sort of custom framework on top of SB-triggered functions.

arkiaconsulting commented 6 months ago

I would add that a Function that returns a ServiceBusMessage can be tested more easily.

Archomeda commented 1 month ago

I just came across this issue where (again) there's no feature parity with the in-process model.

I found out that returning a ServiceBusMessage as output binding serializes the whole object as JSON instead of taking that message directly, which is what this issue describes. This strikes me as odd, because it used to work with the previous model.

It doesn't help that this section can be interpreted in different ways:

For other output scenarios, create and use types from Azure.Messaging.ServiceBus directly.

(as is the ServiceBus equivalent to what's mentioned here for Storage Blobs: https://github.com/Azure/azure-functions-dotnet-worker/issues/1819#issuecomment-1675203491) I interpreted this as, "oh, so if any of the above mentioned types are not sufficient, I can just use the ServiceBusMessage type from Azure.Messaging.ServiceBus and return that instead directly as output". So hence my surprise when I found an entire ServiceBusMessage as JSON in a new message body.

Can someone from the Azure Functions team please clarify what direction the team will take with bindings? Should I just fully step away from output bindings because they are unreliable and support is lacking? Or are you finally making sure that there is feature parity with the in-process model? Because honestly, I'm getting a bit tired of encountering these issues when trying to migrate (multiple times! and failing!) to the isolated model for the past 3 to 4 years now.

pkpaul5 commented 2 weeks ago

I am facing similar issue

I have function app with .NET 8 Isolated model which receives and sends message from Service Bus topic subscription. The output message is going to service bus successfully and I can see in the Insights in service bus that incoming messages have come in the topic but no message is showing in the topic subscription. I have checked the topic subscription rule which is based on subject/label. The subject field in the message and subject for rule in subscription are same. Spent many hours debugging the issue but not able to identify the cause. When I am sending the same message with same subject to Service Bus from console application to the same topic using ServiceBusClient and ServiceBusSender, it is working fine. Below is the function app code:

[Function(nameof(Function1))]

[ServiceBusOutput("topic2", Connection = "ServiceBusConnection1")]

public async Task Run(

[ServiceBusTrigger("topic1", "subscription1", Connection = "ServiceBusConnection1")]

ServiceBusReceivedMessage message,

ServiceBusMessageActions messageActions) { _logger.LogInformation("Message ID: {id}", message.MessageId);

_logger.LogInformation("Message Body: {body}", message.Body);

_logger.LogInformation("Message Content-Type: {contentType}", message.ContentType);

await messageActions.CompleteMessageAsync(message);

ServiceBusMessage message1 = new() { Subject = "Subject1" };

message1.MessageId = Guid.NewGuid().ToString();

message1.Body = new BinaryData(Encoding.UTF8.GetBytes("test"));

return message1;

}