ENG-371 Fix hydration + Adj API ECS autoscale #3940
Conversation
Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
WalkthroughThe metriport subproject was removed entirely. A new consolidated FHIR search implementation was added, including recursive hydration of resource references and comprehensive tests. Several import paths were updated to reflect code restructuring. Additional environment variables were introduced for search configuration, and ECS Fargate CPU scaling was adjusted from 10% to 15%. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConsolidatedSearch
participant OpenSearch
participant BundleUtil
Client->>ConsolidatedSearch: searchPatientConsolidated({patient, query})
ConsolidatedSearch->>OpenSearch: searchFhirResources(patient, query)
ConsolidatedSearch->>OpenSearch: searchDocumentReferences(patient, query)
OpenSearch-->>ConsolidatedSearch: FHIR resources & document references
ConsolidatedSearch->>ConsolidatedSearch: hydrateMissingReferences(resources)
loop up to max iterations
ConsolidatedSearch->>OpenSearch: getByIds(missingReferenceIds)
OpenSearch-->>ConsolidatedSearch: referenced resources
ConsolidatedSearch->>ConsolidatedSearch: merge hydrated resources
end
ConsolidatedSearch->>BundleUtil: buildSearchSetBundle(all resources)
BundleUtil-->>ConsolidatedSearch: SearchSetBundle
ConsolidatedSearch-->>Client: SearchSetBundle
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| * @param query The query to search for. | ||
| * @returns The search results. | ||
| */ | ||
| export async function searchPatientConsolidated({ |
There was a problem hiding this comment.
Moved from packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts w/ modifications.
To see the changes, check out this commit.
There was a problem hiding this comment.
Moved to packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
| @@ -585,7 +585,7 @@ export function createAPIService({ | |||
| maxCapacity: maxTaskCount, | |||
| }); | |||
| scaling.scaleOnCpuUtilization("autoscale_cpu", { | |||
| targetUtilizationPercent: 10, | |||
| targetUtilizationPercent: 15, | |||
There was a problem hiding this comment.
Adjust ECS auto-scale to kick off at 15% avg CPU
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/infra/lib/api-stack/api-service.ts (1)
588-588: LGTM - Autoscaling threshold adjustment is well-documented.The increase from 10% to 15% CPU utilization for autoscaling trigger is intentional and documented in the past review comments. This should reduce scaling noise while maintaining adequate performance.
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
48-54: Well-structured function with comprehensive documentation.The function signature and documentation clearly describe the purpose and parameters. Good implementation detail mentioned in the comment.
🧹 Nitpick comments (8)
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts (1)
5-5: Track the TODO comment for centralized test helpers.This TODO aligns with the broader refactoring effort to consolidate FHIR test utilities. Consider creating a tracking issue to ensure this refactoring is completed.
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (5)
82-92: Consider using immutable array operations.The code mutates the
resourcesMutablearray. According to the coding guidelines, prefer immutable code patterns.Apply this diff to use immutable array operations:
- const resourcesMutable = fhirResourcesResults.flatMap(r => { + const fhirResources = fhirResourcesResults.flatMap(r => { const resourceAsString = r[rawContentFieldName]; if (!resourceAsString) return []; try { return JSON.parse(resourceAsString) as Resource; } catch (error) { log(`Error parsing resource ${resourceAsString}: ${errorToString(error)}`); return []; } }); - resourcesMutable.push(...docRefResults); + const allResources = [...fhirResources, ...docRefResults];Also update the subsequent references:
- `Loaded/converted ${resourcesMutable.length} resources in ${elapsedTimeFromNow( + `Loaded/converted ${allResources.length} resources in ${elapsedTimeFromNow( subStartedAt )} ms, hydrating search results...` - resources: resourcesMutable, + resources: allResources,
107-108: Consider immutable pattern for final resource array.Similar to the previous comment, avoid mutating the hydrated array.
Apply this diff:
const patientResource = patientToFhir(patient); - hydratedMutable.push(patientResource); + const finalResources = [...hydratedMutable, patientResource]; - const entries = hydratedMutable.map(buildBundleEntry); + const entries = finalResources.map(buildBundleEntry);
182-182: Use clearer iteration increment pattern.Pre-increment operator can be confusing. Use a clearer pattern.
- iteration: ++iteration, + iteration: iteration + 1,
139-186: Consider adding resource caching to optimize hydration performance.The recursive hydration process might fetch the same resources multiple times across different search operations. Consider implementing a caching mechanism to improve performance.
Would you like me to propose a caching strategy for the hydration process to avoid redundant OpenSearch queries? This could significantly improve performance for resources that are frequently referenced.
156-156: Consider making the hydration limit configurable.The hardcoded limit of 5 iterations might need adjustment based on the complexity of resource relationships.
Consider making this configurable via environment variable:
-const maxHydrationAttempts = 5; +const maxHydrationAttempts = Config.getHydrationMaxAttempts() ?? 5;packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (2)
269-295: Consider more realistic mock data for string resources.The function creates mock data with "UNKNOWN" resourceType for string inputs, which may not reflect real-world scenarios accurately:
function makeToGetByIdsResultEntry( cxId: string, patientId: string, toEntryId: (resource: Resource | string) => string ) { return (resource: Resource | string): FhirSearchResult => { if (typeof resource === "string") { + // Consider using a more realistic resourceType based on the 8000 string pattern + const resourceType = resource.includes("Encounter") ? "Encounter" : + resource.includes("Location") ? "Location" : "Resource"; return { cxId, patientId, entryId: toEntryId(resource), - resourceType: "UNKNOWN", + resourceType, resourceId: resource, - rawContent: JSON.stringify({ resourceType: "UNKNOWN", resourceId: resource }), + rawContent: JSON.stringify({ resourceType, id: resource }), }; } if (!resource.id) throw new Error(`Resource ID is required`); return { cxId, patientId, entryId: toEntryId(resource), resourceType: resource.resourceType, resourceId: resource.id, rawContent: JSON.stringify(resource), }; }; }
297-306: Simplify conditional logic and improve clarity.The encounter creation logic can be simplified and made more explicit:
function makeEncounter( params: Partial<Encounter> | undefined, patientId: string ): MarkRequired<Encounter, "id"> { const encounter = makeEncounterImported(params, { patient: patientId }); - if (!params?.participant) encounter.participant = []; - if (!params?.location) encounter.location = []; + + // Ensure required arrays are initialized if not provided in params + encounter.participant = params?.participant ?? []; + encounter.location = params?.location ?? []; + if (!encounter.id) throw new Error("Encounter ID is required"); return { ...encounter, id: encounter.id }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
metriport(0 hunks)packages/core/.env.test(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated-direct.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(2 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts(0 hunks)packages/core/src/external/fhir/__tests__/location.ts(1 hunks)packages/core/src/external/opensearch/lexical/fhir-searcher.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts(2 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.ts(1 hunks)packages/infra/lib/api-stack/api-service.ts(1 hunks)packages/utils/src/open-search/semantic-search.ts(1 hunks)
💤 Files with no reviewable changes (2)
- metriport
- packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/core/src/external/opensearch/lexical/fhir-searcher.tspackages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.tspackages/core/src/command/consolidated/search/fhir-resource/fhir-config.tspackages/core/src/command/consolidated/search/fhir-resource/search-consolidated-direct.tspackages/infra/lib/api-stack/api-service.tspackages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.tspackages/utils/src/open-search/semantic-search.tspackages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.tspackages/core/src/external/fhir/__tests__/location.tspackages/core/src/command/consolidated/search/fhir-resource/search-consolidated.tspackages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
🧠 Learnings (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
🧬 Code Graph Analysis (1)
packages/core/src/external/fhir/__tests__/location.ts (2)
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.ts (1)
makeLocation(28-34)packages/shared/src/util/uuid-v7.ts (1)
uuidv7(368-368)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (12)
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.ts (1)
36-36: Good practice for documenting planned refactoring.The TODO comment clearly indicates the intended destination for this test helper function, which helps with future code organization efforts.
packages/core/.env.test (1)
16-20: Appropriate addition of test environment variables.The new search-related environment variables follow the established pattern of using placeholder values ("...") for test configuration, which aligns with the file's purpose as stated in the header comments.
packages/core/src/external/fhir/__tests__/location.ts (1)
1-48: Well-implemented FHIR test helper function.The
makeLocationfunction demonstrates good practices:
- Uses modern
uuidv7()for ID generation- Includes proper FHIR metadata with US Core profile
- Provides realistic test data via faker
- Allows flexible customization through optional parameters
- Correctly places the spread operator to allow overriding defaults
The function follows TypeScript best practices and FHIR resource structure requirements.
packages/core/src/external/opensearch/lexical/fhir-searcher.ts (1)
14-14: Import path update aligns with search refactoring.The import path change from
"./lexical-search"to"./query"is consistent with the broader refactoring effort moving from lexical search to consolidated search implementation.packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts (1)
1-1: Import path update is consistent with refactoring.The import path change from
"lexical-searcher"to"fhir-searcher"aligns with the consolidated search refactoring effort and maintains consistency across the codebase.packages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.ts (1)
3-3: Import path update completes the refactoring consistency.The import path change from
"lexical-searcher"to"fhir-searcher"maintains consistency with the other files in this PR and supports the consolidated search implementation.packages/core/src/command/consolidated/search/fhir-resource/search-consolidated-direct.ts (1)
19-19: Import path update looks good.The import path update from
./search-lexicalto./search-consolidatedcorrectly reflects the module restructuring.packages/utils/src/open-search/semantic-search.ts (1)
5-5: Import path update is correct.The update to import
searchPatientConsolidatedfrom the new consolidated search module aligns with the codebase restructuring.packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (4)
1-16: LGTM: Well-organized imports following TypeScript best practices.The imports are properly organized and follow the coding guidelines. Good use of specific imports from external libraries and proper relative path usage.
27-39: LGTM: Proper test setup with good mock management.The beforeEach/afterEach pattern is correctly implemented with proper mock setup and cleanup. Good use of
constdeclarations and proper TypeScript typing.
41-174: LGTM: Comprehensive test coverage with good edge case handling.The test cases cover the core functionality well, including:
- Basic no-hydration scenario
- Single-level hydration
- Multi-level hydration
- Iteration limits
- Missing resources handling
- Patient reference filtering
The test assertions are appropriate and the mocking strategy is consistent.
176-252: LGTM: Excellent edge case coverage for complex scenarios.These tests handle advanced scenarios like circular references and duplicate resource handling, which are critical for a robust hydration implementation. The test logic correctly validates that the system can handle these complex cases without infinite loops or duplicate processing.
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
Show resolved
Hide resolved
...ges/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
Show resolved
Hide resolved
Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
26-38:⚠️ Potential issueInconsistent
querytyping may cause runtime & compile-time errors
SearchConsolidatedParams.queryis declared asstring | undefined(lines 26-29) whilesearchPatientConsolidatedexpects a non-nullablestring(lines 48-54). Any caller that forwards the optional value directly will hit a TypeScript error or, if cast, will passundefinedto downstream functions that assume a string (e.g.searchFhirResources,searchDocuments).-export async function searchPatientConsolidated({ - patient, - query, -}: { - patient: Patient; - query: string; -}): Promise<SearchSetBundle> { +export async function searchPatientConsolidated({ + patient, + query, +}: { + patient: Patient; + query?: string; +}): Promise<SearchSetBundle> {Follow the same change for
searchFhirResources(lines 115-124).Also applies to: 48-54
🧹 Nitpick comments (3)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (3)
169-174: Avoid side-effectful++iterationto keep recursion pureUsing the pre-increment operator mutates the local variable before it’s passed, which is harder to scan than a pure expression.
- iteration: ++iteration, + iteration: iteration + 1,A tiny tweak, but it aligns with the “functional programming style” guideline.
145-153: Duplicate resources can slip through during hydration
mergedResources = [...resources, ...resourcesToAdd](line 167) may introduce duplicates when a referenced resource is already present. This inflates bundle size and increases downstream processing time.Consider deduplicating by
resource.id(or canonical identifier) after every merge:-import { uniq } from "lodash"; +import { uniq, keyBy } from "lodash"; … -const mergedResources = [...resources, ...resourcesToAdd]; +const mergedResources = Object.values( + keyBy([...resources, ...resourcesToAdd], r => r.id) +);(Or switch to a
Map/Setfor O(1) look-ups.)Also applies to: 165-168
1-6: Minor dependency:lodash/uniqcan be dropped
uniqis the only lodash helper used. You can eliminate the external dependency with:const uniqueIds = [...new Set(missingRefIds)];Reduces bundle size and aligns with the “minimize dependencies” guideline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(2 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/fhir-to-cda/cda-templates/components/tests/make-condition.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code unde 8000 rstands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
🧠 Learnings (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (2)
Learnt from: leite08
PR: metriport/metriport#3857
File: packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts:67-82
Timestamp: 2025-05-28T02:51:35.779Z
Learning: In the search-lexical.ts file, the user prefers to bubble up JSON parsing errors rather than catching and logging them. When processing FHIR resources from OpenSearch results, errors should be thrown and allowed to propagate up the call stack instead of being caught and silently ignored.
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:60-73
Timestamp: 2025-05-31T21:29:39.179Z
Learning: In search-consolidated.ts, the user prefers to let errors bubble up from concurrent search operations (Promise.all) rather than catching them explicitly with try-catch blocks. Errors from searchFhirResources and searchDocuments should be allowed to propagate up the call stack.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
- GitHub Check: check-pr / lint-build-test
Dependencies
Description
Fix OpenSearch post-search hydration - see ticket.
Adj API ECS autoscale - context.
Testing
Release Plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Refactor