dotnet / aspire

Tools, templates, and packages to accelerate building observable, production-ready apps
https://learn.microsoft.com/dotnet/aspire
MIT License
3.86k stars 462 forks source link

Aspire.Hosting.Azure.EventHubs should support configuring Consumer Groups #5561

Open eerhardt opened 2 months ago

eerhardt commented 2 months ago

Today, Aspire.Hosting.Azure.EventHubs allows users to add EventHub instances:

https://github.com/dotnet/aspire/blob/7b1fd6d6fe5aa0c6fc3680f0e23ec127c9743789/src/Aspire.Hosting.Azure.EventHubs/AzureEventHubsExtensions.cs#L80-L85

However, the developer can't add consumer groups to the EventHub.

We should allow for consumer groups to be modeled.

One issue is the AddEventHub method doesn't return an EventHubResource, but instead returns the AzureEventHubsResource (which represents the Event Hubs Namespace). We will need to figure out how to rectify that, as we would need to model EventHubResource as a class we can add consumer groups to.

We may also want to be able to configure partition count as well.

cc @sebastienros @davidfowl

oising commented 1 month ago

Just to add to this, configuring consumer groups is critical for DAPR integration, and let's not forget the emulator too. The DAPR pubsub component, when bound to an event hub, necessitates each subscribed app+sidecar to have its own consumer group (i.e. it's not a pool of competing clients, they are isolated)

oising commented 1 month ago

@eerhardt -- I would like to take a go at this, but it would be for 9.1 9.0 timeline if november is the release -- i.e. the IResourceWithParent hooks for EventHubResource and the configuration of consumer groups, partitions etc. For both the emulator and live resource. I think I can figure it out from the way the storage hosting bits work, i.e add-storage->add-blob

mitchdenny commented 1 month ago

Following the conversation on Discord last night, I had a look at this today. Realistically I don't think we can get this change in for .NET 9.0 RC (need to think about whether we want to take it for GA). My inclination is that we leave this as is for 9.0 and in 9.x we do something better without breaking APIs. Here is what we could do:

+ IResourceBuilder<AzureEventHubsNamespaceResource> AddAzureEventHubsNamespace(this IDistributedApplicationBuilder builder, string name);
+ IResourceBuilder<AzureEventHubResource> AddEventHub(this IResourceBuilder<AzureEventHubsNamespaceResource> builder, string name);
+ IResourceBuilder<AzureEventHubConsumerGroup> AddConsumerGroup(this IResourceBuilder<AzureEventHubResource> builder, string name);

This means your code would end up looking something like this:

var builder = DistributedApplication.Create(args);
var ns = builder.AddAzureEventHubsNamespace("ns");
var orders = ns.AddEventHub("orders");

builder.AddProject<Project.Frontend>("frontend")
       .WithReference(orders); // This is a producer.

builder.AddProject<Projects.Fulfillment>("fulfillment")
       .WithReference(orders.AddConsumerGroup("fulfillment"));

builder.AddProject<Projects.Fulfillment>("marketing")
       .WithReference(orders.AddConsumerGroup("marketing"));

To me this would be a more typical usage. In the meantime, whilst we work on this for 9.x we document how folks can at least configure consumer groups using ConfigureConstruct(...) because it is possible to do.

mitchdenny commented 1 month ago

@oising note that this isn't a "go do" its more of a conversation starter on the strategy we should follow around how this change fits into our release schedule. Trying to strike the balance between rushing in something for 9.0 vs. taking a bit longer and doing something for 9.x whilst avoiding a breaking API change before 10.0.

mitchdenny commented 1 month ago

/cc @davidfowl @eerhardt

oising commented 1 month ago

Alrighty, putting the tools down. Tag me when you have an approach. One thing though: any ConfigureConstruct approach isn't going to work on the emulator, right?

oising commented 1 month ago

This seems an odd usage pattern to me:

builder.AddProject<Projects.Fulfillment>("fulfillment")
       .WithReference(orders.AddConsumerGroup("fulfillment"));

Is there precedent? I would have expected:

var builder = DistributedApplication.Create(args);
var ns = builder.AddAzureEventHubsNamespace("ns");
var orders = ns.AddEventHub("orders")
    .WithConsumerGroups(["a","b","c"])
    .WithPartitionCount(4);

builder.AddProject<Project.Frontend>("frontend")
       .WithReference(orders); // This is a producer.

builder.AddProject<Projects.Fulfillment>("fulfillment")
       .WithReference(orders);

...

For all intents and purposes, consumer groups are a static assignment in the apphost, even though the reality is that you can add and remove them at runtime through the portal. At one point in the past, partition count was also truly static, but now that can be adjusted for premium skus after hub creation. But again, for the purpose of the apphost, it's effectively static. Using the Add verb seems a bit confusing as it may imply the availability of a Remove equivalent.

oising commented 1 month ago

@mitchdenny @davidfowl @eerhardt

Is this the right approach for building this out?

namespace Aspire.Hosting.Azure;

public class AzureEventHubResource(string name, AzureEventHubsResource eventHubsNamespace) : Resource(name), 
    IResourceWithParent<AzureEventHubsResource>,
    IResourceWithConnectionString,
    IResourceWithAzureFunctionsConfig
{
    public ReferenceExpression ConnectionStringExpression =>
        Parent.IsEmulator
            ? ReferenceExpression.Create($"Endpoint=sb://{Parent.EmulatorEndpoint.Property(EndpointProperty.Host)}:{Parent.EmulatorEndpoint.Property(EndpointProperty.Port)};SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=SAS_KEY_VALUE;UseDevelopmentEmulator=true;EntityPath={Name}")
            : ReferenceExpression.Create($"{Parent.EventHubsEndpoint};EntityPath={Name}");

    public AzureEventHubsResource Parent => eventHubsNamespace;

    void IResourceWithAzureFunctionsConfig.ApplyAzureFunctionsConfiguration(IDictionary<string, object> target, string connectionName)
    {
        AzureEventHubsResource.ApplyAzureFunctionsConfigurationInternal(target, connectionName,
            eventHubsNamespace.IsEmulator,  ConnectionStringExpression, eventHubsNamespace.EventHubsEndpoint);
    }
}

public static class AzureEventHubExtensions
{
    public static IResourceBuilder<AzureEventHubResource> WithPartitionCount(
        this IResourceBuilder<AzureEventHubResource> builder,
        int count) => builder.WithAnnotation(new PartitionCountAnnotation(count));

    public static IResourceBuilder<AzureEventHubResource> WithConsumerGroups(
        this IResourceBuilder<AzureEventHubResource> builder,
        IImmutableSet<string> consumerGroups) => builder.WithAnnotation(new ConsumerGroupsAnnotation(consumerGroups));
}

public class ConsumerGroupsAnnotation(IImmutableSet<string> ConsumerGroups) : IResourceAnnotation;

public class PartitionCountAnnotation(int Count) : IResourceAnnotation;

The AzureEventHubResource part demonstrably works as expected, but I'm not certain how I keep the metadata such as Consumer Groups and Partition Count. Above, I am creating two new annotations so I can read them back later constructing the emulator config / bicep parameters. Is this the right way to go about it?

I know nobody is asking for this, but at a minimum I'm trying to understand the framework better.

mitchdenny commented 1 month ago

You would probably call ConfigureConstruct and fish out the EventHub resource by name and then apply the partition count and consumer groups changes to the construct.

mitchdenny commented 1 month ago

This seems an odd usage pattern to me:

builder.AddProject<Projects.Fulfillment>("fulfillment")
       .WithReference(orders.AddConsumerGroup("fulfillment"));

<snip>

AddConsumerGroup would be returning an IResourceBuilder<AzureEventHubConsumerGroup> which is then being referenced by WithReference because it implements IResourceWithConnectionString. The connection string would container information that is used by the EventHubs integration package to target the correct consumer group.

There are a bunch of assumptions here which is why we need to spend some time figuring it out.

Be advised that between now and 9.0 GA, @eerhardt is making a bunch of changes to the Azure-related API surfaces in Aspire as a result of some of the changes in Azure Provisioning and maturity of our thinking around the Aspire API-side of the API surface.

My proposal above is informed by that somewhat. The problem with something with WithConsumerGroups(array) is that you need to figure out how you are going to tell consuming services which consumer group they should be using. This kind of information probably belongs in a connection string.

For something like WithPartitionCount, its a fine extension method to have if its important enough to configure the partition count (I know in the case of Event Hubs this is the case). But ultimately it'll just be calling down into ConfigureConstruct. It's a convenience method.

oising commented 1 month ago

This seems an odd usage pattern to me:

builder.AddProject<Projects.Fulfillment>("fulfillment")
       .WithReference(orders.AddConsumerGroup("fulfillment"));

<snip>

AddConsumerGroup would be returning an IResourceBuilder<AzureEventHubConsumerGroup> which is then being referenced by WithReference because it implements IResourceWithConnectionString. The connection string would container information that is used by the EventHubs integration package to target the correct consumer group.

I think the desire for symmetry here is obscuring the canonical usage patterns for event hubs. A consumer group is not a legitimate segment of a legal event hub connection string. See below why not.

There are a bunch of assumptions here which is why we need to spend some time figuring it out.

I understand.

Be advised that between now and 9.0 GA, @eerhardt is making a bunch of changes to the Azure-related API surfaces in Aspire as a result of some of the changes in Azure Provisioning and maturity of our thinking around the Aspire API-side of the API surface.

Good to know.

My proposal above is informed by that somewhat. The problem with something with WithConsumerGroups(array) is that you need to figure out how you are going to tell consuming services which consumer group they should be using. This kind of information probably belongs in a connection string.

I'm not sure I would agree with this. It does seem convenient on the surface, but there is no guarantee that there's going to be a one-to-one relationship between the hosting configuration and any client(s) with respect to consumer groups. More telling, there is no formal construction of a legal event hub connection string that contains a specified consumer group. It really is down to the application to decide which work/consumer group they are a part of for a given hub.

edit: I see now how you suggest integrating a client-specific consumer group by shifting the Add statement to the project level -- but nonetheless, it seems out of band for an apphost to do this, imho.

For something like WithPartitionCount, its a fine extension method to have if its important enough to configure the partition count (I know in the case of Event Hubs this is the case). But ultimately it'll just be calling down into ConfigureConstruct. It's a convenience method.

Agreed.

Thank you for the guidance and feedback @mitchdenny -- much appreciated.