-
-
Notifications
You must be signed in to change notification settings - Fork 218
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(#9489): collect telemetry for offline freetext searching #9525
base: master
Are you sure you want to change the base?
Conversation
This is an example of what a telemetry sample with this new metric would look like {
"_id": "telemetry-2024-10-21-chw-0aadf69e-aa28-4504-ad39-22790755f763",
"_rev": "1-0fc9e7509e3898e2eb5669bd1f0dcbaa",
"type": "telemetry",
"metrics": {
"search_match:contacts_by_freetext:name": {
"sum": 7,
"min": 1,
"max": 1,
"count": 7,
"sumsqr": 7
},
"search_match:contacts_by_freetext:patient_id": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:contacts_by_freetext:patient_id:$value": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:contacts_by_freetext:phone": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:contacts_by_freetext:phone:$value": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:contacts_by_type_freetext:name": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:contacts_by_type_freetext:name:$value": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:contacts_by_type_freetext:phone": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:reports_by_freetext:contact:$value": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:content_type": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:content_type:$value": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:fields.patient_id": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:reports_by_freetext:fields.patient_name": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:fields.patient_short_name": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:fields.patient_short_name_start": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:fields.patient_uuid": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:reports_by_freetext:form": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:form:$value": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:from": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:reports_by_freetext:from:$value": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:patient_age_in_years:$value": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:patient_id:$value": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:reports_by_freetext:patient_name:$value": {
"sum": 2,
"min": 1,
"max": 1,
"count": 2,
"sumsqr": 2
},
"search_match:reports_by_freetext:patient_uuid:$value": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
},
"search_match:reports_by_freetext:t_danger_signs_referral_follow_up:$value": {
"sum": 1,
"min": 1,
"max": 1,
"count": 1,
"sumsqr": 1
}
}
} |
…hared the same scope
…textRequest.params.key`
cd123a8
to
bf001aa
Compare
@@ -191,7 +191,7 @@ const sortByLastVisitedDate = () => { | |||
const makeCombinedParams = (freetextRequest, typeKey) => { | |||
const type = typeKey[0]; | |||
const params = {}; | |||
if (freetextRequest.key) { | |||
if (freetextRequest.params.key) { |
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 is a fix for key:value
queries against the view contacts_by_type_freetext
. Without this, these queries would fail silently and the select2 input would show an infinite loader until a different query would return results. If this has never been noticed before I believe the key:value
search feature isn't used very much...
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.
🤯
selectedContact, | ||
forms, | ||
loadingContent, | ||
loadingSelectedContactReports, | ||
contactsLoadingSummary, | ||
filters, | ||
]) => { | ||
this.recordSearchTelemetry(this.selectedContact, selectedContact, filters); |
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.
We don't need to block the main thread by awaiting for this promise here, it can run in the background just fine
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.
I have seen it suggested that if you have a promise in TS code that you deliberately do not want to await, you preface it with void
:
void this.recordSearchTelemetry(this.selectedContact, selectedContact, filters);
I don't think we have done this much, but maybe it is time to start?
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.
Seems like we should either do that, or leave an inline comment so future us does not think we just forgot the await
...
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.
I've only ever seen the void
operator used in minified JS code to shorten undefined
to void 0
and gain 3 bytes for each occurrence 😂
But looking into your suggestion, it seems to be what typescript devs @ microsoft recommend when you want to fire and forget a promise
+1 to start doing that
@@ -77,17 +83,39 @@ export class Select2SearchService { | |||
this.searchService | |||
.search('contacts', filters, searchOptions) | |||
.then((documents) => { | |||
if (currentQuery === params.data.q) { |
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 would always be true
because currentQuery
is defined and assigned a value in the same scope here. By pulling it into a class field, it can now be used to discard outdated results
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.
Looks great @m5r thank you for reorganizing the telemetry.wdio-spec.js
test ⭐
I only left one comment, that is not a blocker
PS: I only checked the tests section.
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.
Very nice! Great e2e tests!
]) => { | ||
this.recordSearchTelemetry(this.selectedReports, selectedReport, filters); |
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.
Can we get some unit tests for this behavior?
selectedContact, | ||
forms, | ||
loadingContent, | ||
loadingSelectedContactReports, | ||
contactsLoadingSummary, | ||
filters, | ||
]) => { | ||
this.recordSearchTelemetry(this.selectedContact, selectedContact, filters); |
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.
I have seen it suggested that if you have a promise in TS code that you deliberately do not want to await, you preface it with void
:
void this.recordSearchTelemetry(this.selectedContact, selectedContact, filters);
I don't think we have done this much, but maybe it is time to start?
selectedContact, | ||
forms, | ||
loadingContent, | ||
loadingSelectedContactReports, | ||
contactsLoadingSummary, | ||
filters, | ||
]) => { | ||
this.recordSearchTelemetry(this.selectedContact, selectedContact, filters); |
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.
Seems like we should either do that, or leave an inline comment so future us does not think we just forgot the await
...
selectedContact, | ||
forms, | ||
loadingContent, | ||
loadingSelectedContactReports, | ||
contactsLoadingSummary, | ||
filters, | ||
]) => { | ||
this.recordSearchTelemetry(this.selectedContact, selectedContact, filters); |
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.
Can we get some unit tests for this logic?
private selectEl; | ||
private currentQuery: string | null = null; | ||
private onContactSelect; |
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.
I don't think it is going to work to have this state here at the service level. I double-checked and confirmed that if you have a form with more than one select-contact
field, then the init
function below is going to get called for each of the contact selectors in the form. (And this state will only be set for the last one).
Maybe I am missing something here, but is there a reason we cannot setup the select2:select
event listener somewhere like the initSelect2
method (for each given selector) instead of doing it in the generic query
method?
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.
That's a good catch... I landed with registering this event listener in the query
method because this is where I had access to the user's search to hook up with the contact the user would end up selecting.
I also couldn't find anything in select2 docs to retrieve the search text programmatically without writing code that looks like a pile of hacks with a mix of jQuery and DOM element querying 😅
But! I think I found a solution. I'll come back and resolve this convo if this works.
} | ||
|
||
const doc = await this.getDoc(docId); | ||
await this.searchTelemetryService.recordContactByTypeSearch(doc, 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.
Minor, but technically I think this will only hit the contacts_by_type_freetext
view if the types
param above is valued. Otherwise, I guess shared-libs/search
will just route this to contacts_by_freetext
...
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.
Right, but select2-search
is only ever used to retrieve contacts by type, no?
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.
Select2SearchService
is initialized twice in the codebase. First occurrence in
cht-core/webapp/src/ts/modals/send-message/send-message.component.ts
Lines 174 to 189 in b7b2629
private initPhoneField($phone, initialValue) { | |
return Promise | |
.all([ | |
this.settingsService.get(), | |
this.contactTypesService.getAll() | |
]) | |
.then(([settings, contactTypes = []]) => { | |
const searchIds = contactTypes.map(type => type.id); | |
const personTypes = contactTypes | |
.filter(type => type.person) | |
.map(type => type.id); | |
const select2Options = this.getSelect2Options(settings, personTypes, contactTypes, initialValue); | |
return this.select2SearchService.init($phone, searchIds, select2Options); | |
}); | |
} |
And the second occurrence in
cht-core/webapp/src/ts/modals/edit-report/edit-report.component.ts
Lines 42 to 53 in 59b42e2
return this.contactTypesService | |
.getPersonTypes() | |
.then(types => { | |
const typeIds = types.map(type => type.id); | |
const options = { | |
allowNew: false, | |
initialValue: this.report?.contact?._id || this.report?.from, | |
}; | |
return this.select2SearchService.init(this.getSelectElement(), typeIds, options); | |
}) | |
.catch(err => console.error('Error initialising select2', err)); | |
} |
Both times it's initialized with the types
parameter and then that gets translated into a query against medic-client/contacts_by_type
in
cht-core/shared-libs/search/src/generate-search-requests.js
Lines 137 to 148 in 843888c
const contactTypeRequest = (filters, sortByLastVisitedDate) => { | |
if (!filters.types) { | |
return; | |
} | |
const view = 'medic-client/contacts_by_type'; | |
let request; | |
if (filters.types.selected && filters.types.options) { | |
request = getRequestForMultidropdown(view, filters.types, getKeysArray); | |
} else { | |
// Used by select2search | |
request = getRequestWithMappedKeys(view, filters.types.selected, getKeysArray); | |
} |
and its results get "merged" with the freetext query results in
cht-core/shared-libs/search/src/generate-search-requests.js
Lines 274 to 276 in 843888c
if (hasTypeRequest && freetextRequests?.length) { | |
return getCombinedContactsRequests(freetextRequests, contactsByParentRequest, typeRequest); | |
} |
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.
Select2Search is also inited by the db-object-widget:
Select2Search.init($selectInput, contactTypes, { allowNew, filterByParent }).then(function() { |
I did some double-checking and confirmed that when you use the select-contact
appearance _without a type-*
appearance, it will call through to just the contacts_by_freetext
view (since the types in the search request are empty).
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.
🫡 your enketo-fu is always appreciated, I'll handle this case
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.
Okay I pushed what I think will cover this scenario but I'm about to drop off. I've noted to myself to manually test it and also add unit tests for this particular case
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.
Okay there was no straightforward way to unit test this so I went for an e2e test 🙂
Description
#9489
This PR adds telemetry collection for searches without storing any PII/PHI by storing only the properties that matched against the query.
It would have been more straightforward to edit the views and make it emit the property that matched but that meant re-indexing
medic-client
for 0 immediate benefit to the end users. So instead I hooked into webapp code to record a telemetry entry after the user selected a contact/report from the search results.Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.