aspire icon indicating copy to clipboard operation
aspire copied to clipboard

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

Open eerhardt opened this issue 1 year ago • 7 comments

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

eerhardt avatar Sep 05 '24 19:09 eerhardt

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 avatar Sep 30 '24 14:09 oising

@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

oising avatar Sep 30 '24 20:09 oising

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 avatar Oct 04 '24 02:10 mitchdenny

@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 avatar Oct 04 '24 02:10 mitchdenny

/cc @davidfowl @eerhardt

mitchdenny avatar Oct 04 '24 02:10 mitchdenny

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 avatar Oct 04 '24 13:10 oising

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 avatar Oct 04 '24 14:10 oising

@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.

oising avatar Oct 07 '24 14:10 oising

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 avatar Oct 08 '24 01:10 mitchdenny

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.

mitchdenny avatar Oct 08 '24 01:10 mitchdenny

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.

oising avatar Oct 08 '24 13:10 oising

Here's a workaround for anyone who finds this issue. This intercepts the JSON config builder for the emulator will allow you to change the partition count and/or consumer groups. Note there's no need to add the $default consumer group: this will be added by the emulator automatically (and may even cause the config parsing to fail if you add it explicitly.)

https://gist.github.com/oising/3dd68b7605cae511434ced4971b6551a

It should work in aspire 8 and 9.

oising avatar Nov 21 '24 17:11 oising

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.

@mitchdenny Since native event hub connection strings do not permit specifying a consumer group, could we just emit [an] environment variable(s) that bind(s) to ClientSettings:ConsumerGroup in configuration? The only thing that's a bit dirty is that you don't know in advance what sort of event hub client (there are five) the integration is using, so you'd have to emit all five env vars (unless you add control in the apphost for this), and only one would be used. This concept could be generalized for any out of band (i.e. cannot be specified in a connectionstring) settings, which would require the client settings object to have the relevant field.

IResourceWithClientSettingsBinding ?

oising avatar Nov 27 '24 19:11 oising

We should follow similar API patterns/designs as we are doing to support the ServiceBus emulator. https://github.com/dotnet/aspire/pull/6737

cc @sebastienros

could we just emit [an] environment variable(s) that bind(s) to ClientSettings:ConsumerGroup in configuration?

My opinion is that we add support for modeling it in the AppHost (that works for both the emulator and for azd deployment). And we leave it up to the developer to decide how to pass the names between the AppHost and the app. This is similar to what we are doing in the ServiceBus emulator. I suspect many times the names will just be hard-coded in both - just like "connection names" are hard-coded in both today. For devs who don't want to hard-code the name in the app, they can make a configuration for it. We do something similar in eshop for passing the OpenAI model name between the AppHost and the app(s).

eerhardt avatar Dec 13 '24 23:12 eerhardt

Can we add support for setting an EntityPath now in the connection string? e.g.

builder.AddAzureEventHubs("ns").WithHub("hub", configure => configure.IsDefaultEntity = true);

The builder should throw [on build?] if more than one hub is set as default. I presume this should also work for ASB.

oising avatar Jan 13 '25 21:01 oising

@oising - can you open a new issue for that? I believe this issue is now completed with https://github.com/dotnet/aspire/pull/7005.

eerhardt avatar Jan 13 '25 21:01 eerhardt

@oising - can you open a new issue for that? I believe this issue is now completed with #7005.

@eerhardt

https://github.com/dotnet/aspire/issues/7093

oising avatar Jan 13 '25 23:01 oising