Skip to content
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

useview_telemetry failing for telemetry docs without an app version falling in january #98

Closed
eljhkrr opened this issue May 31, 2021 · 5 comments
Assignees

Comments

@eljhkrr
Copy link
Member

eljhkrr commented May 31, 2021

{ error: date/time field value out of range: "2021-0-1"
    at Connection.parseE (/var/adapter/shared/medic-couch2pg-v3.3.0/node_modules/pg/lib/connection.js:545:11)
    at Connection.parseMessage (/var/adapter/shared/medic-couch2pg-v3.3.0/node_modules/pg/lib/connection.js:370:19)
    at Socket.<anonymous> (/var/adapter/shared/medic-couch2pg-v3.3.0/node_modules/pg/lib/connection.js:113:22)
    at emitOne (events.js:125:13)
    at Socket.emit (events.js:221:7)
    at addChunk (_stream_readable.js:265:12)
    at readableAddChunk (_stream_readable.js:252:11)
    at Socket.Readable.push (_stream_readable.js:209:10)
    at TCP.onread (net.js:598:20)
  name: 'error',
  length: 163,
  severity: 'ERROR',
  code: '22008',
  detail: undefined,
  hint: 'Perhaps you need a different "datestyle" setting.',
  position: undefined,
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  schema: undefined,
  table: undefined,
  column: undefined,
  dataType: undefined,
  constraint: undefined,
  file: 'datetime.c',
  line: '3890',
  routine: 'DateTimeParseError',
  appliedMigrations: [] }

This error is thrown for zero indexed months pointing to January in telemetry docs which do not have an app version so get missed by the condition here https://github.com/medic/medic-couch2pg/blob/e6ff06bbf469b773320a5524db1e97158271680e/libs/medic-users-meta/migrations/202105171933.do.86.usersMeta.sql#L32

@mrsarm
Copy link
Contributor

mrsarm commented May 31, 2021

From what I'm understand, the deployment when the error is happening has an old CHT version, where records are stored with months 0-indexed, and the deployment is not managed by Horti, so the field metadata.versions.app is not set in the Couch records.

The problem as I understand cannot be solved by medic-couch2pg, because lets say we switch the condition in the view query, that will solve the issue for THIS deployment, but what happens if another deployment with a CHT greater than 3.8.0 that is also not managed by Horti tries to sync with this new release that invert the condition? we will have the same exception , but now instead of 0 indexed months we will have months 2-indexed, from 2 - 13, so at the first record from December that is parsed as month "13" we will have the same exception.

I don't known how likely is to move that project to Horti or what other alternatives are to set that field to the right value in Couch before performing the synchronization, and what other improvements to expect when using Horti.

@garethbowen @dianabarsan may give us more insights into that.

@garethbowen
Copy link
Contributor

As this is a live site incident we just need to find a way to unblock production. We can either...

  1. Use a string not a date, and defer the problem to the queries
  2. Assume a blank version means < 3.8.0 so add 1 to the month

I think 2 is more correct but we should also add a check that if the month is 12 then don't add 1. This way we may get a bunch of off by 1 errors (adding one when we shouldn't), but we get back up and running. We can then raise an issue to sort it out properly later if/when we discover incorrect dates.

Either way, the code needs to be bullet proof - practice defensive programming and handle every possible case even if it shouldn't happen.

@kennsippell
Copy link
Member

kennsippell commented Jun 1, 2021

Another acceptable option might be to omit data from this view if it can't be reliably interpreted (eg. if the doc doesn't have an associated core version). This would ensure a useful reliable view with few assumptions for the majority of deployments .. but these "unmanaged" deployments would need to fend for themselves and interpret/parse their data contextually.

@mrsarm
Copy link
Contributor

mrsarm commented Jun 1, 2021

Another acceptable option might be to omit data from this view if it can't be reliably interpreted (eg. if the doc doesn't have an associated core version). This would ensure a useful reliable view with few assumptions for the majority of deployments .. but these "unmanaged" deployments would need to fend for themselves and interpret/parse their data contextually.

Agree, maybe at least until we move forward with the improvements needed to centralize DB changes management from this repo scripts and custom made changes for particular changes, I don't see a way to fix this from here without breaking other deployments.

There are deployments with CHT 3.8+ , and make the SQL change to make more compatible medic-couch2pg with older versions of CHT will make less compatible with newer versions of CHT deployments that may also not have that value set in the records, so if we have to decide what versions to support better, according with our table of supported versions of CHT, should be 3.8.0+, lower versions are not supported any more by Medic (though I would be glad to make a query that fix the problem for both, I'm afraid is not possible).

@mrsarm
Copy link
Contributor

mrsarm commented Jun 8, 2021

@kennsippell I'm going to close this issue, talking with @derickl , looks like all the projects were migrated executing a SQL script after upgrade medic-couch2pg.

We cannot include that patch script because it will solve the issue only for some deployments and produce the same problems in others were the current script works.

However, adding a setting in a env variable that says what is the current version of CHT where the data comes from could solve the problem, in that case, we can add that script to the codebase of this project and execute accordingly , but CHT <= 3.8 are not supported officially by medic, and not sure how many CHT <= 3.8 deployments are that didn't upgrade yet but going to upgrade at some point that we should support instead of saying "please upgrade your CHT".

Finally, I think the solution for problems similar to this will be to implement #93 , we are going to discuss it soon with the product team.

CC @craig-landry @garethbowen

@mrsarm mrsarm closed this as completed Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants