Add Cosmos playground code. #1697

mitchdenny · 2024-01-18T12:36:04Z

This PR contains an end to end Cosmos app which can be used to validate Cosmos scenarios whilst working in our repo. It creates databases (when using emulator), creates containers, and inserts and gets data.

@eerhardt wouldn't mind getting your thoughts on this. This PR does:

Adds a playground project independent of the eShopLite sample for experimenting with Cosmos app model and componentry.
As I was writing it, I found a bug with the emulator usage so this PR fixes that (might end up splitting that off onto a separate PR depending on your thoughts on the broader PR.
Modifies the Aspire Cosmos component to look for a Database=x; value on the Cosmos connection string, and if it is present inject the Database instance that matches that value.

One of the issues with the end to end scenario for Cosmos is that the CosmosClient isn't a client for a particular database, it is a client for the entire Cosmos account. Which means that even if someone does this in the app model:

builder.AddAzureCosmosDB("account").AddDatabase("db");

The still need to provide some kind of configuration value so that they code knows what database to talk to. The change I made registers the database instance (incomplete, but enough to actually run and work). In the playground sample itself I also register the container via keyed DI. This means that developers only really need to reference the keyed container by name and then they can perform operations.

I think registering containers as part of the Aspire component is a bridge too far, but registering the database, if it is provided on the connection string would be really useful. It might also be useful to make it an option to determine whether or not to create the database dynamically as well.

Microsoft Reviewers: Open in CodeFlow

mitchdenny · 2024-01-18T12:36:28Z

@Pilchie your thoughts about this approach from a Cosmos perspective?

eerhardt · 2024-01-18T12:56:50Z

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/CosmosEndToEnd.ApiService.csproj

@@ -0,0 +1,15 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">


I don't think we should use the word "Playgrounds" and this shouldn't be under the src folder - which is for product code.

I'd rather call these "TestProjects", even if they are manual tests.

eerhardt · 2024-01-18T12:58:38Z

src/Aspire.Hosting.Azure/AzureCosmosDBDatabaseResource.cs

    /// </summary>
    /// <returns>The connection string to use for this database.</returns>
-    public string? GetConnectionString() => ConnectionString;
+    public string? GetConnectionString() => ConnectionString ?? $"{Parent.GetConnectionString()}Database={Name};"; // HACK: Will go away when we get rid of Azure Provisioner package.


What exactly will go away? If we are going to get rid of the Azure Provisioner package, do we even need this hack now?

eerhardt · 2024-01-18T13:00:01Z

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireAzureCosmosDBExtensions.cs

        {
            builder.Services.AddSingleton(_ => ConfigureDb());
+
+            var csBuilder = new DbConnectionStringBuilder();


a) why are we only doing this for the non-EF component?
b) why are we only doing this for the non-Keyed DI path?

The PR is incomplete. I was just getting something working to facilitate conversation.

eerhardt · 2024-01-18T13:04:04Z

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireAzureCosmosDBExtensions.cs

