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

feat: Add PerformanceApi to SearchEngine #2720

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import 'cozy-client'
import { CozyLink } from 'cozy-client'
import { CozyLink, defaultPerformanceApi } from 'cozy-client'

Check failure on line 2 in packages/cozy-dataproxy-lib/src/search/@types/cozy-client.d.ts

View workflow job for this annotation

GitHub Actions / Build and publish

'defaultPerformanceApi' is defined but never used
Copy link
Contributor

Choose a reason for hiding this comment

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

This import does not look necessary

import {
CozyClientDocument,
QueryOptions,
Expand Down
41 changes: 38 additions & 3 deletions packages/cozy-dataproxy-lib/src/search/SearchEngine.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import FlexSearch from 'flexsearch'

import CozyClient from 'cozy-client'
import CozyClient, { defaultPerformanceApi } from 'cozy-client'
import type { PerformanceAPI } from 'cozy-client/types/performances/types'
import Minilog from 'cozy-minilog'
import { RealtimePlugin } from 'cozy-realtime'

Expand Down Expand Up @@ -48,10 +49,12 @@ export class SearchEngine {
searchIndexes: SearchIndexes
debouncedReplication: () => void
isLocalSearch: boolean
performanceApi: PerformanceAPI

constructor(client: CozyClient) {
constructor(client: CozyClient, performanceApi?: PerformanceAPI) {
this.client = client
this.searchIndexes = {} as SearchIndexes
this.performanceApi = performanceApi ?? defaultPerformanceApi
Copy link
Contributor

Choose a reason for hiding this comment

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

j'ai pas suivi pourquoi on importe 2 API et qu'on doit les charger toutes les 2 ? Est-ce que cozy-client ne devrait pas retourner un performanceOrDefaultApi par exemple qui porterait cette responsabilité ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus d'infos : https://github.com/cozy/cozy-client/blob/master/docs/performances.md

Est-ce que cozy-client ne devrait pas retourner un performanceOrDefaultApi

C'est le cas, cf L3. Mais tu peux vouloir une autre implem que celle par défaut, selon si tu es AA ou Web

Copy link
Contributor

@JF-Cozy JF-Cozy Feb 3, 2025

Choose a reason for hiding this comment

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

Ce n'est pas ça que j'avais pas compris, en fait j'ai mal lu on n'importe pas 2 API, le deuxième import est un type :) Et dans la condition la première vient des Props, et non de cozy-client


this.isLocalSearch = !!getPouchLink(this.client)
log.info('Use local data on trusted device: ', this.isLocalSearch)
Expand All @@ -76,6 +79,9 @@ export class SearchEngine {
if (!this.client) {
return
}

const markName = this.performanceApi.mark('indexDocuments')

// Create search indexes by querying docs, either locally or remotely
for (const doctype of SEARCHABLE_DOCTYPES) {
const searchIndex = await this.indexDocsForSearch(
Expand All @@ -90,6 +96,11 @@ export class SearchEngine {
// Use replication events to have up-to-date search indexes, based on local data
this.indexOnReplicationEvents()
}

this.performanceApi.measure({
markName: markName,
category: 'Search'
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'search' category might be a bit misleading here, since we are indexing documents. Furthermore, this category name is reused later for the actual search

})
}

indexOnReplicationEvents(): void {
Expand Down Expand Up @@ -337,6 +348,9 @@ export class SearchEngine {
async indexDocsForSearch(
doctype: keyof typeof SEARCH_SCHEMA
): Promise<SearchIndex | null> {
const markeNameIndex = this.performanceApi.mark(
`indexDocsForSearch ${doctype}`
)
const searchIndex = this.searchIndexes[doctype]
const startIndexing = performance.now()

Expand All @@ -345,6 +359,11 @@ export class SearchEngine {
// First creation of search index
index = await this.initialIndexation(doctype)
if (!index) {
this.performanceApi.measure({
markName: markeNameIndex,
measureName: `${markeNameIndex} initial indexation`,
category: 'Search'
})
return null
}
} else {
Expand All @@ -358,6 +377,13 @@ export class SearchEngine {
log.debug(
`Indexing ${doctype} took ${(endIndexing - startIndexing).toFixed(2)} ms`
)

this.performanceApi.measure({
markName: markeNameIndex,
measureName: `${markeNameIndex} incremental indexation`,
category: 'Search'
})

return index
}

Expand All @@ -368,6 +394,8 @@ export class SearchEngine {
return []
}

const markeNameIndex = this.performanceApi.mark('search')

const allResults = this.searchOnIndexes(query, options?.doctypes)
const dedupResults = this.deduplicateAndFlatten(allResults)
const sortedResults = this.sortSearchResults(
Expand All @@ -381,7 +409,14 @@ export class SearchEngine {
const normalizedRes = normalizeSearchResult(this.client, res, query)
normResults.push(normalizedRes)
}
return normResults.filter(res => res.title)
const output = normResults.filter(res => res.title)

this.performanceApi.measure({
markName: markeNameIndex,
category: 'Search'
})

return output
}

searchOnIndexes(
Expand Down
Loading