If we can agree on tihs, I'll send a PR for the same thing in Azure Service Bus for queue/topic in EntityPath, or I can put it in this one

Does this also work when deployed?

Does this also work when deployed?

It's funny you should ask -- I had just deployed the playground project to test, and I found a bug. I'll report back when fixed.

Well, sh*t.

It turns out that there's no way to officially pass the event hub name when using the FQNS + TokenCredential for a real Event Hub. It's fine for the emulator, because the non-tokencredential connection string allows you to pass an EntityPath. I had mistakely believed you could include the hub name as part of the Uri in the non-emulator case.

One way to do it would be to add Aspire-specific handling of a FQNS that includes the hub name as a query string, e.g. https://foobar.servicebus.windows.net?EntityPath=hubName. But this would be setting a new precedent for Aspire I think, i.e. special casing parsing of connection strings in the Aspire clients. As for my thoughts on the matter, at first it felt off (and it does a little still) but it does not break compatibility in the connection strings for non-Aspire wrapped clients.

The other way might be to emit an environment variable, but then this needs even more thought for scoping/namespacing it.
Thoughts?

@davidfowl @mitchdenny @eerhardt

We can use our own connection string format that the client understands. This is what openai does

We can use our own connection string format that the client understands. This is what openai does

What about setting the ENV that will be bound to the EntityPath property in the settings on the client instead? I am doing this in a PR for openai to split the connection string from the models/deployments.

Another comment on this PR, late one sorry, I personally prefer calling a helper like WithEntityPath(string?) instead of setting a flag on any hub. I assume this way you could even set it if you don't create the hubs at the host level too?

We can use our own connection string format that the client understands. This is what openai does

What about setting the ENV that will be bound to the EntityPath property in the settings on the client instead? I am doing this in a PR for openai to split the connection string from the models/deployments.

Another comment on this PR, late one sorry, I personally prefer calling a helper like WithEntityPath(string?) instead of setting a flag on any hub. I assume this way you could even set it if you don't create the hubs at the host level too?

Hi @sebastienros - I had thought of that in an earlier point in time, but given that there are five different clients for eventhub, we'd have to inject five env vars to bind to all client settings. Also, they would take precedence over client appsettings which might be at best surprising and at worst possibly breaking, right? If we use a custom connection string then it's just a hint and the regular configuration should take precedence, if provided.

As for the WithEntityPath idea, I guess so? What does everyone else think?

I prefer a single connection string… This brings back our connection info discussion 🙂

@davidfowl:

I prefer a single connection string… This brings back our connection info discussion 🙂

Alrighty -- so I'll add a query path hint in the FQNS. And are we good with having WithEntityPath(string) ? It would just set IsDefaultEntity on the model, and I'd drop the visibility to internal on the field. I take it we don't want to start down the "two ways to do everything" path.

before making the change, can we compare both API samples?

@davidfowl

before making the change, can we compare both API samples?

A

var eventHub = builder.AddAzureEventHubs("eventhubns")
    .RunAsEmulator()
    .WithHub("hub1")
    .WithHub("hub2", h => h.IsDefaultEntity = true);

vs

B

var eventHub = builder.AddAzureEventHubs("eventhubns")
    .RunAsEmulator()
    .WithHub("hub1")
    .WithHub("hub2")
    .WithDefaultEntity("hub2"); // WithEntityPath ?

At first you might think why not WithDefaultHub? And the reasoning is if we bring the same API to service bus, you'd need WithDefaultQueue and WithDefaultTopic, which would be mutually exclusive. Using Entity is clearer, IMO. I prefer WithDefaultEntity over WithEntityPath because the former is about the resource and the latter is about connection strings, but the implication should be clear.

I am fine with both approaches, my preference being an extension method (whatever name makes more sense), or an argument on the AddAzureEventHubs(). The value could then be stored in the event hubs resource. We may don't even have to deal with invalid state, in case you want to set the value with a hub name that doesn't need to be defined on the resource (like you can do today).

Does my suggestion to generate a separate ENV to flow to the settings make sense? It would be prefixed with the resource name to bind to the correct settings.

@sebastienros

Does my suggestion to generate a separate ENV to flow to the settings make sense? It would be prefixed with the resource name to bind to the correct settings.

Yes, I thought I had mentioned this above. It does make sense from a mechanical perspective, but like I had said, there are five different clients that ship in the integration and you'd have to inject an ENV var for all of them since you cannot know in the hosting side which clients will be used. Personally, I prefer to inline a hint in the connectionstring. It keeps the scope narrower. Although injecting environment variables means we could probably get away without modifying the clients, it might cause unexpected behaviour when someone tries to specify/override the event hub in appsetting.json -- the env var will always win for configuration.

when someone tries to specify/override the event hub in appsetting.json

Isn't it of higher priority than ENVs? And then the configure callback has the most priority.

there are five different clients that ship in the integration

I checked the code as I am not familiar with it, I see the issue is each component has its own settings class, but the EventHubName is set on the base one.

Would something like this work:

    protected override void BindSettingsToConfiguration(AzureMessagingEventHubsBufferedProducerSettings settings,
        IConfiguration configuration)
    {
        configuration.Bind((AzureMessagingEventHubsSettings)settings);
        configuration.Bind(settings);
    }

If not set in component specific section, it would take the one from the apphost config. configureSettings would still work. Bonus, now you can even define the defaults for any component.

when someone tries to specify/override the event hub in appsetting.json

Isn't it of higher priority than ENVs? And then the configure callback has the most priority.

Nope. AppSettings are loaded first, then environment. Last one wins. Priority is effectively:

https://learn.microsoft.com/en-us/dotnet/core/extensions/configuration

Marking this as draft while I update the integration bits and add some more tests. Note that I also found an existing bug in the current connection string validation, I'll add more details after.

What happens when you use this feature with azure functions, none of this applies right?

cc @captainsafia

@davidfowl

What happens when you use this feature with azure functions, none of this applies right?

cc @captainsafia

It only improves the scenario for Functions that use the SASKey style connection string that officially supports EntityPath. Existing Function bindings that are explicit about the Event Hub name will override this, so I don't forsee any breaking behaviour there. For those bound via an FQNS + managed identity, I don't touch it since we don't have control over the client to parse out the hint.

Ooops, I accidentally checked in overrides in the playground to use AzureCliCredential for my local testing. I'll remove them in the next commit.

Ready for review @davidfowl

I've updated the PR description with more details to help with the review.