Repository navigation
Release 0.7.6 - #220
Release 0.7.6#220
Conversation
…ogle health converters
…zed users for subscription
Converters for the New Data Types: Floors, SedentaryPeriod, and ActivityLevels
…cise speed from pace
Write ECG result classification and HRV frequency powers, fix exercise speed
Add civil times to Google Health ping intervals
… drop non-numeric values, log dropped records
Add converters for ECG and IRN data, and update exercise data parsing.
yatharthranjan
left a comment
yatharthranjan
left a comment
There was a problem hiding this comment.
Great work aditya. please find some comments below. If some of these might take longer, we can proceed with the release and tackle in another PR+release.
| import java.time.Instant | ||
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| data class GoogleHealthUser( |
There was a problem hiding this comment.
the fields are are non-null here. IIRC Upstream RestSourceUserDTO in Rest-Source-Auth declares all of them nullable.
this relies on the caller filtering on authorized=true. Make these fields nullable and filter after parsing, or perhaps just document the contract.
There was a problem hiding this comment.
I made it similar to how we do it for garmin, where GarminUser has all these fields non-null and it has been working fine, so I thought this would be fine too. Still, I've updated it now to handle null fields. The list is read into a new class, which has the nullable fields, and only complete users are processed, and incomplete users are skipped.
But I am keeping this class as non-null because it implements the User interface, which has a contract of non-null user fields.
| "Exercise data point has no name or dataPointName to derive a log id from " + | ||
| "for user=${user.versionedId}", | ||
| ) | ||
| val activityId = idSegment.toLongOrNull() |
There was a problem hiding this comment.
can you confirm this is always numeric? I think safer would be to just use a string here, that saves us from an error down the line if google ever changes this format (even if right now it is always numeric)
There was a problem hiding this comment.
This isn't mentioned in the docs for exercise, and they're inconsistent here. We take the id from the last part of the data point's name. I've tested it with real data and it was always numeric and fits in a long, the schema field is also already a long, should we keep it for now. If it didn't work in future we can update it by a new string field?
There was a problem hiding this comment.
ok sure, perhaps add a comment in code to remind us if it does ever happen
| this.beatsPerMinuteAvg = beatsPerMinuteAvg | ||
| this.samplingFrequencyHertz = frequency | ||
| this.millivoltsScalingFactor = scalingFactor | ||
| this.leadNumber = leadNumber | ||
| this.deviceModel = deviceModel | ||
| this.firmwareVersion = firmwareVersion | ||
| this.featureVersion = featureVersion | ||
| this.resultClassification = resultClassification |
There was a problem hiding this comment.
a lot of these values are repeated across records. i wonder if it would be more efficient to just have a summary record and then these samples can be linked to that summary by the id ?
There was a problem hiding this comment.
Yeah, this is the better way, but if we do it now, it needs a new summary schema and topic, release, and then a new converter here. Is it okay if we do it in the next release?
ecg data also doesn't come often, only when the user explicitly takes a reading on the watch
|
Thanks yatharth, I've addressed the other points. For the ecg record and the exercise, I've noted them down and can we update them in the next release? |
|
ok thanks aditya. looks good. please add issues to track those. |
This release contains updates for the Google Health migration. It adds the new
google-health-librarywith converters for the Google Health API data types, moves to radar-schemas 0.9.4, enables Maven Central publishing for the library, and includes security fixes. The library is published asorg.radarbase:google-health-library:0.7.6.Included PRs: