[BACK-2218][BACK-2187] Add hourlystats and multiple summary periods #74
Conversation
Roukoswarf
changed the title
…tient [BACK-2216] Add demo clinic patient on new clinic creation
|
/query qa2 |
|
image: tidepool/clinic:hourlystats-660a198a6d4518ff3af4c93cc5a47ff6fb6a334f |
…fixes [BACK-2235] Set newly created clinics `IsMigrated` property to true
| @@ -105,10 +105,154 @@ func (r *repository) Initialize(ctx context.Context) error { | |||
| SetBackground(true). | |||
| SetName("PatientSummaryLastUploadDate"), | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
Having more than 15 indexes per collection is usually considered a bad practice and likely means that the model is not optimal. It's probably best to store all periods in an array, instead of an object/map so you can than have a multikey index on summary.periods.period.timeCGMUsePercent (where periods is an array, and period is an attribute in an object) instead of having to create an index per period.
…-change Revert renaming of patient demo user variable
patients/patients.go
Outdated
| LastUploadDateFrom *time.Time | ||
| LastUploadDateTo *time.Time | ||
|
|
||
| TimeCGMUsePercentCmp1d *string |
There was a problem hiding this comment.
We should refactor this and combine the comparator and value in a single struct.
| func MaybeApplyNumericFilter(selector bson.M, field string, cmp *string, value float64) { | ||
| if f, ok := cmpToMongoFilter(cmp); ok { | ||
| selector[field] = bson.M{f: value} | ||
| func MaybeApplyNumericFilter(selector bson.M, period string, field string, cmp *string, value float64) { |
There was a problem hiding this comment.
This function was used to apply generic numeric filters to a query, but now it only works for summary periods - the name of the function should reflect that.
| { | ||
| Keys: bson.D{ | ||
| {Key: "clinicId", Value: 1}, | ||
| {Key: "summary.periods.14d.percentTimeInHigh", Value: 1}, | ||
|
|
||
| {Key: "summary.periods.14d.timeCGMUsePercent", Value: 1}, |
There was a problem hiding this comment.
I'm not entirely sure our use of those indices (with the $ne trick) achieves much, as the all levels of the b-tree index which use the $ne operator must be interated over, instead of doing a binary search. Have you ran any benchmarks against a non-indexed filter/sort? Is it possible that the performance is actually worse as we'd have to iterate over all levels, even though the fields are not really used? Also, I guess the order here matters a lot - most frequently used fields should come first.
| func MaybeApplyNumericFilter(selector bson.M, period string, field string, cmp *string, value float64) { | ||
| if operator, ok := cmpToMongoFilter(cmp); ok { | ||
| // ugly, but needed to ensure index prefix | ||
| for _, filterable := range filterablePeriodFields { |
There was a problem hiding this comment.
You shouldn't add $ne for every filterable summary field, as it could result in multiple iterations over the index which are not required. You should add it only to those fields that come before field you're filtering by in the compound index order.
If you have a compound index on a.b.c, and you want to filter the results by b, you should add $ne: -1 only to a, but not c.
| if operator, ok := cmpToMongoFilter(cmp); ok { | ||
| // ugly, but needed to ensure index prefix | ||
| for _, filterable := range filterablePeriodFields { | ||
| if _, exists := selector["summary.periods."+period+"."+filterable]; !exists { |
There was a problem hiding this comment.
It's probably better to use fmt.Sprintf instead of concatenation
| {Key: "summary.periods.1d.hasTimeCGMUsePercent", Value: -1}, | ||
|
|
||
| {Key: "summary.periods.1d.glucoseManagementIndicator", Value: 1}, | ||
| {Key: "summary.periods.1d.hasGlucoseManagementIndicator", Value: -1}, |
There was a problem hiding this comment.
Should boolean fields be part of this index? The fields are not filterable, and I'm not sure the index can be utilized for sorting given that the index is compound.
spec/clinic.v1.yaml
Outdated
| description: Rotating list containing the stats for each currently tracked hour in order | ||
| items: | ||
| $ref: '#/components/schemas/PatientSummaryStat' | ||
| PatientSummaryStat: |
There was a problem hiding this comment.
FYI, if the API is not backward compatible, the clinic client must be updated in all services that use it (e.g. clinic worker, shoreline, hydrophone). You should probably delete all summaries, update clinic client in all services that use it, and only then push this PR to production.
|
Superseded by #83 |
No description provided.