Azure cosmosdb support in aspire#359
Azure cosmosdb support in aspire#359eerhardt merged 92 commits intomicrosoft:mainfrom Pilchie:azure-cosmosdb
Conversation
|
Note - I know a bunch of stuff is likely needed to clean this up, but would love to start getting some feedback on it. @kirankumarkolli - FYI |
src/Aspire.Hosting/CosmosDB/AzureCosmosDBCloudApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/CosmosDB/AzureCosmosDBCloudApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Aspire.Hosting.Azure.CosmosDB; | ||
|
|
||
| public class CosmosDBConnectionResource(string name, string? connectionString) |
There was a problem hiding this comment.
We'll probably need to have CosmosDbAccountResource as well I suspect to represent a resource that we want the tool reading the manifest to interpret.
There was a problem hiding this comment.
Can you elaborate on the difference and the scenario?
There was a problem hiding this comment.
When we are dealing with resource types like this where there is a child resource involved we'll typically end up dealing with three types:
- A type that represents a manually supplied connection string.
- A type that represents the "container type" ... e.g. a cosmos account.
- A type that represents the "contained type" ... e.g. a database.
There was a problem hiding this comment.
Per above, I think there is actually another level of nesting for Cosmos DB Account -> Database -> Container. However, the ConnectionString only uniquely identifies the Account, so I'm not sure how to fit that into Aspire right now.
There was a problem hiding this comment.
Oh, and to make this tricky - for the EF version, the containers it uses will have properties that depend on the code that sets up the (for example what the partition key is).
src/Components/Aspire.Azure.EntityFrameworkCore.CosmosDB/README.md
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Azure.EntityFrameworkCore.CosmosDB/README.md
Outdated
Show resolved
Hide resolved
...re.EntityFrameworkCore.CosmosDB.Tests/Aspire.Azure.EntityFrameworkCore.CosmosDB.Tests.csproj
Show resolved
Hide resolved
| /// <summary> | ||
| /// A health check for Azure CosmosDB. | ||
| /// </summary> | ||
| internal sealed class AzureCosmosDBHealthCheck : IHealthCheck |
There was a problem hiding this comment.
Can we use https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/tree/master/src/HealthChecks.CosmosDb instead of writing our own? We have been using these OSS libraries to help consolidate efforts. If these health checks don't meet our needs, our intention is to submit PRs to make them better.
There was a problem hiding this comment.
I talked to the team about this a bit after I wrote this. For CosmosDB in direct mode (the default for .NET), health checks don't really make sense. That's because the SDK has direct TCP connections to the machines hosting the replicas of each data partition, and each of those could fail independently.
Those style of healthchecks only maintain health of the gateway connection, and do drain some of your provisioned throughput, or else cost you directly in serverless.
Thinking of removing this and marking the component as "won't support health checks".
There was a problem hiding this comment.
I'm not sure that makes sense. Most databases are TCP based but we still have a health check for them...
There was a problem hiding this comment.
They may be TCP based, but to a single host over a single connection. CosmosDB is 4 hosts per partition, each a separate connection. You can't have a health-check that ensures all of those are constantly available without also draining the RU budget you have an ending up throttling yourself.
There was a problem hiding this comment.
Then that client API should itself have a way to report health status right?
There was a problem hiding this comment.
Define health then? If the gateway has a health API that reports healthy but there is a network issue preventing you from getting to the replicas what happens? The team's position here is that the way to know whether things are healthy is to monitor metrics.
There was a problem hiding this comment.
So a user would never need to care about network connectivity between the client and cosmos?
src/Components/Aspire.Azure.CosmosDB/Aspire.Azure.CosmosDB.csproj
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Azure.CosmosDB/Aspire.Azure.CosmosDB.csproj
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Azure.CosmosDB/AspireAzureCosmosDBExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Azure.CosmosDB/AspireAzureCosmosDBExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private static void WriteCosmosDBDatabaseToManifest(Utf8JsonWriter jsonWriter, CosmosDatabaseResource cosmosDatabase) | ||
| { | ||
| jsonWriter.WriteString("type", "azure.data.cosmos.server.v1"); |
There was a problem hiding this comment.
Is this type name correct? If this resource has a parent, then the server should be the parent resource, and this should be database or collection or something like that.
There was a problem hiding this comment.
No idea. I have no intuition about what should be in the manifest, or what it's used for. Any docs/specs/etc that would help me reason about it would be great.
There was a problem hiding this comment.
Take a look at samples/eshoplite/aspire-manifest.json. We have a postgres server and postgres database. The database has a parent field that points to the server indicating a relationship between the two:
The idea is that the tool that processes the manifest can use this information to pre-create the database for the application because typically code running on the app server wouldn't have permissions to perform these kinds of operations.
There was a problem hiding this comment.
So in this case the hierarchy is:
- Azure Subscription
- one or more Azure Cosmos DB Accounts
- one or more Databases
- one or more Containers
It would be ideal to be able to model all the way down to the Container level and use the control plane SDK to provision these. (for example, Managed Identities in the data plane SDK don't support control plane operations, though they do with keys). Today it's a bit of a mix. You need a connection string to connect to an account, but then you need a name to get the database. That's why there is that weird extra parameter to the AddCosmosDbContext method, since EF needs to know the database name to use.
I initially tried to model this better, but couldn't figure out how to flow the connection string from the Connection resource (which should maybe be renamed Account?), as well. I'd love some advice on how to model this properly, though maybe we could do that in a separate PR?
There was a problem hiding this comment.
I think we'll need to write a provisioner to give reasonable feedback here.
|
@eerhardt @davidfowl @Pilchie I've just merged main into this branch so that it has picked up all the latest changes (there was quite a bit of churn on main in the last 24 hours). Now I think we have a baseline we can look at for landing this PR. |
* Initial addition of CosmosDB support, based on SqlServer * Remove Healthchecks support from CosmosDB EF Component * Cleanup connection string handling in Cosmos EF * Cleanup connection string handling in Cosmos component * Update CosmosDB package to get OTel support * Use the parent name for the connection * Udpate manifest strings * Add CosmosDB components to Progress and Telemetry * Rename CosmosDB components to Aspire.Azure.Data.Cosmos[.EntityFrameworkCore] * Rename options -> settings * Rename Cosmos Components to follow naming guidelines * Update to CosmosDB preview package and pin to get OpenTelemetry support * Update comments and add Keyed DI to Aspire.Microsoft.Azure.Cosmos * Add log categories to Cosmos Component schemas * Add basic support for CosmosClientOptions (no IConfiguration binding yet) * Remove healthchecks support from CosmosDB Component * Add README for Aspire.Microsoft.Azure.Cosmos * Add README for Aspire.Microsoft.EntityFrameworkCore.Cosmos, and rename a couple of things * Update config schema to be nested for Aspire.Microsoft.EntityFramework.Cosmos and Aspire.Microsoft.Azure.Cosmos * Rename AzureDataCosmosSettings -> AzureCosmosDBSettings * Update Aspire_Components_Progress.md * Add PackageTags, Descriptions, and Icons * Add AccountEndpoint to ConfigurationScheama.json * Fix DB context builder config * Add xml doc comments for CosmosDB hosting methods and types * Move Cosmos DB hosting to Aspire.Hosting.Azure * Update manifest type names * Respond to PR feedback
|
Thanks for all your help everyone! |
* Initial addition of CosmosDB support, based on SqlServer * Remove Healthchecks support from CosmosDB EF Component * Cleanup connection string handling in Cosmos EF * Cleanup connection string handling in Cosmos component * Update CosmosDB package to get OTel support * Use the parent name for the connection * Udpate manifest strings * Add CosmosDB components to Progress and Telemetry * Rename CosmosDB components to Aspire.Azure.Data.Cosmos[.EntityFrameworkCore] * Rename options -> settings * Rename Cosmos Components to follow naming guidelines * Update to CosmosDB preview package and pin to get OpenTelemetry support * Update comments and add Keyed DI to Aspire.Microsoft.Azure.Cosmos * Add log categories to Cosmos Component schemas * Add basic support for CosmosClientOptions (no IConfiguration binding yet) * Remove healthchecks support from CosmosDB Component * Add README for Aspire.Microsoft.Azure.Cosmos * Add README for Aspire.Microsoft.EntityFrameworkCore.Cosmos, and rename a couple of things * Update config schema to be nested for Aspire.Microsoft.EntityFramework.Cosmos and Aspire.Microsoft.Azure.Cosmos * Rename AzureDataCosmosSettings -> AzureCosmosDBSettings * Update Aspire_Components_Progress.md * Add PackageTags, Descriptions, and Icons * Add AccountEndpoint to ConfigurationScheama.json * Fix DB context builder config * Add xml doc comments for CosmosDB hosting methods and types * Move Cosmos DB hosting to Aspire.Hosting.Azure * Update manifest type names * Respond to PR feedback Co-authored-by: Kevin Pilch <me@pilchie.com>
danmoseley
added
the
area-integrations
15 participants
No description provided.