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

Azure cosmosdb support in aspire #359

Merged
merged 92 commits into from
Nov 2, 2023
Merged

Conversation

Pilchie
Copy link
Member

@Pilchie Pilchie commented Oct 17, 2023

No description provided.

@Pilchie
Copy link
Member Author

Pilchie commented Oct 17, 2023

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


namespace Aspire.Hosting.Azure.CosmosDB;

public class CosmosDBConnectionResource(string name, string? connectionString)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate on the difference and the scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

@eerhardt thoughts on this one?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. @mitchdenny - can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. A type that represents a manually supplied connection string.
  2. A type that represents the "container type" ... e.g. a cosmos account.
  3. A type that represents the "contained type" ... e.g. a database.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

/// <summary>
/// A health check for Azure CosmosDB.
/// </summary>
internal sealed class AzureCosmosDBHealthCheck : IHealthCheck
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that makes sense. Most databases are TCP based but we still have a health check for them...

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@davidfowl davidfowl Oct 27, 2023

Choose a reason for hiding this comment

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

Then that client API should itself have a way to report health status right?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

So a user would never need to care about network connectivity between the client and cosmos?


private static void WriteCosmosDBDatabaseToManifest(Utf8JsonWriter jsonWriter, CosmosDatabaseResource cosmosDatabase)
{
jsonWriter.WriteString("type", "azure.data.cosmos.server.v1");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mitchdenny mitchdenny Nov 2, 2023

Choose a reason for hiding this comment

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

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:

"postgres": {
"type": "postgres.server.v1"
},
"catalogdb": {
"type": "postgres.database.v1",
"parent": "postgres"
},

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need to write a provisioner to give reasonable feedback here.

@mitchdenny
Copy link
Member

@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.

@eerhardt eerhardt mentioned this pull request Nov 2, 2023
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.

The runtime Component code looks great. I will log a couple follow up issues, but this is PR looks good for the initial commit.

I've opened the following follow-up issues that can be resolved post-merge:

@eerhardt eerhardt merged commit 0b3fd6e into dotnet:main Nov 2, 2023
5 checks passed
@eerhardt eerhardt deleted the azure-cosmosdb branch November 2, 2023 19:51
eerhardt pushed a commit that referenced this pull request Nov 2, 2023
* 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
@Pilchie
Copy link
Member Author

Pilchie commented Nov 2, 2023

Thanks for all your help everyone!

davidfowl pushed a commit that referenced this pull request Nov 2, 2023
* 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 danmoseley added the area-components Issues pertaining to Aspire Component packages label Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-components Issues pertaining to Aspire Component packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet