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

Improve document logs table perfomance #472

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

andresgutgon
Copy link
Contributor

@andresgutgon andresgutgon commented Oct 21, 2024

What?

Doing something similar to what we did with evaluation results, we filter by page and object the rows to be shown in the aggregated table of logs. This should improve performance

TODO

  • Fix test
  • Use it in next Page route for document logs
  • Use it in next API route for document logs
  • Use it in websockets notify document log created
  • QA

@andresgutgon andresgutgon added the 🚧 wip Work in progress label Oct 21, 2024

import { Commit } from '../../browser'
import { database } from '../../client'
import {
createDocumentLogQuery,
getCommitFilter,
} from './_createDocumentLogQuery'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 🔥 this archive. Now all the "with metadata" things are in this file. I left a FIXME in evaluations to do the same

}) {
const byDocumentUuid = documentUuid
? eq(scope.documentUuid, documentUuid)
: sql`1 = 1`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If documentUuid is not passed we don't care and use 1 = 1. This case is covering /evaluations playground where we get a documentLog.resolvedContent for the evaluation variables

We don't have the scope of documentUuid

Copy link
Collaborator

@geclos geclos Oct 22, 2024

Choose a reason for hiding this comment

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

hmm don't get it is this gonna match all document logs a pelo? in what scenario do we care about document logs without filtering by document uuid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the playground we can import any document log from any project and any document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const documentLogUuid = uuid()
let providerLogs: ProviderLog[] = []

if (!skipProviderLogs) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make provider logs creation optional on this factory

@andresgutgon andresgutgon removed the 🚧 wip Work in progress label Oct 22, 2024
@andresgutgon andresgutgon force-pushed the fix/document-logs-query-pagination branch from 00a51ab to 22655ba Compare October 22, 2024 11:07
Doing something similar to what we did with evaluation results where we
filter by page and object the rows to be shown in the aggregated table
of logs. This should improve performance
@andresgutgon andresgutgon force-pushed the fix/document-logs-query-pagination branch from 22655ba to c6a5943 Compare October 22, 2024 11:38
Copy link
Collaborator

@geclos geclos left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +123 to 146
export async function computeDocumentLogsWithMetadataCount(
{
workspaceId,
documentUuid,
draft,
}: {
workspaceId: number
documentUuid: string
draft?: Commit
},
db = database,
) {
const { scope } = getRepositoryScopes(workspaceId, db)

const countList = await db
.select({
count: sql<number>`count(*)`.as('total_count'),
})
.from(scope)
.innerJoin(commits, eq(commits.id, scope.commitId))
.where(getCommonQueryConditions({ scope, documentUuid, draft }))

return countList?.[0]?.count ? Number(countList[0].count) : 0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not "with metadata", could easily be moved to DocumentLogsRepository. Although I get that you want to keep the getCommonQueryConditions

@andresgutgon andresgutgon merged commit e5b1ca0 into main Oct 22, 2024
3 checks passed
@andresgutgon andresgutgon deleted the fix/document-logs-query-pagination branch October 22, 2024 12:23
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants