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

Adding WebPubSub component and provisioning #2495

Merged
merged 5 commits into from
Jun 7, 2024
Merged

Conversation

vicancy
Copy link
Contributor

@vicancy vicancy commented Feb 28, 2024

For #1063

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Feb 28, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 28, 2024
.WithParameter("name", resource.CreateBicepResourceName())
.WithParameter(AzureBicepResource.KnownParameters.PrincipalId)
.WithParameter(AzureBicepResource.KnownParameters.PrincipalType)
// TODO: add hub settings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan to address this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hub settings are used for Azure Web PubSub to send events to the web app, (kind of similar to when event grid sends out events), here contains some detailed description: https://learn.microsoft.com/en-us/azure/azure-web-pubsub/howto-develop-eventhandler

For now when we not yet support hub setting configuration, the basic workflow (as shown in the playground) works, so I think supporting hub settings is not a blocking feature. I tend to get AddAzureWebPubSub available to aspire first, how do you think?

An ideal workflow to enable configuring hub settings would be:

  • When running locally, hub settings are set as "tunnel:///" with awps-tunnel container running as a sidecar (this tool as described here https://learn.microsoft.com/en-us/azure/azure-web-pubsub/howto-web-pubsub-tunnel-tool?tabs=bash is for local development), as similar to Azure Storage Emulator but yet both the container and WebPubSub service are needed)
  • When azd up deploying to cloud, aspire is able to add a hook to update the hub settings to the deployed website URL. As my last sync with @davidfowl this feature is on the way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When azd up deploying to cloud, aspire is able to add a hook to update the hub settings to the deployed website URL. As my last sync with @davidfowl this feature is on the way.

I'm not sure we'll be able to do anything here for GA. We're still attempting to model this.

@danmoseley danmoseley added area-components Issues pertaining to Aspire Component packages and removed area-codeflow for labeling automated codeflow. intentionally a different color! labels Mar 5, 2024
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like CI is broken. Can you fix it?

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good. I was able to use azure provisioning locally and even azd up and the playground app worked!

/// <param name="configureClientBuilder">An optional method that can be used for customizing the <see cref="IAzureClientBuilder{WebPubSubServiceClient, WebPubSubServiceClientOptions}"/>.</param>
/// <remarks>Reads the configuration from "Aspire.Azure.Messaging.WebPubSub" section.</remarks>
/// <exception cref="InvalidOperationException">Thrown when neither <see cref="AzureMessagingWebPubSubSettings.ConnectionString"/> nor <see cref="AzureMessagingWebPubSubSettings.Endpoint"/> is provided.</exception>
public static void AddAzureWebPubSub(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static void AddAzureWebPubSub(
public static void AddAzureWebPubSubClient(

We added Client to all our extension methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebPubSub is a little bit different than other service's Rest Client, as WebPubSub has another real client role, as shown in this diagram:

image

And we also have 2 packages, one is Azure.Messaging.WebPubSub which aspire component adds, another is Azure.Messaging.WebPubSub.Client which is for the real client role.
AddAzureWebPubSubClient might be a bit confusing for WebPubSub, maybe AddAzureWebPubSubServiceClient? I actually prefer the shorter one AddAzureWebPubSub ....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we changed the method names is to disambiguate the methods between the AppHost and the apps:

```csharp
var webPubSub = builder.AddAzureWebPubSub("wps");

var myService = builder.AddProject<Projects.MyService>()
                       .WithReference(webPubSub);
```

The `AddAzureWebPubSub` method will read connection information from the AppHost's configuration (for example, from "user secrets") under the `ConnectionStrings:wps` config key. The `WithReference` method passes that connection information into a connection string named `wps` in the `MyService` project. In the _Program.cs_ file of `MyService`, the connection can be consumed using:

```csharp
builder.AddAzureWebPubSub("wps", "your_hub_name");
```

Notice both methods at the top and bottom are builder.AddAzureWebPubSub. We received feedback that this is confusing to name the methods the same.

So to be consistent with the rest of the components, we should rename this method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe AddAzureWebPubSubServiceClient?

I'd be fine with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option is AddAzureWebPubSubHub since the method also requires hub parameter, how do you feel about this one @eerhardt ? I am a little concerned about appending ServiceClient since that would limit this method to add ServiceClient, there is an aspnetcore middleware https://github.com/Azure/azure-sdk-for-net/tree/main/sdk/webpubsub/Microsoft.Azure.WebPubSub.AspNetCore that I'd like to add when later the ideal workflow #2495 (comment) is possible. This middleware is to help handle the traffic in the upstream server from the client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option is AddAzureWebPubSubHub since the method also requires hub parameter, how do you feel about this one @eerhardt ?

I'm fine with appending "hub" if that's what you feel is best. Note that other components use the term "Client" and don't necessarily mean the DI service is named "Client". For example, RabbitMQ, NATS, and Redis all have a "connection" object, but their extension methods are named "AddXXXClient", meaning this app is going to be a "client" of the XXX server.

The main point here is that we don't want the same names between the AppHost extension method and the Component extension method.

there is an aspnetcore middleware

Note that the middleware can't go in this method because it is too early in the app's lifecycle. Middleware get added after the builder is "built" into an app, and are added to the "app" object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to ...Hub considering WebPubSub plays more like a server role.

Note that the middleware can't go in this method because it is too early in the app's lifecycle. Middleware get added after the builder is "built" into an app, and are added to the "app" object.

Understood. AddAzureWebPubSubHub would add necessary dependencies, and users can call app.UseWebPubSubHub in their middleware if builder.AddAzureWebPubSubHub.

@davidfowl
Copy link
Member

davidfowl commented Mar 21, 2024

There's a big split coming here #3062

cc @mitchdenny

@mitchdenny
Copy link
Member

The split is merged so this PR will need to react to that @vicancy

@vicancy
Copy link
Contributor Author

vicancy commented Apr 1, 2024

Waiting for this Azure/azure-sdk-for-net#43027 to be integrated into Azure.Provisioning

@vicancy vicancy force-pushed the wps2 branch 2 times, most recently from 9883d2f to 6a05919 Compare May 21, 2024 08:45
/// <param name="builder">The <see cref="IDistributedApplicationBuilder"/>.</param>
/// <param name="name">The name of the resource. This name will be used as the connection string name when referenced in a dependency.</param>
/// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<AzureWebPubSubResource> AddAzureWebPubSub(this IDistributedApplicationBuilder builder, string name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get some examples added here e.g.:

/// <example>
/// Brief summary of example
/// <code>
/// ....
/// </code>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and on the other extension methods.

Copy link
Contributor Author

@vicancy vicancy May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is <example> a supported tag in triple slash comment? looks like VS intellisense does not support it.

@mitchdenny
Copy link
Member

This is looking pretty good. General feedback is about making sure you put some good examples in the /// docs just to make it easy for folks.

@mitchdenny
Copy link
Member

One more thing. What is the story here around the .NET client side of this experience? If someone wants to use WebPubSub in a S2S scenario, do we want any observability from WebPubSubClient to the service?

@vicancy
Copy link
Contributor Author

vicancy commented May 24, 2024

Hi @mitchdenny I changed to always use hubName as the serviceKey, please check if you have any concerns about the change.

For the websocket WebPubSubClient working in S2S scenario, it is a really good question, and it is indeed a valid scenario, but I'd prefer hold adding it into Aspire to avoid confusions on too many components in WebPubSub for now until we have a good story/definition/good-practices for the S2S scenario using WebPubSubClient.

@mitchdenny
Copy link
Member

@eerhardt wanted to get your input here around the keyed DI injection of the client where you might have a single webpubsub resource but multiple hubs. Should we be providing the option for people to specify the connection string name, hub name AND the key that is used?

eerhardt
eerhardt previously approved these changes May 31, 2024
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good and is ready to be merged from my stand-point.
It would be good to get once last look from @mitchdenny.

Thanks @vicancy for sticking with this!

@eerhardt eerhardt dismissed their stale review June 5, 2024 14:05

Changes were made after the review.

mitchdenny and others added 2 commits June 6, 2024 11:40
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution @vicancy!

@eerhardt eerhardt enabled auto-merge (squash) June 7, 2024 21:48
@eerhardt eerhardt merged commit 94e36fd into dotnet:main Jun 7, 2024
8 checks passed
@vicancy vicancy deleted the wps2 branch June 11, 2024 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-components Issues pertaining to Aspire Component packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants