Add support for Azure Event Hubs

[oising]

Initial support for Azure Event Hubs, as per #1147

• Minimally adapt the Service Bus component and resource for Event Hubs
• Add top-level support for EventHubProducerClient
• Add top-level support for EventHubConsumerClient
• Add top-level support for EventProcessorClient
  • Allow providing component name for a BlobContainerClient to use (read connection string and instantiate local instance)
  • Read BlobContainerClient from keyed DI using its Aspire connection name
• Add top-level support for PartitionReceiver
• Add playground sample AppHost and projects (modify davidfowl's test project)
• Add ability to configure credential used in Azure Provisioner (via AzureProvisionCredential in AppHost configuration)
• Add xmldoc documentation for components etc
• Update README

Optional extras (may defer to a second PR)

• Add support for chained AddHub(...) calls for top-level EH resource to allow variable config of hubs
  • Implement as child resource to allow deep-linking (i.e. provide connection string including EntityPath)
• Implement Microsoft.Extensions.Azure style top-level component AddEventHubs(...) and hang clients configuration from clientBuilder
• Add Streaming sample with Orleans

Aspire API shortcomings

EventProcessorClient requires a BlobContainerClient for managing EH checkpoints. Ideally we should be able to pass a connection name to a mounted aspire blob client component and retrieve the instance from the service provider, but there is no plumbing to get it in the component infra. The unbuilt service collection is available on the distributed builder and it could be fished out of there, assuming it was added before the current component, but this seems flakey. Instead, I'm creating a new blob client using the blob connection name (taken from user-supplied settings) and creating my own.

Microsoft Reviewers: Open in CodeFlow

[davidfowl]

The azure provisioning stuff here is outdated so I would just delete all of it since it isn't required.

[davidfowl]

This entire PR seems based on a really old version of aspire. Everything is bicep based now.

[oising]

This entire PR seems based on a really old version of aspire. Everything is bicep based now.

I think your definition of "really old" doesn't match with mine :) I'm working off of main, and I cloned barely a week ago? I'll update it.

[davidfowl]

I'm not sure what happened in this PR, but if you look at main the Aspire.Hosting.Azure.Provisioning package has been deleted... 😄

#2603

Somehow this brought it back.

[oising]

I'm not sure what happened in this PR, but if you look at main the Aspire.Hosting.Azure.Provisioning package has been deleted... 😄

#2603

Somehow this brought it back.

I'll start from scratch -- tbh, this first pass was just about understanding how components and resources work (in this week's build lol)

[oising]

Restarting for the third time, because of #2875

Done.

[davidfowl]

@sebastienros @eerhardt for the component review
@mitchdenny @JoshLove-msft for the resource review. CDK codegen is also required here.

[davidfowl]

Also @jsquire for azure sdk review

[oising]

I rebased to stay up to date and now I'm hitting:

Application orchestrator dependency check had an unexpected error System.TimeoutException: The operation has timed out.
at Aspire.Hosting.Dcp.DcpDependencyCheck.EnsureDcpDependenciesAsync(CancellationToken cancellationToken) in C:\p\github\aspire\src\Aspire.Hosting\Dcp\DcpDependencyCheck.cs:line 67

on start up.

[davidfowl]

Application orchestrator dependency check had an unexpected error System.TimeoutException: The operation has timed out.
at Aspire.Hosting.Dcp.DcpDependencyCheck.EnsureDcpDependenciesAsync(CancellationToken cancellationToken) in C:\p\github\aspire\src\Aspire.Hosting\Dcp\DcpDependencyCheck.cs:line 67

Consistently?

[oising]

Application orchestrator dependency check had an unexpected error System.TimeoutException: The operation has timed out.
at Aspire.Hosting.Dcp.DcpDependencyCheck.EnsureDcpDependenciesAsync(CancellationToken cancellationToken) in C:\p\github\aspire\src\Aspire.Hosting\Dcp\DcpDependencyCheck.cs:line 67

Consistently?

Yes. I tried bumping the timeout to 5 mins and at some point something else timed out and brought the whole process down. I tried disabling the timeout (setting to 0) and same thing. Tried about a half dozen times.

edit: resolved, see #3003

[eerhardt]

This assumes you have included the EntityPath in the connection string to specify the Event Hub name.

This doesn't seem ideal, since we say we don't recommend using a connection string.

[eerhardt]

It is also surprising since I would have expected:

var eventHub = builder.AddAzureEventHubs("eventhubns")
    .AddEventHub("hub");

builder.AddProject<Projects.EventHubsApi>("api")
    .WithReference(eventHub);

To result with the EntityPath in the connection string (just like it does if I'm using a database server + database, the connection string has Database=<db_name> in the connection string.

Is the above code supposed to use EntityPath in the connection string?

[oising]

@mitchdenny Brought this up already and it would mean making the "hub" it's own de-facto AspireResource to enable deep-linking like this. I replied and said the same could be said for allowing deep-linking of ASB topic and queues (a feature that isn't there either) so I think this could be punted for another release. But yep, you're right -- AEH and ASB should do like MSSQL does. But I don't have time to get this done for this release. I'm happy to improve it for P6.

