feat(ihe): stream responses to response endpoints + epic bigger doc r… #2306

jonahkaye · 2024-06-19T22:35:34Z

Refs: #1667

Downstream

Sub request id + improve retries logging for ihe v2 + Retries for ERR_BAD_RESPONSE on DQ #2315

Description

stream responses back to api
increase number of doc refs to send for epic from 5 to 10 per DR

Testing

Local
- unit tests
Staging
- staging

Release Plan

Merge this

…ef chunks Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

packages/core/src/external/carequality/ihe-gateway-v2/gateways.ts

jonahkaye · 2024-06-20T13:48:29Z

packages/core/src/external/carequality/ihe-gateway-v2/ihe-gateway-v2-async.ts

@@ -17,7 +17,7 @@ dayjs.extend(duration);
 const SLEEP_IN_BETWEEN_DOCUMENT_RETRIEVAL_REQUESTS = dayjs.duration({ seconds: 1 });
 const MAX_GATEWAYS_BEFORE_CHUNK = 1000;
 const MAX_DOCUMENT_QUERY_REQUESTS_PER_INVOCATION = 20;
-const MAX_DOCUMENT_RETRIEVAL_REQUESTS_PER_INVOCATION = 20;
+const MAX_DOCUMENT_RETRIEVAL_REQUESTS_PER_INVOCATION = 10;


we are upping docRefs per req from 5-10 (only for epic but thats 98% of our docs so lets just treat it like that for everything) so to make sure we stay under the lambda payload size limit lets half the number of gateways per request.

Might make sense to ultimately flatten this logic so that there's a single request count per lambda invocation.

I think this would be especially good for DR, yeah.

I see the benefit of grouping a bunch of XCPDs in a single lambda request (no much memory/timeout issue), but to download a file, a single lambda invocation seem to make sense.

ok I think this concern needs more consideration and is beyond the scope of the PR. There are a bunch of other considerations to take into account:

cost

concurrency limits

complete reworking of querrying architecture to be queue based

Moving DRs to individual ones has nothing to do with (3) and (2).

For (2), we have 1,000 concurrent lambda executions and we can ask to increase this limit to 10's of 1,000 (source).

For (3), there's no requirement to add queues in order to break them down.

For. (1)/cost, this might impact, but it would make each single invocation shorter - and lambdas are cheap, it doesn't even make it to the top 10 costs we have.

Not saying we have to do it here, just addressing the justifications.

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

jonahkaye · 2024-06-20T17:19:24Z

packages/core/src/external/carequality/ihe-gateway-v2/constants.ts

@@ -16,4 +16,4 @@ export const namespaces = {
 xsiType: "xsd:string",
 };

-export const expiresIn = 5;
+export const expiresIn = 20;


need saml timestamp to last longer!
5 mins too short if we are doing retries with the same request

thomasyopes · 2024-06-20T18:30:47Z

packages/core/src/external/carequality/ihe-gateway-v2/ihe-gateway-v2-async.ts

@@ -17,7 +17,7 @@ dayjs.extend(duration);
 const SLEEP_IN_BETWEEN_DOCUMENT_RETRIEVAL_REQUESTS = dayjs.duration({ seconds: 1 });
 const MAX_GATEWAYS_BEFORE_CHUNK = 1000;
 const MAX_DOCUMENT_QUERY_REQUESTS_PER_INVOCATION = 20;
-const MAX_DOCUMENT_RETRIEVAL_REQUESTS_PER_INVOCATION = 20;
+const MAX_DOCUMENT_RETRIEVAL_REQUESTS_PER_INVOCATION = 10;


Might make sense to ultimately flatten this logic so that there's a single request count per lambda invocation.

packages/core/src/external/carequality/ihe-gateway-v2/ihe-gateway-v2-logic.ts

packages/core/src/external/carequality/ihe-gateway-v2/outbound/xcpd/process/error.ts

packages/core/src/external/carequality/ihe-gateway-v2/ihe-gateway-v2-logic.ts

packages/core/src/external/carequality/ihe-gateway-v2/outbound/xcpd/process/error.ts

packages/core/src/external/carequality/ihe-gateway-v2/gateways.ts

leite08 · 2024-06-20T19:30:55Z

packages/core/src/external/carequality/ihe-gateway-v2/ihe-gateway-v2-async.ts