+
+            if (csBuilder.TryGetValue("Database", out var databaseName))
+            {
+                builder.Services.AddSingleton<Database>(sp =>


This feels really weird to conditionally add a DI service based on the format used in the connection string.

The more I think about this, the more I don't think we should be doing this in the Aspire component. If someone wants to make a separate NuGet package to do these kinds of convenience operations, that would be OK, but Aspire is more geared towards "cloud" concerns like telemetry, config, health checks, etc. Its goal isn't trying to make the underlying libraries easier to use. It registers the root concept in DI and then gets out of the user's way and let's them decide how to structure their app.

I think this is fair. Ideally Cosmos libraries would do a lot of this for us.

eerhardt · 2024-01-18T13:09:48Z

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireAzureCosmosDBExtensions.cs

+            var csBuilder = new DbConnectionStringBuilder();
+            csBuilder.ConnectionString = settings.ConnectionString;
+
+            if (csBuilder.TryGetValue("Database", out var databaseName))


Is "Database" a valid connection string property? I only see these 2:

https://github.com/Azure/azure-cosmos-dotnet-v3/blob/431c6f224747e463bed0475083d5e0d7c1afaee2/Microsoft.Azure.Cosmos/src/CosmosClientOptions.cs#L52-L53

Would CosmosDB start throwing if it finds unrecognized properties in the connection string?

cc @Pilchie @sourabh1007

I don't think it throws, it will ignore other properties...you can run a quick test to check it out.

It is not. Cosmos DB Connection String only has endpoint + Key. Users do not specify the DB on the connection string. Also, this would not work if the authentication method is AAD?

I'm actually going to propose that Cosmos start accepting a connection string with segments even for AAD auth. The reason is being able to encode extra options like this.

mitchdenny · 2024-01-18T13:58:13Z

@eerhardt i should have marked this draft. It's not fully implemented the intention was to trigger a conversation about things we could do to make the Cosmos experience better.

EF support is slightly different anyway because database creation can be triggered in EF along with container creation.

Database= is not part of the standard Cosmos connection string.

Pilchie · 2024-01-18T18:26:18Z

Also tagging @kirankumarkolli and @ealsur for thoughts.

ealsur · 2024-01-18T18:34:37Z

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireAzureCosmosDBExtensions.cs

+                builder.Services.AddSingleton<Database>(sp =>
+                {
+                    var client = sp.GetRequiredService<CosmosClient>();
+                    var database = client.CreateDatabaseIfNotExistsAsync(databaseName.ToString()).Result.Database;


Suggested change

var database = client.CreateDatabaseIfNotExistsAsync(databaseName.ToString()).Result.Database;

var database = client.GetDatabase(databaseName);

Doing a blocking metadata async call sounds not ideal? Also, what if the app interacts with multiple databases?

If an app interacts with multiple databases, then the developer using Aspire would probably have wired up their app model a little differently and this code wouldn't kick in.

ealsur · 2024-01-18T18:36:11Z

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs

+builder.Services.AddKeyedSingleton<Container>("entries", (sp, _) =>
+{
+    var db = sp.GetRequiredService<Database>();
+    var container = db.CreateContainerIfNotExistsAsync("entries", "/sessionId").Result.Container;


Is there a way to avoid the blocking call?

This is occurring in DI wire up which isn't async. It may be that we just don't do this database injection, and we leave all this to the developer, but my goal was to try and make it as seamless as possible. But perhaps this should be a Cosmos SDK responsibility as @eerhardt mentions above.

ealsur · 2024-01-18T18:37:00Z

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireAzureCosmosDBExtensions.cs

+
+            if (csBuilder.TryGetValue("Database", out var databaseName))
+            {
+                builder.Services.AddSingleton<Database>(sp =>


What if instead is a keyedSingleton that takes the DB Name? That way the caller can obtain the Database instance for the name they need?

Or a KeyedSingleton that takes dbName + containerName and return a Container from client.GetContainer().

ealsur

I like the idea, some thoughts:

Customers don't interact with the Database, but the Container, we could have a KeyedSingleton that takes dbname + container and returns a Container instance from return client.GetContainer(dbName, container).
Avoid CreateIfNotExists in registrations:
1. They would be blocking calls
2. They don't work if AAD is used for auth
3. It consumes the account metadata RU
4. Creating these resources is normally something that is done outside of the application scope, like creating a Database and Tables in SQL Server. They require cost planning and proper configuration. Auto-magically creating them for the customer might not be ideal.

Pilchie · 2024-01-18T18:42:53Z

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs

+    await container.CreateItemAsync(new Entry(Guid.NewGuid().ToString(), sessionId)).ConfigureAwait(false);
+
+    var entries = new List<Entry>();
+    var iterator = container.GetItemQueryIterator<Entry>(requestOptions: new QueryRequestOptions() { MaxItemCount = 5 });


@ealsur - will this use query serialization? If so, it's not going to use the S.T.J serializer until Azure/azure-cosmos-dotnet-v3#4138 ships and your serializer is updated with the new API

Queries work well with Custom Serializer. What does not work well is LINQ Queries

This call here is effectively a ReadFeed operation (a query without any query text)

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs

kirankumarkolli · 2024-01-19T16:10:13Z

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs

+                         {
+                             // Default serializer for Cosmos V3 client is JSON.NET, this changes
+                             // us to use S.T.J for this playground.
+                             clientOptions.Serializer = new StjSerializer(new System.Text.Json.JsonSerializerOptions());


Later follow-up: With next release of SDK there will be a new type for serialization to be implemented to handle LINQ queries.

ealsur · 2024-01-19T16:35:09Z

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireAzureCosmosDBExtensions.cs

        if (serviceKey is null)
        {
            builder.Services.AddSingleton(_ => ConfigureDb());
+


In line 104, please remove clientOptions.LimitToEndpoint = true; this should never be set in any general content, even in the case of the emulator, users might copy this and think this is ok.

I know this came from an official doc (I found the source) and I also corrected it there.

If I remove this, it seems to stop the code working. It just hangs.

It is not hanging, it probably is facing an HttpRequestException and retrying. This flag turns off the retries. HttpRequestException is normally related to some connectivity issue with the endpoint being passed. Was it localhost?

mitchdenny · 2024-01-19T22:46:10Z

Customers don't interact with the Database, but the Container,

We only model down to the database in the Aspire App model, so registering databases is where I'd probably step off. In the playground sample in the PR you can see that i go a little further and register containers. But that is kind of a last mile thing that would be in end-developer code.

…t/aspire into mitchdenny/add-playgrounds

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs

mitchdenny · 2024-01-21T04:43:15Z

@davidfowl @eerhardt this is just playground code sans some of the ideas that I shared above. Want to get this in so I can use it with the AZD team to validate end to end scenarios.

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ServiceDefaults/Extensions.cs

mitchdenny · 2024-01-21T09:12:00Z

Restarting the build job. Looks like the agent got ripped out from under us.

Cosmos playground.

26a8c44

mitchdenny requested a review from eerhardt January 18, 2024 12:36

ghost added the area-integrations Issues pertaining to Aspire Integrations packages label Jan 18, 2024

mitchdenny requested a review from Pilchie January 18, 2024 12:36

mitchdenny self-assigned this Jan 18, 2024

mitchdenny added this to the future (would take a change but work not scheduled) milestone Jan 18, 2024

eerhardt reviewed Jan 18, 2024

View reviewed changes

mitchdenny mentioned this pull request Jan 18, 2024

Rename 'samples' to 'playground' #1689

Merged

eerhardt reviewed Jan 18, 2024

View reviewed changes

ealsur reviewed Jan 18, 2024

View reviewed changes

Pilchie reviewed Jan 18, 2024

View reviewed changes

davidfowl reviewed Jan 18, 2024

View reviewed changes

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs Outdated Show resolved Hide resolved

mitchdenny marked this pull request as draft January 18, 2024 22:17

kirankumarkolli reviewed Jan 19, 2024

View reviewed changes

ealsur reviewed Jan 19, 2024

View reviewed changes

mitchdenny added 3 commits January 20, 2024 09:52

Merge branch 'main' into mitchdenny/add-playgrounds

aef819f

Remove DI experiments, and just leave stock standard playground.

e4e706c

Merge branch 'mitchdenny/add-playgrounds' of https://github.com/dotne…

b2b3c40

…t/aspire into mitchdenny/add-playgrounds

davidfowl reviewed Jan 21, 2024

View reviewed changes

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs Outdated Show resolved Hide resolved

Remove STJ serializer.

fbba8cf

mitchdenny modified the milestones: future (would take a change but work not scheduled), preview 3 (Feb) Jan 21, 2024

mitchdenny changed the title ~~Some Cosmos ideas.~~ Add Cosmos playground code. Jan 21, 2024

mitchdenny added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 21, 2024

mitchdenny marked this pull request as ready for review January 21, 2024 04:42

mitchdenny requested review from Pilchie and davidfowl January 21, 2024 04:42

Suppres CA2007 for playground code.

db955d7

davidfowl reviewed Jan 21, 2024

View reviewed changes

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 21, 2024

View reviewed changes

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 21, 2024

View reviewed changes

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 21, 2024

View reviewed changes

src/Playgrounds/CosmosEndToEnd/CosmosEndToEnd.ServiceDefaults/Extensions.cs Show resolved Hide resolved

mitchdenny added 2 commits January 21, 2024 16:20

Remove session ID and unneeded configreawaits.

56b82b7

Making service defaults reusable across playground projects.

6798647

davidfowl approved these changes Jan 21, 2024

View reviewed changes

mitchdenny enabled auto-merge (squash) January 21, 2024 05:34

mitchdenny merged commit 0c2daa5 into main Jan 21, 2024

mitchdenny deleted the mitchdenny/add-playgrounds branch January 21, 2024 09:24

github-actions bot locked and limited conversation to collaborators Apr 25, 2024

	var database = client.CreateDatabaseIfNotExistsAsync(databaseName.ToString()).Result.Database;
	var database = client.GetDatabase(databaseName);

Add Cosmos playground code. #1697

Add Cosmos playground code. #1697

Uh oh!

Conversation

mitchdenny commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

mitchdenny commented Jan 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchdenny commented Jan 18, 2024

Uh oh!

Pilchie commented Jan 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ealsur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchdenny commented Jan 19, 2024

Uh oh!

Uh oh!

mitchdenny commented Jan 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mitchdenny commented Jan 21, 2024

Uh oh!

Reviewers

Assignees

mitchdenny commented Jan 18, 2024 •

edited

Loading

ealsur left a comment •

edited

Loading