As you (or maybe someone else said?) before, while we do control what Resources inject as connection strings when using an AppHost, we don't control what developers may do when managing their own connection strings. I'd rather that we "do the right thing" when a dev passes an entitypath. It seems like the path of least surprise. It's what I would expect.

[eerhardt]

we don't control what developers may do when managing their own connection strings. I'd rather that we "do the right thing" when a dev passes an entitypath. It seems like the path of least surprise. It's what I would expect.

I don't disagree that we should "do the right thing" when someone passes an entitypath.

The part I was commenting on is the "Usage example" the README gives assumes you are doing something we don't recommend you do - use a connection string.

[oising]

Ah, gotcha!

[eerhardt]

This API pattern differs from how databases work. I think it will be surprising to our customers.

In databases, we have:

var db = builder.AddPostgres("db-server").AddDatabase("db1");

builder.AddProject<Projects.Api>("api")
    .WithReference(db);

When executing that code, db points to the db1 resource - a "child" of the db-server. So when you .WithReference(db), the connection string name that you use in your app is going to be db1.

But here,

var hub = builder.AddEventHubs("eventhub-ns").AddEventHub("hub");

builder.AddProject<Projects.Api>("api")
    .WithReference(hub);

hub is going to point to the eventhub-ns resource, and not the hub resource. So in your web app, you need to use builder.AddAzureEventHubProducerClient("eventhub-ns"), which is different than how databases work.

[oising]

Agree 100% -- see my comment above. The service bus resource is equally deficient, and I copied that code. I'd want to see this fixes for both ASB and AEH in P6. /cc @mitchdenny

[davidfowl]

Does this work with managed identity?

[oising]

Does this work with managed identity?

Did you mean to put this here, @davidfowl ? If so, I don't see why not?

[eerhardt]

FYI @oising, I pushed some changes you should be aware of. The most important is that I fixed Tracing to work. Just wanted to let you know so you don't force push and revert those changes.

I think this is close to getting the initial merge. My most major concerns are:

• Getting the Storage Blob client. I really think this should come from DI
• The Settings class having properties that don't work depending on the client being used.
• Full testing

If we want to merge this soon to guarantee it gets into preview5, and log issues for these to be followed up, I would be OK with that. FYI @davidfowl

[oising]

FYI @oising, I pushed some changes you should be aware of. The most important is that I fixed Tracing to work. Just wanted to let you know so you don't force push and revert those changes.

I think this is close to getting the initial merge. My most major concerns are:

• Getting the Storage Blob client. I really think this should come from DI
• The Settings class having properties that don't work depending on the client being used.
• Full testing

If we want to merge this soon to guarantee it gets into preview5, and log issues for these to be followed up, I would be OK with that. FYI @davidfowl

Thanks @eerhardt -- I really appreciate the thorough review and insightful suggestions :) I'm about to check in the suggested split settings approach as discussed above. It took a bit of jiggery-pokery with generics to fix a few things, but it seems to be good now.

As for the storage blob client, yes, I agree this is a bit of a rough edge; especially around the lack of unit tests here. As mentioned, the service provider isn't built at this point, but I could build it and look for a blob client within. I think this needs some configuration-by-convention love in the sense that we should probably only permit a keyed blob client so it can be configured in settings. Also, to test the logic around the container creation, I might want to lightly shim the blob client so I can unit test the behaviour without requiring a live connection. I'll spike this tomorrow.

[eerhardt]

As mentioned, the service provider isn't built at this point, but I could build it and look for a blob client within.

This isn't necessary. We should just be able to call a different overload on the AzureClientFactoryBuilder, which gives us the IServiceProvider. See https://github.com/Azure/azure-sdk-for-net/blob/87d426597b261b88f8a6c6c21fab6031f857fb70/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientFactoryBuilder.cs#L164-L172. (Note it would take some refactoring of the base AzureComponent<TSettings, TClient, TClientOptions> class, as right now the abstract AddClient method takes a TBuilder, when it could just be passed an AzureClientFactoryBuilder.

I think this needs some configuration-by-convention love in the sense that we should probably only permit a keyed blob client so it can be configured in settings.

I see it a different way. It should allow for a keyed blob client, but shouldn't require it to be keyed. If no "blob service key" was configured, it should just get the BlobServiceClient from DI. If a "blob service key" was specified, then use it to get the BlobServiceClient from DI.

That way the simple case "just works". You just need to:

var builder = WebApplication.CreateBuilder(args);

builder.AddAzureBlobClient("blobConnectionName");
builder.AddAzureEventProcessorClient("eventhubns");

and the EventProcessorClient grabs the BlobClient. There's no need to force someone to use keys when there is only one.

[eerhardt]

Thank you, @oising, for this contribution! Great work!

This LGTM to merge for preview5.

Can you log follow up issues for the remaining work? Notably:

• Using DI to get the blob client in the EventProcessor
• Testing
• (I might have missed others. Please ensure it is all logged.)

[davidfowl]

Thank you @oising !!!!!