Platform/13199/fhir function dynamic message routing #15273
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
There was a problem hiding this comment.
This PR appears to me to be very similar to the original code. We are essentially moving extra configuration from one file to another. Additionally, the original implementation has the translate queue names defined in the package space in FHIREngine, which is poor practice. This PR is on the right track, and we can go even further by exploring a solution that keeps the queue name tied to the class, and make it impossible to configure a new QueueMessage class without defining the queue name. See the proof of concept patch below that illustrates this idea:
|
@arnejduranovic I looked at the patch and I could use a 10m huddle because I think I'm missing something. If I read this right the queue name becomes a hard-coded value tied to the message type which is what I thought we were getting away from. I tied the queue name to a "when" block so we could add logic as needed to short circuit default behavior based on message content. If it's baked into the data class for a given message type then isn't it still hard coded? |
|
@david-navapbc I think we are confusing some things. There are two parts here:
|
|
copied from slack for observability
|
| import java.util.Base64 | ||
| import java.util.UUID | ||
|
|
||
| const val elrConvertQueueName = "elr-fhir-convert" |
There was a problem hiding this comment.
I think it would be better for these strings to just exist in the concrete queuemessage classes they belong to. I don't see a reason why these constants need to be available at a package level? It's kind of a security principle, we should expose information on a need to know basis (i.e. scope things as small as possible).
There was a problem hiding this comment.
our code is open source - we're exposing all of it to the world. Also I think for testing if nothing else it's useful to have these at the top level vs baked into a particular message class.
Happy to make the change though if you feel strongly.
There was a problem hiding this comment.
We currently have a requirement (implicit) that specific queue messages should go to specifically named queues in Azure. So, we should "hardcode" these in the concrete queue message classes and make it so the caller cannot change the QueueName. I don't see testing or open source considerations outweighing this requirement in this case.
| @@ -96,6 +112,7 @@ data class FhirConvertQueueMessage( | |||
| override val blobSubFolderName: String, | |||
| override val topic: Topic, | |||
| val schemaName: String = "", | |||
| override val messageQueueName: String = elrConvertQueueName, | |||
There was a problem hiding this comment.
This is not how the suggested patch set the messageQueueName:
@JsonTypeName("convert")
data class FhirConvertQueueMessage(
override val reportId: ReportId,
override val blobURL: String,
override val digest: String,
override val blobSubFolderName: String,
override val topic: Topic,
val schemaName: String = "",
) : ReportPipelineMessage() {
override val messageQueueName = "testing"
}
Did the above not work for some reason?
There was a problem hiding this comment.
are they not functionally equivalent? what's the difference that makes a difference in having the override messageQueueName in the argument list vs as part of the body?
Happy to do whatever - just curious as to what the issue is
There was a problem hiding this comment.
The issue is the caller can pass in a different messageQueueName with the way you have it now and we do not want to allow that ability. When a queue message is designed, we know at design time what the name of the azure queue should be so we should prevent it from being changed by the caller -- which is what the snippet I posted does.
prime-router/src/test/kotlin/fhirengine/azure/FhirFunctionIntegrationTests.kt
Outdated
Show resolved
Hide resolved
| @@ -244,12 +244,13 @@ class FhirFunctionTests { | |||
| emptyMap(), | |||
| emptyList() | |||
| ) | |||
| val message = FhirRouteQueueMessage( | |||
| val message = FhirTranslateQueueMessage( | |||
There was a problem hiding this comment.
What's the story here?
There was a problem hiding this comment.
the test logic wasn't testing what it was supposed to be testing
…f github.com:CDCgov/prime-reportstream into platform/13199/fhir_function_dynamic_message_routing
|
Branch deployed to Chromatic 🚀.
View via: |
|
| filterManager, | ||
| isLoading, | ||
| } = useReceiverDeliveries(activeReceiver.name); | ||
| const { data: results, filterManager, isLoading } = useReceiverDeliveries(activeReceiver.name); |
There was a problem hiding this comment.
We need to figure this out, but your PR should not be touching any .tsx files. That is outside the scope of platform work.
| @@ -351,7 +350,7 @@ class UniversalPipelineReceiver : SubmissionReceiver { | |||
| if (sender.customerStatus != CustomerStatus.INACTIVE) { | |||
| // move to processing (send to <elrProcessQueueName> queue) | |||
| workflowEngine.queue.sendMessage( | |||
| elrConvertQueueName, | |||
| "elr-fhir-convert", | |||
There was a problem hiding this comment.
We should read this value from one place instead of typing the string in multiple spots. see comment about bringing back the constants.
| @@ -40,33 +35,12 @@ class FHIRFunctions( | |||
| @FunctionName("convert-fhir") | |||
| @StorageAccount("AzureWebJobsStorage") | |||
| fun convert( | |||
| @QueueTrigger(name = "message", queueName = elrConvertQueueName) | |||
| @QueueTrigger(name = "message", queueName = "elr-fhir-convert") | |||
There was a problem hiding this comment.
For all of these, read from the appropriate queue object.
| @@ -23,12 +23,6 @@ import gov.cdc.prime.router.serializers.Hl7Serializer | |||
| import org.jooq.Field | |||
| import java.time.OffsetDateTime | |||
|
|
|||
| const val elrConvertQueueName = "elr-fhir-convert" | |||
There was a problem hiding this comment.
Easiest/ least disruptive solution to the problem of defining these strings all over probably is to move these constants to the static area of QueueMessage (inside object), then you can read the value from there instead of defining the string multiple times. Still solves the issue of these being defined at a package level.
|
replaced by #15613. Closing. |
This PR ...
Refactors how queue message routing happens such that the behavior (1) no longer hard coded (2) baked into the queue message itself.
The trigger for changing default routing behavior is expected to be the something in the content of the message itself and thus it makes sense for the routing logic to be colocated with the content of the message itself.Changes
QueueMessage has been updated with a "when" clause (switch/case for Java-heads) that sets default routing behavior for a given queue message type. Should a short circuit of this behavior be required the germane "when" block for that message type may be updated to route the message to a different queue topic. the queueAccess object is provided to the a new function, QueueMessage.send() to keep separation of concern in tact. The QueueMessage object handles routing itself with routing plumbing provided by FHIRFunctions.QueueMessage,FhirFunctions, and associated tests have been updated.QueueMessagewas changed to attach the correct queue name to the QueueMessage obj typed for that kind of QueueMessage. Functionally speaking, the queue message itself knows where it's supposed to go.In
FhirFunctionsthe calls todoRoute(), doConvert(), etchave been consolidated into a single common method. The test updates were made to accommodate this change.Checklist
Testing
./prime testor./gradlew testSmokeagainst local Docker ReportStream container?