ENG-365 Add metrics for search and ingestion #3912
Conversation
|
""" WalkthroughThis update introduces granular metrics collection and AWS CloudWatch reporting across the consolidated OpenSearch search and ingestion flows. It replaces the FHIR-specific searcher with a consolidated variant, updates function signatures and test imports accordingly, and adds a new utility for metrics reporting and unit conversions. Deprecated notices are added to legacy utility functions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConsolidatedSearcher
participant CloudWatchUtils
participant OpenSearch
Caller->>ConsolidatedSearcher: search(params)
ConsolidatedSearcher->>CloudWatchUtils: withMetrics (start timing)
ConsolidatedSearcher->>OpenSearch: execute search query
OpenSearch-->>ConsolidatedSearcher: search results
ConsolidatedSearcher->>CloudWatchUtils: reportMetrics(metrics)
ConsolidatedSearcher-->>Caller: results
Caller->>ConsolidatedSearcher: hasData(patientId)
ConsolidatedSearcher->>CloudWatchUtils: withMetrics (start timing)
ConsolidatedSearcher->>OpenSearch: check data existence
OpenSearch-->>ConsolidatedSearcher: result
ConsolidatedSearcher->>CloudWatchUtils: reportMetrics(metrics)
ConsolidatedSearcher-->>Caller: boolean
Caller->>CloudWatchUtils: reportMemoryUsage()
Possibly re 8000 lated 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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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 (
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts
Did you find this useful? React with a 👍 or 👎 |
f75fea7 to
01c8874
Compare
There was a problem hiding this comment.
Copied from packages/lambdas/src/shared/cloudwatch.ts w/ some changes
packages/core/src/util/units.ts
Outdated
There was a problem hiding this comment.
Copied from packages/lambdas/src/shared/units.ts w/o changes
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/core/src/util/units.ts (1)
1-3: Minor optimization: Remove unnecessary Number() wrapper.The
kbToMbfunction already returns anumber, so wrapping it withNumber()is redundant.export function kbToMbString(value: number) { - return Number(kbToMb(value)).toFixed(2) + "MB"; + return kbToMb(value).toFixed(2) + "MB"; }packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts (1)
26-26: Consider dependency injection instead of module-level instanceThe module-level
cloudWatchUtilsinstance creates shared state that could be problematic for testing and unit isolation.Consider moving this to function parameters or a factory pattern:
-const cloudWatchUtils = new CloudWatchUtils(Config.getAWSRegion(), Features.ConsolidatedSearch);Then pass the instance as a parameter or create it within the function where needed.
packages/core/src/external/aws/cloudwatch.ts (1)
89-94: Consider using proper logger instead of console.log in error handlingThe error handling uses
console.logstatements, which violates the coding guidelines that recommend avoidingconsole.login packages other than utils, infra and shared, and suggest usingout().loginstead.Apply this pattern consistently:
- console.log(`${msg}, ${JSON.stringify(metrics)} - ${errorToString(error)}`); + // Consider using out().log or a proper logger instead of console.logSince this is error logging for CloudWatch failures, consider importing and using the
out()utility from the project's logging utilities.Also applies to: 132-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(2 hunks)packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts(3 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts(7 hunks)packages/core/src/domain/features.ts(1 hunks)packages/core/src/external/aws/cloudwatch.ts(1 hunks)packages/core/src/external/opensearch/lexical/lexical-searcher.ts(6 hunks)packages/core/src/util/units.ts(1 hunks)packages/lambdas/src/shared/cloudwatch.ts(4 hunks)packages/lambdas/src/shared/units.ts(1 hunks)
🧰 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/util/units.tspackages/core/src/command/consolidated/search/fhir-resource/fhir-config.tspackages/core/src/domain/features.tspackages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.tspackages/lambdas/src/shared/units.tspackages/core/src/command/consolidated/search/fhir-resource/search-lexical.tspackages/lambdas/src/shared/cloudwatch.tspackages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.tspackages/core/src/external/opensearch/lexical/lexical-searcher.tspackages/core/src/external/aws/cloudwatch.ts
🧬 Code Graph Analysis (4)
packages/core/src/util/units.ts (1)
packages/lambdas/src/shared/units.ts (2)
kbToMbString(9-11)kbToMb(16-18)
packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts (1)
packages/core/src/external/opensearch/lexical/lexical-searcher.ts (1)
OpenSearchConsolidatedSearcherConfig(19-19)
packages/lambdas/src/shared/units.ts (1)
packages/core/src/util/units.ts (2)
kbToMbString(1-3)kbToMb(5-7)
packages/core/src/external/aws/cloudwatch.ts (3)
packages/lambdas/src/shared/cloudwatch.ts (5)
DurationMetric(5-5)CountMetric(6-6)Metrics(8-8)CloudWatchUtils(18-115)logMemoryUsage(120-127)packages/lambdas/src/shared/capture.ts (1)
capture(18-103)packages/core/src/util/units.ts (2)
kbToMb(5-7)kbToMbString(1-3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (33)
packages/core/src/util/units.ts (1)
5-7: LGTM! Correct unit conversion implementation.The implementation correctly converts kilobytes to megabytes using the proper conversion factor (1048576 = 1024²).
packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts (1)
1-1: LGTM! Correct type migration to consolidated searcher.The import and return type changes correctly migrate from the FHIR-specific searcher config to the consolidated searcher config, aligning with the broader refactoring in this PR.
Also applies to: 4-4
packages/lambdas/src/shared/units.ts (1)
1-4: LGTM! Correct delegation pattern with proper imports.The refactoring correctly delegates to the core package implementations using appropriate import aliases to avoid naming conflicts. The delegation pattern maintains backward compatibility while centralizing the logic.
Also applies to: 10-10, 17-17
packages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.ts (7)
2-2: LGTM! Feature flag import aligns with consolidated approach.The import of the Features enum supports the new consolidated metrics infrastructure.
4-5: LGTM! Proper imports for metrics instrumentation.The CloudWatchUtils and related types import enables comprehensive metrics collection, and the OpenSearchConsolidatedSearcher import aligns with the consolidated search migration.
12-15: LGTM! CloudWatchUtils initialization follows best practices.The CloudWatchUtils is properly initialized with AWS region from config and the appropriate feature flag for this specific use case.
36-37: LGTM! Metrics collection setup is correct.The metrics object initialization and overall timing start are properly positioned.
41-45: LGTM! Precise timing measurement for hasData operation.The timing logic correctly measures the duration of the hasData check and stores it with appropriate metadata.
48-52: LGTM! Accurate timing for ingestion process.The conditional timing measurement for the ingestion operation is correctly implemented and only measures when ingestion actually occurs.
55-56: LGTM! Complete metrics reporting implementation.The total duration calculation and asynchronous metrics reporting properly concludes the instrumentation without blocking the function return.
packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts (6)
1-3: LGTM! Proper imports for consolidated metrics infrastructure.The imports correctly bring in the Features enum and comprehensive CloudWatch utilities including the withMetrics helper.
11-11: LGTM! CloudWatchUtils initialization follows the established pattern.The CloudWatchUtils instance is properly configured with the appropriate feature flag for consolidated ingestion.
25-28: LGTM! Clean setup for metrics collection.The patient data destructuring improves readability, and the metrics setup follows the expected pattern.
35-42: LGTM! Effective use of withMetrics helper.The withMetrics wrapper cleanly instruments the async operations without cluttering the code. The parallel execution of getConsolidatedPatientData and delete operations is preserved.
54-58: LGTM! Consistent metrics instrumentation for bulk ingestion.The withMetrics wrapper is properly applied to the bulk ingestion operation, maintaining clean separation of concerns.
61-62: LGTM! Complete metrics reporting implementation.The total duration calculation and metrics reporting properly conclude the instrumentation without affecting the function's primary logic.
packages/core/src/external/opensearch/lexical/lexical-searcher.ts (5)
19-19: LGTM! Type rename aligns with consolidated architecture.The type name change from
OpenSearchFhirSearcherConfigtoOpenSearchConsolidatedSearcherConfigis consistent with the consolidated approach.
33-41: LGTM! Proper CloudWatchUtils integration.The class rename and CloudWatchUtils initialization correctly implement the consolidated search architecture with comprehensive metrics support.
57-69: LGTM! Clean metrics instrumentation for search operation.The withMetrics wrapper cleanly instruments the paginated search without affecting the core logic, and the async metrics reporting doesn't block the return.
85-91: LGTM! Consistent metrics pattern for hasData operation.The metrics instrumentation follows the same clean pattern as the search method, maintaining consistency across the class.
160-172: LGTM! Complete metrics coverage for getByIds operation.The getByIds method is properly instrumented with the same metrics pattern, ensuring comprehensive observability across all OpenSearch operations.
packages/lambdas/src/shared/cloudwatch.ts (4)
15-16: LGTM! Clear deprecation notice with migration path.The deprecation notice clearly directs users to the new core package implementation while maintaining backward compatibility.
33-35: LGTM! Consistent deprecation notice for reportMetrics method.The method-level deprecation notice ensures users are aware of the migration path when using this specific functionality.
80-81: LGTM! Comprehensive deprecation coverage for reportMemoryUsage.The deprecation notice for the memory usage reporting method completes the migration guidance for all public methods.
117-119: LGTM! Complete deprecation strategy for utility functions.The deprecation notice for the logMemoryUsage function ensures all CloudWatch-related utilities have clear migration paths to the core package.
packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts (3)
4-6: LGTM: Clean import additions for metrics functionalityThe new imports for
Features, CloudWatch utilities, andConfigare well-organized and support the metrics collection functionality being added.Also applies to: 20-20
46-47: LGTM: Well-implemented metrics collection with proper timingThe metrics collection implementation is clean and follows good practices:
- Uses consistent
Date.now()timing approach- Properly integrates with
withMetricshelper for timing individual operations- Captures timing for search, hydration, and total duration
- Reports metrics asynchronously to CloudWatch without blocking main flow
The timing measurements provide valuable observability into the search performance.
Also applies to: 59-66, 82-91, 98-101
115-115:✅ Verification successful
Verify consistent usage of OpenSearchConsolidatedSearcher
The code has been updated to use
OpenSearchConsolidatedSearcherinstead of the previous searcher class.Also applies to: 143-143
🏁 Script executed:
#!/bin/bash # Description: Verify all OpenSearch searcher usage has been updated consistently # Expected: Should find usages of OpenSearchConsolidatedSearcher and no remaining OpenSearchFhirSearcher echo "Searching for old OpenSearchFhirSearcher:" rg "OpenSearchFhirSearcher" --type ts echo -e "\nSearching for new OpenSearchConsolidatedSearcher:" rg "OpenSearchConsolidatedSearcher" --type ts -A 2Length of output: 4464
Consistent replacement of OpenSearchFhirSearcher confirmed
- No occurrences of
OpenSearchFhirSearcherremain in the codebase.- All searcher instantiations and imports now use
OpenSearchConsolidatedSearcher.packages/core/src/external/aws/cloudwatch.ts (5)
6-9: LGTM: Clean type definitions for metricsThe type definitions are well-structured:
- Clear separation between duration and count metrics using discriminated unions
- Proper use of undefined to ensure mutual exclusivity
- Clean
Metricsrecord type for collecting multiple metricsAlso applies to: 11-11
38-58: LGTM: Well-designed CloudWatchUtils class with good documentationThe class constructor and structure follow TypeScript best practices:
- Clear parameter documentation
- Sensible defaults for metrics namespace
- Proper AWS SDK initialization
- Good getter for CloudWatch client access
77-88: LGTM: Robust metrics reporting with proper AWS integrationThe
reportMetricsimplementation is solid:
- Correctly handles both duration and count metrics
- Proper CloudWatch MetricDatum structure
- Uses appropriate units (Milliseconds, Count)
- Includes service dimension for filtering in CloudWatch
- Handles errors gracefully without failing the main flow
160-172: LGTM: Excellent utility function for timing operationsThe
withMetricshelper function is well-implemented:
- Clean async function wrapper
- Consistent timing using
Date.now()- Proper metrics object population
- Optional logging parameter
- Good TypeScript generics usage
This provides a clean way to instrument any async operation with timing metrics.
141-148: LGTM: Comprehensive memory usage loggingThe
logMemoryUsagefunction provides detailed memory statistics including RSS, heap usage, external memory, and array buffers. The console.log usage here is acceptable as it's a dedicated logging utility function.
packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts
Outdated
Show resolved
Hide resolved
01c8874 to
8e7b63e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (2)
30-38:⚠️ Potential issueType mismatch:
queryis optional in params but mandatory in implementation
SearchConsolidatedParamsallowsquery: string | undefined, butsearchPatientConsolidated
requiresquery: string. Callers passingundefinedwill now fail to compile.Either make the function accept
string | undefined, or make the param type strict.-export type SearchConsolidatedParams = { - patient: Patient; - query: string | undefined; -}; +export type SearchConsolidatedParams = { + patient: Patient; + query: string; // keep it mandatory everywhere +}; ... -export async function searchPatientConsolidated({ patient, query }: { patient: Patient; query: string; }) +export async function searchPatientConsolidated({ patient, query }: SearchConsolidatedParams)Also applies to: 52-58
62-83: 🛠️ Refactor suggestion
Promise.allrejects on the first failure – metrics & hydration may be skippedIf either
searchConsolidatedorsearchDocumentsthrows, the other promise is cancelled and
the catch path never publishes metrics, leaving you blind during outages.Instead:
- Use
Promise.allSettledso both tasks run to completion.- Publish whatever metrics were gathered in a
finallyblock.- Decide how to surface partial failures to callers.
-const [fhirResourcesResults, docRefResults] = await Promise.all([ - withMetrics(searchConsolidatedPromise, "search_consolidated", metrics, log), - withMetrics(searchDocumentsPromise, "search_documents", metrics, log), -]); +const [fhirResSettled, docRefSettled] = await Promise.allSettled([ + withMetrics(searchConsolidatedPromise, "search_consolidated", metrics, log), + withMetrics(searchDocumentsPromise, "search_documents", metrics, log), +]); +if (fhirResSettled.status === "rejected" || docRefSettled.status === "rejected") { + // decide: throw combined error, return partial results, etc. +} +const fhirResourcesResults = + fhirResSettled.status === "fulfilled" ? fhirResSettled.value : []; +const docRefResults = + docRefSettled.status === "fulfilled" ? docRefSettled.value : [];
🧹 Nitpick comments (3)
packages/core/src/external/opensearch/lexical/fhir-searcher.ts (2)
33-41: Prefer re-using a single CloudWatch client instead of creating one per searcher
CloudWatchUtilsspins up a new AWS SDK client every time anOpenSearchConsolidatedSearcheris instantiated.
If the searcher is built per request (e.g., inside a Lambda handler or a web-request scope) this can create
dozens of CloudWatch connections, leading to unnecessary socket churn and higher cold-start latency.- private readonly cloudWatchUtils: CloudWatchUtils; - - constructor(readonly config: OpenSearchConsolidatedSearcherConfig) { - this.cloudWatchUtils = new CloudWatchUtils( - Config.getAWSRegion(), - Features.OpenSearchConsolidatedSearcher - ); - } + private static cloudWatchUtils = new CloudWatchUtils( + Config.getAWSRegion(), + Features.OpenSearchConsolidatedSearcher + ); + + constructor(readonly config: OpenSearchConsolidatedSearcherConfig) {}This keeps one CloudWatch client per runtime instead of per instance.
(If DI is preferred, inject the shared util from above the searcher.)
99-134: Consider adding metrics togetByIdfor observability completenessThe other public methods expose timing data but
getByIddoes not.
If this endpoint becomes slow or fails frequently you will have no CloudWatch signal.Adding a tiny wrapper as shown below keeps the instrumentation consistent:
- try { - const response = ( + const metrics: Metrics = {}; + let response; + try { + response = await withMetrics( + () => + client.get({ + index: indexName, + id: entryId, + }), + "getById", + metrics + ); + response = response.body as OpenSearchResponseGet<FhirSearchResult>; ... - } catch (error) { + } catch (error) { ... - } + } finally { + await this.cloudWatchUtils.reportMetrics(metrics); + }packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
121-136: Repeatedly instantiating the searcher can be costlyBoth
searchConsolidatedand every recursive call inhydrateMissingReferences
create a newOpenSearchConsolidatedSearcher, which in turn creates a new
OpenSearch client & auth handshake. Consider passing a shared instance around:-const searchService = new OpenSearchConsolidatedSearcher(getConfigs()); +// create once at module-level or pass as arg +const searchService = singletonOpenSearchConsolidatedSearcher ?? + (singletonOpenSearchConsolidatedSearcher = new OpenSearchConsolidatedSearcher(getConfigs()));This cuts connection setup time and avoids excess sockets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(2 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(2 hunks)packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts(3 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(7 hunks)packages/core/src/domain/features.ts(1 hunks)packages/core/src/external/aws/cloudwatch.ts(1 hunks)packages/core/src/external/opensearch/lexical/fhir-searcher.ts(6 hunks)packages/core/src/util/units.ts(1 hunks)packages/lambdas/src/shared/cloudwatch.ts(4 hunks)packages/lambdas/src/shared/units.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts
- packages/core/src/domain/features.ts
- packages/lambdas/src/shared/units.ts
- packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts
- packages/core/src/util/units.ts
- packages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.ts
- packages/lambdas/src/shared/cloudwatch.ts
- packages/core/src/external/aws/cloudwatch.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/command/consolidated/search/fhir-resource/search-consolidated.ts
🧠 Learnings (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.480Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
🧬 Code Graph Analysis (2)
packages/core/src/external/opensearch/lexical/fhir-searcher.ts (4)
packages/core/src/external/opensearch/index.ts (1)
OpenSearchConfigDirectAccess(7-11)packages/core/src/external/aws/cloudwatch.ts (3)
CloudWatchUtils(38-139)Metrics(9-9)withMetrics(160-172)packages/core/src/external/opensearch/paginate.ts (1)
paginatedSearch(28-58)packages/core/src/external/opensearch/index-based-on-fhir.ts (1)
FhirSearchResult(14-14)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (4)
packages/core/src/external/aws/cloudwatch.ts (3)
CloudWatchUtils(38-139)Metrics(9-9)withMetrics(160-172)packages/core/src/external/fhir/shared/bundle.ts (2)
buildBundleEntry(210-216)buildSearchSetBundle(204-208)packages/core/src/external/opensearch/lexical/fhir-searcher.ts (1)
OpenSearchConsolidatedSearcher(33-176)packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts (1)
getConfigs(4-12)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
Show resolved
Hide resolved
Ref eng-365 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-365 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
6df822b to
3175cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (2)
6-8: Verify thatFeatures.ConsolidatedSearchenum member exists.This was flagged in a previous review. Please ensure the
Featuresenum includes theConsolidatedSearchmember to prevent compile-time errors.#!/bin/bash # Description: Verify Features.ConsolidatedSearch enum member exists # Expected: Find the enum definition with ConsolidatedSearch member ast-grep --pattern $'enum Features { $$$ ConsolidatedSearch$_$ $$$ }' # Alternative: Search for the Features enum definition rg -A 10 "enum Features" --type ts
28-29: Same verification needed for CloudWatchUtils instantiation.The
Features.ConsolidatedSearchusage here will also fail if the enum member doesn't exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(2 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(2 hunks)packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts(3 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(7 hunks)packages/core/src/domain/features.ts(1 hunks)packages/core/src/external/aws/cloudwatch.ts(1 hunks)packages/core/src/external/opensearch/lexical/fhir-searcher.ts(6 hunks)packages/core/src/util/units.ts(1 hunks)packages/lambdas/src/shared/cloudwatch.ts(4 hunks)packages/lambdas/src/shared/units.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/core/src/domain/features.ts
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated.test.ts
- packages/core/src/command/consolidated/search/fhir-resource/fhir-config.ts
- packages/core/src/command/consolidated/search/fhir-resource/ingest-if-needed.ts
- packages/core/src/util/units.ts
- packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts
- packages/lambdas/src/shared/units.ts
- packages/lambdas/src/shared/cloudwatch.ts
- packages/core/src/external/opensearch/lexical/fhir-searcher.ts
- packages/core/src/external/aws/cloudwatch.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 9E81 ’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 (1)
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.480Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
🪛 GitHub Actions: PR Check - Core
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
[error] 86-86: ESLint: 'subStartedAt' is never reassigned. Use 'const' instead (prefer-const)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (5)
62-63: LGTM! Clean metrics initialization.Good approach to initialize the metrics object and start timing measurement.
65-70: Good pattern for metrics wrapping.Wrapping the search function in a promise-returning function enables clean integration with the
withMetricshelper.
97-106: Good metrics tracking for hydration step.The hydration metrics follow the same pattern as the search metrics, providing consistent timing data.
121-136: Function rename and searcher update look good.The transition from
searchFhirResourcestosearchConsolidatedand usingOpenSearchConsolidatedSearcheraligns with the consolidated search implementation.
161-161: Consistent use of new searcher in hydration.Good consistency using
OpenSearchConsolidatedSearcherin the hydration function as well.
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
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
Ref eng-365 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Dependencies
Description
Add metrics for Consolidated search and ingestion.
Duplicate
CloudWatchUtilsclass from lambdas to core.Testing
Release Plan
Summary by CodeRabbit
New Features
Refactor
Chores
Tests