-
Notifications
You must be signed in to change notification settings - Fork 46
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
IHE Gateway V2 Monitoring #2200
base: develop
Are you sure you want to change the base?
Conversation
85c4f70
to
14122f5
Compare
…ces added on created_at Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>
14122f5
to
17c7a2a
Compare
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.
rename this file and folder monitor
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 think we should avoid adding more code that uses Sentry on lambdas. We have this ticket to move to pg
so we're lighter when hitting the DB from lambdas: https://github.com/metriport/metriport-internal/issues/1561
WRT to the reports themselves, I'd try to store more raw results in a .csv
and send/log a link to it. That's because by putting it on a Spreadsheet we have more flexibility to work the data. Example: we're sending percentage of failure/success, but we might want to see the amount over time as well.
But NABD, just trying to avoid back-and-forth with code updates, PRs, releases, just to change how we see the data.
...rc/sequelize/migrations/2024-06-04_00_alter-patient-discovery-result-add-created-at-index.ts
Outdated
Show resolved
Hide resolved
...lize/migrations/2024-06-04_01_alter-document-query-retrieval-results-add-created-at-index.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/carequality/ihe-gateway-v2/ihe-gateway-v2-logic.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/carequality/ihe-gateway-v2/report/report.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/carequality/ihe-gateway-v2/report/report.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/carequality/ihe-gateway-v2/report/report.ts
Outdated
Show resolved
Hide resolved
dbCredsSecret.grantRead(scheduledReportLambda); | ||
|
||
const rule = new events.Rule(this, "ScheduledReportRule", { | ||
schedule: events.Schedule.rate(Duration.hours(12)), |
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.
Will this run every 12h from the time its deployed? If so, wouldn't it be better to have specific times on the day so we have more predictable Ops?
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 changed it to run at 12am and 2pm UTC (7am PST and 5pm PST
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.
Made requested changes, especially switching from sequelize to pg.
WRT to csv, I think I am gonna hold off on doing that for now.
The format this is in works for me and I dont see the value in overcomplicating things with csvs since this is really just for us to track things and isnt for outside reporting or analytics.
Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>
Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>
SELECT request_id | ||
FROM ${tableName} | ||
WHERE created_at >= $1 | ||
ORDER BY created_at DESC |
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 should be referencing the TableColumns enum, right?
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.
As for the Query, wouldn't be better to random
on the request IDs filtered by created at?
`; | ||
const values = [since, limit]; | ||
const { rows } = await pool.query(query, values); | ||
return rows.map(row => row.request_id); |
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.
ditto: row
is probably any
, right? We could use this instead, to at least make sure we're accessing the correct property:
... row[TableColumns.requestId]);
request_id = "request_id", | ||
created_at = "created_at", |
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.
NABD, but the prop for the column name could follow camelCase
PatientDiscoveryResult = "patient_discovery_result", | ||
DocumentQueryResult = "document_query_result", | ||
DocumentRetrievalResult = "document_retrieval_result", |
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.
nit: props/enum items should be camelCase; PascalCase is for Classes/Types only.
packages/core/src/external/carequality/ihe-gateway-v2/report/report.ts
Outdated
Show resolved
Hide resolved
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 dont see the value in overcomplicating things with csvs
I actually think this code is more complicated than appending a line at the end of 3-6 files. This PR calculates percentages and summaries, while we could just add lines w/ raw data to files on S3.
Second, this assumes we're not changing how we assess the result/success/usage of the IHE GW. As we all know, things tend to change after we ship them. 😄
Lastly, how to you plan to look at data over time? Looking at logs and calculating in your head, or copy/pasting and parsing the results? If we have a CSV where we add a line for each new run, we already have data over time there.
Its not a blocker for me, but my experience tells me that we'd be better served with a handful of CSVs instead of logs on CloudWatch.
result.patientMatch === true || | ||
result.operationOutcome?.issue?.some(issue => issue.code === "not-found") | ||
); | ||
|
||
const failures = results.filter( | ||
(result: OutboundPatientDiscoveryResp) => | ||
!( | ||
result.patientMatch === true || | ||
result.operationOutcome?.issue?.some(issue => issue.code === "not-found") |
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.
Since we're replicating the condition in verbatim, might as well write a small function isSuccess
or something that we can use in both places, to avoid updating one and forgetting the other one.
); | ||
|
||
const errorOids = failures.map((error: OutboundPatientDiscoveryResp) => ({ | ||
oid: (error.gateway as XCPDGateway).oid, |
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.
No need to cast here, right?
); | ||
|
||
const errorOids = failures.map((error: OutboundDocumentQueryResp) => ({ | ||
oid: (error.gateway as XCAGateway).homeCommunityId, |
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.
ditto: no need to cast
); | ||
|
||
const errorOids = failures.map((error: OutboundDocumentRetrievalResp) => ({ | ||
oid: (error.gateway as XCAGateway).homeCommunityId, |
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.
ditto: cast
const errorString = errorToString(error); | ||
log(errorString); | ||
capture.error("Failed to send PD response to Internal Carequality Endpoint", { |
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 also send the message to the log? Just the error w/o context is not as helpful.
We have a "pattern" for this:
const msg = "Failed to send PD response to Internal Carequality Endpoint";
log(`${msg} - ${errorToString(error)}`);
capture.error(msg, {
]); | ||
|
||
fs.writeFileSync( | ||
"./runs/carequality-report/patient-discovery-report.json", |
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.
NABD, but carequality-report
is tied to a specific HIE, but this is IHE GW, which in theory could be other HIEs in the mid-term.
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.
Also, no date on the file name?
async function main() { | ||
const sqlDBCreds = getEnvVarOrFail("DB_CREDS"); | ||
const readReplicaEndpoint = getEnvVarOrFail("DB_READ_REPLICA_ENDPOINT"); |
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.
Missing initRunsFolder()
- see other scripts that use the "runs" folder.
const dbReadOnlyEndpoint = getEnvVarOrFail("DB_READ_REPLICA_ENDPOINT"); | ||
const region = getEnvVarOrFail("AWS_REGION"); | ||
|
||
capture.setExtra({ lambdaName: "scheduled-report-lambda" }); |
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.
There's an env var set by AWS for this, see other lambdas - example:
// Automatically set by AWS
const lambdaName = getEnvOrFail("AWS_LAMBDA_FUNCTION_NAME");
const region = getEnvOrFail("AWS_REGION");
// Set by us
...
import { DbCreds, dbCredsSchema, dbReadReplicaEndpointSchema } from "./sequelize"; | ||
// Initialize the main DB pool |
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.
supernit: new line
const pool = new Pool({ | ||
user: parsedDbCreds.username, | ||
host: parsedDbCreds.host, | ||
database: parsedDbCreds.dbname, | ||
password: parsedDbCreds.password, | ||
port: parsedDbCreds.port, | ||
}); |
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.
It would be good to have a const and set the pool size (property max
). Even though the default 10 connections is fine for this use case, we're building something that might be used afterwards, so being explicit about this, with a link to the docs, is helpful - I had to search it up online to make sure there was more than 1 connection by default.
Ticket: #1667
TODO
Description
Testing
Release Plan