Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
eerhardt opened this issue Sep 5, 2024 · 14 comments · May be fixed by #6085
Open

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

eerhardt opened this issue Sep 5, 2024 · 14 comments · May be fixed by #6085
Labels
area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure untriaged New issue has not been triaged

Comments

@eerhardt
Copy link
Member

eerhardt commented Sep 5, 2024

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

/// <summary>
/// Adds an Azure Event Hubs hub resource to the application model. This resource requires an <see cref="AzureEventHubsResource"/> to be added to the application model.
/// </summary>
/// <param name="builder">The Azure Event Hubs resource builder.</param>
/// <param name="name">The name of the Event Hub.</param>
public static IResourceBuilder<AzureEventHubsResource> AddEventHub(this IResourceBuilder<AzureEventHubsResource> builder, string name)

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Sep 5, 2024
@davidfowl davidfowl added azure Issues associated specifically with scenarios tied to using Azure enhancement area-integrations Issues pertaining to Aspire Integrations packages and removed area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Sep 5, 2024
@oising
Copy link
Contributor

oising commented Sep 30, 2024

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
Copy link
Contributor

oising commented Sep 30, 2024

@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
Copy link
Member

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
Copy link
Member

mitchdenny commented Oct 4, 2024

@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
Copy link
Member

/cc @davidfowl @eerhardt

@oising
Copy link
Contributor

oising commented Oct 4, 2024

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
Copy link
Contributor

oising commented Oct 4, 2024

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
Copy link
Contributor

oising commented Oct 7, 2024

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

oising commented Oct 8, 2024

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.

@joperezr joperezr added the untriaged New issue has not been triaged label Oct 15, 2024
@oising
Copy link
Contributor

oising commented Nov 21, 2024

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
Copy link
Contributor

oising commented Nov 27, 2024

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 ?

@eerhardt
Copy link
Member Author

We should follow similar API patterns/designs as we are doing to support the ServiceBus emulator. #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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure untriaged New issue has not been triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants