-
Notifications
You must be signed in to change notification settings - Fork 42
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
[16141] Implement ReceiverEnrichment Function #16854
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Test Results1 274 tests +2 1 270 ✅ +2 7m 20s ⏱️ -31s Results for commit 208692c. ± Comparison against base commit 8766daa. This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Integration Test Results 60 files 60 suites 42m 19s ⏱️ Results for commit 208692c. ♻️ This comment has been updated with latest results. |
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.
The new receiver enrichment is looking good! I mostly had some nitpicks to offer.
I ran the manual test steps locally and was able to verify them with a little adjustment (see fhir-message-header-sample.yml for details).
One thing to note is that I had to do some weird steps to run RS due to the db migrations. AFAIK this is the first time I've really had to update my local setup to reflect db changes so I may be ignorant of how to do this properly.
My command order to run these changes:
./gradlew clean
docker-compose -f docker-compose.build.yml up --detach
./gradlew resetDB
./gradlew reloadTables // this is going to fail
./gradlew quickRun
./gradlew reloadTables // this is going to succeed now
./gradlew reloadSettings
I had to run reloadTables twice because trying to run RS without it errored out as "Unresolved reference: receiver_enrichment"
but if running reloadTables before RS is up, it will eventually error out (after affecting the changes) because the backend isn't running.
Testing results
These results are with the changes detailed in fhir-message-header-sample.yml, snippets extracted for brevity.
Input HL7 sent with development.dev-elims
:
MSH|^~\&|CDC PRIME - Atlanta,^2.16.840.1.114222.4.1.237821^ISO|Winchester House^05D2222542^ISO|CDPH CA REDIE^2.16.840.1.114222.4.3.3.10.1.1^ISO|CDPH_CID^2.16.840.1.114222.4.1.214104^ISO|20210803131511.0147+0000||ORU^R01^ORU_R01|1234d1d1-95fe-462c-8ac6-46728dba581c|P|2.5.1|||NE|NE|USA|UNICODE UTF-8|||PHLabReport-NoAck^ELR_Receiver^2.16.840.1.113883.9.11^ISO
SFT|Centers for Disease Control and Prevention|0.1-SNAPSHOT|PRIME Data Hub|0.1-SNAPSHOT||20210726
Resulting output HL7 received by development.DEV_ENRICHMENT
:
MSH|^~\&|CDC PRIME - Atlanta,^2.16.840.1.114222.4.1.237821^ISO|Winchester House^05D2222542^ISO|CDPH CA REDIE^2.16.840.1.114222.4.3.3.10.1.1^ISO|CDPH_CID^2.16.840.1.114222.4.1.214104^ISO|20210803131511.0147+0000||ORU^R01^ORU_R01|1234d1d1-95fe-462c-8ac6-46728dba581c|P|2.5.1|||NE|NE|USA|UNICODE UTF-8|||PHLabReport-NoAck^ELR_Receiver^2.16.840.1.113883.9.11^ISO
SFT|Orange Software Vendor Name|0.2-YELLOW|Purple PRIME ReportStream|0.1-SNAPSHOT||20210726
Setup a FHIR receiver using the same enrichment:
- name: DEV_ENRICHMENT_FHIR
externalName: Enrichment Development FHIR
organizationName: development
topic: full-elr
customerStatus: active
translation:
type: "FHIR"
enrichmentSchemaNames: ["classpath:/metadata/fhir_transforms/receivers/fhir-message-header-sample.yml"]
Resulting output FHIR received by development.DEV_ENRICHMENT_FHIR
:
{
"resourceType": "Bundle",
"id": "1738026051290321000.f19c5c44-bc9e-4451-9021-31a472c63311",
"timestamp": "2021-08-03T06:15:11.015-07:00",
"entry": [
{
"fullUrl": "MessageHeader/1738026051344027000.77ab067b-c8ef-4bc8-b225-d338aaa89a26",
"resource": {
"resourceType": "MessageHeader",
"id": "1738026051344027000.77ab067b-c8ef-4bc8-b225-d338aaa89a26",
"source": {
"software": "PRIME Data Hub",
"version": "0.1-SNAPSHOT",
"endpoint": "urn:oid:2.16.840.1.114222.4.1.237821"
}
}
},
{
"fullUrl": "Provenance/1738026051394889000.b3eddc01-54a4-478a-8974-09160eb98815",
"resource": {
"resourceType": "Provenance",
"id": "1738026051394889000.b3eddc01-54a4-478a-8974-09160eb98815",
"entity": [
{
"role": "source",
"what": {
"reference": "Device/1738026051396739000.581ba9b0-2d47-45c0-9ee9-92ea4301ae54"
}
}
]
}
},
{
"fullUrl": "Organization/1738026051396594000.75cda0eb-85c4-43c9-a7eb-f79c8e76750b",
"resource": {
"resourceType": "Organization",
"id": "1738026051396594000.75cda0eb-85c4-43c9-a7eb-f79c8e76750b",
"name": "Orange Software Vendor Name"
}
},
{
"fullUrl": "Device/1738026051396739000.581ba9b0-2d47-45c0-9ee9-92ea4301ae54",
"resource": {
"resourceType": "Device",
"id": "1738026051396739000.581ba9b0-2d47-45c0-9ee9-92ea4301ae54",
"extension": [
{
"url": "https://reportstream.cdc.gov/fhir/StructureDefinition/software-vendor-org",
"valueReference": {
"reference": "Organization/1738026051396594000.75cda0eb-85c4-43c9-a7eb-f79c8e76750b"
}
}
],
"manufacturer": "Centers for Disease Control and Prevention",
"deviceName": [
{
"name": "Purple PRIME ReportStream",
"type": "manufacturer-name"
}
],
"modelNumber": "0.1-SNAPSHOT",
"version": [
{
"extension": [],
"value": "0.2-YELLOW"
}
]
}
}
]
}
prime-router/src/main/kotlin/fhirengine/engine/FHIRTranslator.kt
Outdated
Show resolved
Hide resolved
prime-router/src/main/kotlin/fhirengine/engine/FHIRReceiverEnrichment.kt
Show resolved
Hide resolved
prime-router/src/main/kotlin/fhirengine/engine/FHIRReceiverEnrichment.kt
Outdated
Show resolved
Hide resolved
prime-router/src/test/kotlin/fhirengine/azure/FHIRTranslatorIntegrationTests.kt
Outdated
Show resolved
Hide resolved
prime-router/src/test/kotlin/fhirengine/azure/FHIRReceiverEnrichmentIntegrationTests.kt
Show resolved
Hide resolved
prime-router/src/test/kotlin/fhirengine/engine/FHIRReceiverEnrichmentTests.kt
Outdated
Show resolved
Hide resolved
...-router/src/main/resources/metadata/fhir_transforms/receivers/fhir-message-header-sample.yml
Outdated
Show resolved
Hide resolved
...-router/src/main/resources/metadata/fhir_transforms/receivers/fhir-message-header-sample.yml
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.
Looks great, just a few little comments. I compared the the implementation to the SRD documented here and found no deviations (besides the one listed in my comments).
@@ -0,0 +1,12 @@ | |||
# Universal Pipeline Receiver Enrichment Step | |||
After the [destination filter](./destination-filter.md) step has completed, each configured receiver will receive the converted FHIR bundle for potential further enrichment. If the specific receiver in question has been configured with one or more enrichment schema names, each enrichment schema will be applied in turn and update the FHIR bundle. These enrichments are small customizations that don't warrant the creation of an entirely new translation schema. |
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.
After the [destination filter](./destination-filter.md) step has completed, each configured receiver will receive the converted FHIR bundle for potential further enrichment. If the specific receiver in question has been configured with one or more enrichment schema names, each enrichment schema will be applied in turn and update the FHIR bundle. These enrichments are small customizations that don't warrant the creation of an entirely new translation schema. | |
After the [destination filter](./destination-filter.md) step has completed, each configured receiver will receive the converted FHIR bundle for potential further enrichment. If the specific receiver in question has been configured with one or more enrichment schema names, each enrichment schema will be applied in turn and update the FHIR bundle. |
I'm not sure what the part I removed here means? Purpose of a "translation schema" is to translate from one format to another, and the purpose of an "enrichment schema" is to add/remove/change data.
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.
Change applied.
) : FHIREngine(metadata, settings, db, blob, azureEventService, reportService, reportStreamEventService) { | ||
|
||
/** | ||
* Accepts a [FhirReceiverEnrichmentQueueMessage] [message] and sends a report to the |
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.
* Accepts a [FhirReceiverEnrichmentQueueMessage] [message] and sends a report to the | |
* Accepts a [FhirReceiverEnrichmentQueueMessage] and sends a report to the |
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.
Change applied.
) | ||
) | ||
reportEventService.sendItemEvent( | ||
eventName = ReportStreamEventName.ITEM_ROUTED, |
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.
ITEM_ROUTED event here seems incorrect? Should probably be ITEM_TRANSFORMED instead according to the SRD?
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.
Change applied.
/* | ||
* Add receiver-enrichment enum type to underlying custom data types. | ||
*/ | ||
ALTER TYPE public.task_action ADD VALUE 'receiver-enrichment' after 'destination-filter'; |
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 know we discussed this but looking at the actual DB changes in these two migration files, I don't see why we can't make these two a separate PR, merge and deploy it, and then merge this branch?
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.
Second PR is in place in case we want to do that.
val bundle = FhirTranscoder.decode(fhirJson) | ||
if (receiver.enrichmentSchemaNames.isNotEmpty()) { | ||
receiver.enrichmentSchemaNames.forEach { enrichmentSchemaName -> | ||
logger.info("Applying enrichment schema $enrichmentSchemaName") |
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.
Include report id and root report Id here maybe?
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.
Root report id is not available within this scope. I did add reportId.
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.
Looking great now! 🎸 🚀 👍
|
This PR introduces a new step into the ReportStream pipeline. Receiver Enrichment relocates the enrichment schema functionality from FHIRTranslator to the new FHIRReceiverEnrichment step.
Test Steps:
../gradlew quickRun
)../gradlew clearDB
,../gradlew reloadSettings
,../gradlew reloadTables
).Changes
Checklist
Testing
./prime test
or./gradlew testSmoke
against local Docker ReportStream container?Process
Linked Issues
To Be Done
Specific Security-related subjects a reviewer should pay specific attention to