-
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?
Conversation
`cozy-client` has been upgraded to `53.1.0` in order to retrieve typings for the Performance API Related PR: cozy/cozy-client#1574
9cad53d
to
1e13480
Compare
We want to add some metrics to understand where time is consumed All measurement are made through the PerformanceApi helper that can be injected from consuming app Related PR: cozy/cozy-client#1574
1e13480
to
2727210
Compare
In cozy/cozy-libs#2720 we added PerformanceAPI in the SearchEngine This commit reflect the changes to the SearchEngine constructor Related PR: cozy/cozy-libs#2720
In cozy/cozy-libs#2720 we added PerformanceAPI in the SearchEngine This commit reflect the changes to the SearchEngine constructor Related PR: cozy/cozy-libs#2720
this.client = client | ||
this.searchIndexes = {} as SearchIndexes | ||
this.performanceApi = performanceApi ?? defaultPerformanceApi |
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.
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é ?
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.
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
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.
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
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.
Some small remarks, but looks good!
|
||
this.performanceApi.measure({ | ||
markName: markName, | ||
category: 'Search' |
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.
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
@@ -1,5 +1,5 @@ | |||
import 'cozy-client' | |||
import { CozyLink } from 'cozy-client' | |||
import { CozyLink, defaultPerformanceApi } from 'cozy-client' |
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
We want to add some metrics to understand where time is consumed
All measurement are made through the PerformanceApi helper that can be injected from consuming app
Measurements have been added for the
indexDocuments()
duration (global and per doctype) and thesearch()
durationRelated PR: cozy/cozy-client#1574