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.

Sign up for GitHub

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

Jump to bottom

CosmosDb partitioning strategy causing high cost/low perf, max quota/too many requests #238

Closed
dluc opened this issue Aug 22, 2023 · 3 comments · Fixed by #240
Closed

CosmosDb partitioning strategy causing high cost/low perf, max quota/too many requests #238

dluc opened this issue Aug 22, 2023 · 3 comments · Fixed by #240
Assignees
@crickman
Labels
performance PR: ready to merge PR has been approved by all reviewers, and is ready to merge. sk team issue webapi Pull requests that update .net code

Comments

@dluc
Copy link
Collaborator

dluc commented Aug 22, 2023

  1. chatsessions table is partitioned by ID, but should be partitioned by User and have an index on the ID
  2. chatmessages table is partitioned by ID, but should be partitioned by Chat ID

In the current setup, as these tables grow, CosmosDb takes more and more time to execute these operations:

  1. Load the list of chats for the current user
  2. Load the list of messages for the selected chat

Other than increasing latency, also the incurred cost in RUs (and $) grows, with the risk of hitting Max Quota when autoscale is not enabled.
@crickman crickman added webapi Pull requests that update .net code sk team issue labels Aug 22, 2023
@crickman
Copy link
Contributor

crickman commented Aug 22, 2023 •
edited
Loading

Thank you for reporting this. I've gotten a bit more context from Harleen as well.

Will the customer require a tool to migrate their existing chats?

@crickman
Copy link
Contributor

crickman commented Aug 23, 2023 •
edited
Loading

Linked branch with core fix:

CosmosDB database and container are created during deployment (not by code). The partitioning scheme is defined at this time in scripts/deploy/main.bicep.

If no migration is required (as Devis stated), the containers can simply be deleted and redeployed along with the updated application.

Updating the application without resetting the cosmosdb contains will result in breakage.

If migration pathway is required or some other type of schema-version detection, let's identify and iterate on those cases.

@crickman crickman linked a pull request Aug 23, 2023 that will close this issue
Update partitionKey path definitions for cosmosDatabase containers. #240
Merged
4 tasks
@crickman crickman mentioned this issue Aug 23, 2023
Merged
4 tasks
@crickman
Copy link
Contributor

Draft PR - #240

Following up w/ Tao tomorrow to explore deployment options to realize this change w/ minimal fuss.

@crickman crickman moved this from In progress to In review in Apps & Services Semantic Kernel Aug 23, 2023
@crickman crickman added the PR: ready to merge PR has been approved by all reviewers, and is ready to merge. label Aug 25, 2023
@crickman crickman moved this from In review to Ready in Apps & Services Semantic Kernel Aug 25, 2023
@crickman crickman moved this from Ready to In review in Apps & Services Semantic Kernel Aug 25, 2023
github-merge-queue bot pushed a commit that referenced this issue Aug 28, 2023
@crickman
Update partitionKey path definitions for cosmosDatabase containers. (#…
679bd07 
…240)

### Motivation and Context
Customer issue reported in
#238 related to
partition scheme in cosmosdb containers.

### Description
Update partition-key path definition for `ChatMessage`,
`ChatPartitipant`, and `MemorySource`.

Existing deployments will continue to work in their current mode.

CosmosDB containers must be removed and redeployed to realize the
peformance update related to the partition scheme.

### Contribution Checklist
- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [Contribution
Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@crickman crickman

Labels
performance PR: ready to merge PR has been approved by all reviewers, and is ready to merge. sk team issue webapi Pull requests that update .net code
Projects
No open projects
Apps & Services Semantic Kernel
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update partitionKey path definitions for cosmosDatabase containers. microsoft/chat-copilot
2 participants
@dluc @crickman