LinuxForHealth / FHIR

The LinuxForHealth FHIR® Server and related projects
https://linuxforhealth.github.io/FHIR
Apache License 2.0
330 stars 157 forks source link

CodeSystem resource lookup results in search even though the resource is available from other resource providers #2862

Open punktilious opened 3 years ago

punktilious commented 3 years ago

Describe the bug When testing update-as-create using an Observation resource, processing search parameters results in search queries executed by the persistence layer to resolve CodeSystem resource lookups:

[18/10/2021, 11:02:29:395 UTC] 00000030 FHIRPersisten 1   Processing SearchParameter resource: Observation, code: code, type: token, expression: AllergyIntolerance.code | AllergyIntolerance.reaction.substance | Condition.code | (DeviceRequest.code as CodeableConcept) | DiagnosticReport.code | FamilyMemberHistory.condition.code | List.code | Medication.code | (MedicationAdministration.medication as CodeableConcept) | (MedicationDispense.medication as CodeableConcept) | (MedicationRequest.medication as CodeableConcept) | (MedicationStatement.medication as CodeableConcept) | Observation.code | Procedure.code | ServiceRequest.code

Results in the following query:

[18/10/2021, 11:02:30:429 UTC] 00000030 QueryUtil     1        query string:
      SELECT COUNT(*) AS CNT
        FROM CodeSystem_LOGICAL_RESOURCES AS LR0
       WHERE LR0.IS_DELETED = 'N'
         AND EXISTS (
      SELECT 1
        FROM CodeSystem_LOGICAL_RESOURCES AS LR1
  INNER JOIN CodeSystem_STR_VALUES AS P2 ON P2.LOGICAL_RESOURCE_ID = LR1.LOGICAL_RESOURCE_ID
         AND P2.PARAMETER_NAME_ID = 20764
         AND (P2.STR_VALUE = ?)
       WHERE LR1.LOGICAL_RESOURCE_ID = LR0.LOGICAL_RESOURCE_ID)

In this case the search is for bindString(1, 'http://snomed.info/sct')

If we try the lookup directly from the registry, we see it is successful:

CodeSystem snomed =
                FHIRRegistry.getInstance().getResource("http://snomed.info/sct", CodeSystem.class);
        assertNotNull(snomed);

but we still drop through to the persistence layer to perform a search, which slows down ingestion considerably.

Environment 4.10.0

To Reproduce Ingest an Observation resource with a code value like:

   "code": {
        "coding": [{
            "system": "http://snomed.info/sct",
            "code": "363779003",
            "display": "Genotype determination"
        }],
        "text": "Diplotype Call"
    },

Expected behavior When the serverRegistryResourceProviderEnabled is true, the registry lookup should be sufficient and no search query is needed.

Additional context This needs to be addressed before any further performance measurement work is done.

punktilious commented 3 years ago

There are a couple of issues at play here. The first is the order in which the registry resource providers are searched. The database gets searched before the internal Snomed provider is searched. This means we always end up running a database search, which is expensive.

The second issue relates to looking up the default code-system names like http://hl7.org/fhir/observation-status, showing by the following stack:

com.ibm.fhir.server.registry.ServerRegistryResourceProvider.computeRegistryResources(ServerRegistryResourceProvider.java:115)
        at com.ibm.fhir.server.registry.ServerRegistryResourceProvider.lambda$0(ServerRegistryResourceProvider.java:67)
        at com.github.benmanes.caffeine.cache.LocalCache.lambda$statsAware$0(LocalCache.java:139)
        at com.github.benmanes.caffeine.cache.BoundedLocalCache.lambda$doComputeIfAbsent$14(BoundedLocalCache.java:2375)
        at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1908)
        at com.github.benmanes.caffeine.cache.BoundedLocalCache.doComputeIfAbsent(BoundedLocalCache.java:2373)
        at com.github.benmanes.caffeine.cache.BoundedLocalCache.computeIfAbsent(BoundedLocalCache.java:2356)
        at com.github.benmanes.caffeine.cache.LocalCache.computeIfAbsent(LocalCache.java:108)
        at com.ibm.fhir.server.registry.ServerRegistryResourceProvider.getRegistryResources(ServerRegistryResourceProvider.java:67)
        at com.ibm.fhir.registry.spi.AbstractRegistryResourceProvider.getRegistryResource(AbstractRegistryResourceProvider.java:25)
        at com.ibm.fhir.registry.FHIRRegistry.findRegistryResource(FHIRRegistry.java:279)
        at com.ibm.fhir.registry.FHIRRegistry.getResource(FHIRRegistry.java:162)
        at com.ibm.fhir.term.util.CodeSystemSupport.getCodeSystem(CodeSystemSupport.java:232)
        at com.ibm.fhir.term.util.CodeSystemSupport.isCaseSensitive(CodeSystemSupport.java:547)
        at com.ibm.fhir.persistence.jdbc.util.JDBCParameterBuildingVisitor.setTokenValues(JDBCParameterBuildingVisitor.java:984)
        at com.ibm.fhir.persistence.jdbc.util.JDBCParameterBuildingVisitor.visit(JDBCParameterBuildingVisitor.java:230)
        at com.ibm.fhir.model.type.Code.accept(Code.java:69)
        at com.ibm.fhir.model.visitor.Visitable.accept(Visitable.java:64)
        at com.ibm.fhir.persistence.jdbc.impl.FHIRPersistenceJDBCImpl.extractSearchParameters(FHIRPersistenceJDBCImpl.java:2075)
        at com.ibm.fhir.persistence.jdbc.impl.FHIRPersistenceJDBCImpl.updateWithMeta(FHIRPersistenceJDBCImpl.java:597)

The value is never found which means the non-result is never cached, so we have to perform the search every time.

lmsurpre commented 3 years ago

I opened #2863 for the http://hl7.org/fhir/observation-status one because its a little different from the SNOMED case.

punktilious commented 3 years ago

In the case of SNOMED, we do cache the result, although by default the time-to-live is very short. Need to see cache statistics in real-world scenarios to understand the performance impact. Without that insight, it's difficult to assess the real priority for this issue.

lmsurpre commented 2 years ago

With the caching thats in place, we should only pay this cost when the cache is stale. I think its working as designed, although there is work that could be done to change/control the ordering for the RegistryResourceProviders.