@@ -17,7 +17,7 @@ dayjs.extend(duration);
 const SLEEP_IN_BETWEEN_DOCUMENT_RETRIEVAL_REQUESTS = dayjs.duration({ seconds: 1 });
 const MAX_GATEWAYS_BEFORE_CHUNK = 1000;
 const MAX_DOCUMENT_QUERY_REQUESTS_PER_INVOCATION = 20;
-const MAX_DOCUMENT_RETRIEVAL_REQUESTS_PER_INVOCATION = 20;
+const MAX_DOCUMENT_RETRIEVAL_REQUESTS_PER_INVOCATION = 10;


I think this would be especially good for DR, yeah.

I see the benefit of grouping a bunch of XCPDs in a single lambda request (no much memory/timeout issue), but to download a file, a single lambda invocation seem to make sense.

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

leite08

LGTM! 🚀

It'd be good to keep the convo about single lambda invocation for DR somewhere else (it would be bad to lose a large XML file w/ lots of resources b/c the lambda ran out of memory from downloading multiple files in a single invocation.

feat(ihe): stream responses to response endpoints + epic bigger doc r…

858a904

…ef chunks Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

jonahkaye self-assigned this Jun 19, 2024

jonahkaye commented Jun 20, 2024

View reviewed changes

packages/core/src/external/carequality/ihe-gateway-v2/gateways.ts Show resolved Hide resolved

jonahkaye commented Jun 20, 2024

View reviewed changes

jonahkaye added 3 commits June 20, 2024 12:39

feat(retry): nit

8771bfa

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

feat(ihe): add tests

e5f7d5d

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

feat(ihe): develop to branch

0ce0360

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

jonahkaye marked this pull request as ready for review June 20, 2024 16:57

jonahkaye requested review from Goncharo, leite08, Orta21, RamilGaripov and thomasyopes as code owners June 20, 2024 16:57

feat(ihe): bump saml timestamp expiration up to 20 mins

ebd7cf8

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

jonahkaye commented Jun 20, 2024

View reviewed changes

jonahkaye mentioned this pull request Jun 20, 2024

Sub request id + improve retries logging for ihe v2 + Retries for ERR_BAD_RESPONSE on DQ #2315

Merged

2 tasks

thomasyopes reviewed Jun 20, 2024

View reviewed changes

leite08 reviewed Jun 20, 2024

View reviewed changes

feat(ihe): type, naming, ordering

f282fc5

Refs: #1667 Signed-off-by: Jonah Kaye <[email protected]>

jonahkaye mentioned this pull request Jun 20, 2024

RELEASE 429s, Stream IHE Results, Epic Chunk Sizes #2319

Merged

1 task

leite08 approved these changes Jun 20, 2024

View reviewed changes

jonahkaye added this pull request to the merge queue Jun 21, 2024

Merged via the queue into develop with commit 3ed64a6 Jun 21, 2024
13 checks passed

jonahkaye deleted the 1667-stream-flow-epic-larger-chunks branch June 21, 2024 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ihe): stream responses to response endpoints + epic bigger doc r… #2306

feat(ihe): stream responses to response endpoints + epic bigger doc r… #2306

jonahkaye commented Jun 19, 2024 •

edited

Loading

jonahkaye Jun 20, 2024

thomasyopes Jun 20, 2024

leite08 Jun 20, 2024

jonahkaye Jun 20, 2024

leite08 Jun 20, 2024 •

edited

Loading

jonahkaye Jun 20, 2024

thomasyopes Jun 20, 2024

leite08 Jun 20, 2024

leite08 left a comment

feat(ihe): stream responses to response endpoints + epic bigger doc r… #2306

feat(ihe): stream responses to response endpoints + epic bigger doc r… #2306

Conversation

jonahkaye commented Jun 19, 2024 • edited Loading

Downstream

Description

Testing

Release Plan

jonahkaye Jun 20, 2024

Choose a reason for hiding this comment

thomasyopes Jun 20, 2024

Choose a reason for hiding this comment

leite08 Jun 20, 2024

Choose a reason for hiding this comment

jonahkaye Jun 20, 2024

Choose a reason for hiding this comment

leite08 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

jonahkaye Jun 20, 2024

Choose a reason for hiding this comment

thomasyopes Jun 20, 2024

Choose a reason for hiding this comment

leite08 Jun 20, 2024

Choose a reason for hiding this comment

leite08 left a comment

Choose a reason for hiding this comment

jonahkaye commented Jun 19, 2024 •

edited

Loading

leite08 Jun 20, 2024 •

edited

Loading