-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
6915 - Telemetry daily frequency #7095
Conversation
b6a63ad
to
b9eca76
Compare
- Output errors in the standard error stream to see the errors in the console while piping right results to a JSON file - Add Unix exec permission and header to execute the script without the `node ` prefix
@dianabarsan would you mind to review? @garethbowen , could you at least check the description of the PR? specially the "Backward compatibility" section. |
I think the backwards compatibility section is fine. What will happen if couch2pg is not updated but this is rolled out? |
The view has a unique index that should fail, so I guess will prevent couch2pg to insert data. Anyway, I think we only need to update the SQL definition of that view, should be easy to do, I'll start to work on that tomorrow so we will have a PR this week. Moreover the changes should be backward compatible and work with both schemas of the metadata, so we can release couch2pg first, to have time to deploy it before CHT 3.12. |
How will we know which is the "current" month if we check these records years after they were created? |
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.
Nice work! I left a few suggestions inline.
Co-authored-by: Diana Barsan <[email protected]>
Co-authored-by: Diana Barsan <[email protected]>
Co-authored-by: Diana Barsan <[email protected]>
Co-authored-by: Diana Barsan <[email protected]>
For backwards compatibility we can't require an upgrade to couch2pg as part of a CHT Core upgrade unless we bump the major (ie: make 3.12.0 a 4.0.0 release). Not only would this be a headache for the internal rollout, there are self-hosting partners with couch2pg that would need to be notified. We must make every attempt to maintain backwards compatibility. Worst case would be making this configurable defaulting to monthly, so app developers can opt in when their couch2pg supports it. |
@garethbowen I just tested medic-couch2pg against a CouchDB that has daily telemetry generated with the changes made in this PR and indeed it's NOT compatible with daily telemetry, it fails because the unique constraint The definition of the constraint is as follow: CREATE UNIQUE INDEX idx_useview_telemetry_period_start_user ON useview_telemetry(period_start,user_name); The problem is not the constraint though, it is fine to have only one record per perdiod / user, the problem is that period is defined as the concatenation of We can release a new version of medic-couch2pg that is compatible with both versions of the telemetry data, so if installed before the upgrade of the CHT, the transition would be smooth: records from CHT < 3.12 will have the Otherwise, as you said, we will need to make telemetry configurable. In that case we need to:
|
4e100b8
to
c9328d6
Compare
Worth to add that the concatenation in the column |
I just found this bug in the last changes introduced in medic-couch2pg to support users meta data that will require a patch release regardless of the telemetry changes proposed here, forcing at least all the deployments that has the last version installed to upgrade soon or later. CC @garethbowen |
@mrsarm It is essential that upgrading is easy and free of breaking changes so we can keep projects on recent versions of the CHT. As such, we cannot knowingly release a that will crash couch2pg. If you cannot find a solution to the backwards compatibility issue, then we'll have to delay this change to go out with CHT v4.0.0. |
c9328d6
to
49c54d5
Compare
@garethbowen , summarizing what we discussed in our last call regarding this: medic-couch2pg versions < 3.2.x don't support migration of medic-couch2pg >= 3.2.x (3.2.0 and 3.2.1) do support So, if CHT <= 3.11 deployments that upgrade to CHT 3.12 (with the changes included in this PR) don't upgrade to the upcoming medic-couch2pg release and stick with medic-couch2pg < 3.2.x, they won't notice any change when upgrading to CHT 3.12, because feedback and telemetry records aren't accessible from the Postgres databases synchronized with old versions of medic-couch2pg. If CHT deployments of any version want to access feedback and telemetry data from the PG database synchronized with medic-couch2pg, they will need to upgrade the medic-couch2pg to an upcoming version not released yet, regardless of the CHT version, and therefore regardless of telemetry being monthly or daily, because there is no medic-couch2pg version today that supports feedback and telemetry without bugs, so we will need to install the upcoming version of medic-couch2pg for that, and this upcoming version will support both daily and monthly aggregation. The upcoming release of medic-couch2pg needs to be released first than CHT 3.12, but the PR that addresses the bugs and adds support to the daily aggregation is under review now so shouldn't be a problem: medic/cht-couch2pg#91 |
@dianabarsan , all the changes you suggested were applied, and the PR was paused because the couch2pg issue that was indirectly related but now fixed (the recreation of the telemetry view with the wrong index that now is also compatible with the changes here). Could you please check again? |
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.
Thanks for the updates. I think some of the comments from the previous review were skipped, so I've "bumped" them.
@dianabarsan could you review again ? I'm confident enough that I have addressed all the changes requested this time 😬 |
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.
Great work!
Description
Change telemetry frequency from monthly to daily.
#6915
Improvements
telemetry-<year>-<month>-<day>-<username>-<uuid>
. Also in themetadata
section a new fieldday
is added with the day of the month the data belongs to.scripts/get_users_meta_docs.js
):node
prefix.Please also review the documentation changes here.
Bug fixed
It also fix the following
bugsbug found in the current implementation:When
record()
is called for the first time in the period (a period was a month, now is a day), the record is added to the telemetry DB and then the aggregation is executed with the data that was stored previously, so the aggregation process aggregates all the records from the previous period + a record that is actually from the current period. The last record shouldn't be taken into account in the aggregation submitted.This represents a minor issue even for daily reports, and it is fixed in this PR just changing the order of the execution: executing first the aggregation if a new period started, and then add the new record.
Backward compatibility
Systems that rely on telemetry data will need to adequate to the new frequency, but so far the data collected and the schema is the same, with the only addition of the new field
metadata.day
.There is no special code to "migrate" existent data collected in the devices where the new version is installed: if a device was collecting telemetry data to send next month and then the new version is installed, the app will aggregate the data collected the same day the device is used, and send the aggregation when connection is available, but the record will look like a "daily" aggregation, not monthly, or more precisely "partially monthly". E.g.:
telemetry-2021-4-greg-aaabbb1234
.telemetry-2021-4-2-greg-aaabbb1234
.telemetry-2021-5-13-greg-aaabbb1234
. Next day if there is data collected will betelemetry-2021-5-14-...
and son on. Days that the devices is not used won't have telemetry recorded therefore no reports to send.So once a upgrade of the CHT is rolled out, first reports will include data from many days, but should be easy to identify these reports because the month in the id (or the field
metadata.month
) will be from the past month, not the current month. If we want to implement a more smooth transition, like send one report for each day the telemetry data has not been aggregated, will require more changes, specially in the device webapp, and not sure if it worth the effort.CC @helizabetholsen and @dianabarsan on this, let me know if is not clear enough.
To-doin another app / issuemedic-couch2pg has a PG view useview_telemetry with the telemetry data, and there is a field field
period_start
that we need to adapt to support these changes. I've created a ticket to keep track of it: medic-couch2pg#85 → released in 3.3.0Checklist
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.