-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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' | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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( | ||
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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() | ||
|
||
|
@@ -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 { | ||
|
@@ -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 | ||
} | ||
|
||
|
@@ -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( | ||
|
@@ -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( | ||
|
There was a problem hiding this comment.
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