Skip to content
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

search parameter extraction leads to registry lookup for implicit systems #2863

Closed
lmsurpre opened this issue Oct 18, 2021 · 3 comments
Closed
Assignees
Labels
bug Something isn't working P2 Priority 2 - Should Have performance performance reindex Resolution of issue will require a $reindex during upgrade

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Oct 18, 2021

Describe the bug
We store (and retrieve) code values with their implicit system value. This makes it much faster to search for a value like 'status=active' because we can bind it to the particular system that the active code is associated with (instead of doing a system-less search in the db for any code with a value of "active").

However, because we're now setting this implicit system, our extraction logic is doing a CodeSystem lookup to determine whether this CodeSystem is case-sensitive or not. Since the system doesn't exist, it never gets cached, and we end up looking for it each time. When ServerRegistryResourceProvider is enabled, that can be expensive because it makes a roundtrip to the db during the lookup.

Environment
main

To Reproduce
Steps to reproduce the behavior:

  1. enable the ServerRegistryResourceProvider and turn up logging
  2. ingest an observation
  3. note that the system performs a search for a CodeSystem with a url of http://hl7.org/fhir/observation-status
  4. do it again

note that the ServerRegistryResourceProvider performs this search each time because none of the providers actually have a CodeSystem for this URL.

Expected behavior
We shouldn't be looking up these implicit CodeSystem resources because

  1. they're never going to exist (unless we invent them and register them in our registry)
  2. there shouldn't be any reason that we actually need them

Additional context
Per the following stack trace, it turns out that the lookup is actually invoked from our code that is trying to determine whether these codesystems are case-sensitive or not:

[18/10/2021, 13:22:08:412 UTC] 00000038 FHIRPersisten 1   Processing SearchParameter resource: Observation, code: status, type: token, expression: Observation.status
[18/10/2021, 13:22:08:414 UTC] 00000038 ServerRegistr I   URL IS:http://hl7.org/fhir/observation-status
                                 java.lang.Exception
        at 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)
@lmsurpre lmsurpre added the bug Something isn't working label Oct 18, 2021
@lmsurpre
Copy link
Member Author

lmsurpre commented Oct 18, 2021

Per team discussion, we think that it will be safe to treat all code datatype as case-sensitive, so long as that gets documented in our Conformance.md

One thing I noticed while investigating this issue is that we use a different technique for the implicit system on extraction vs search.
On the extraction side we use ModelSupport.getSystem(code) to get the code's implicit system name.
However, that isn't feasible on the search side, and so there we are using the presence of a SearchParameter extension instead.

We should consider falling back to this same search parameter extension logic in the case of an extraction where the implicit system does not exist in the model (for example, for an Extension whose value is a code that has a binding to codes from a particular system).

@lmsurpre lmsurpre added this to the Sprint 2021-14 milestone Oct 18, 2021
@lmsurpre lmsurpre added performance performance P2 Priority 2 - Should Have labels Oct 18, 2021
@lmsurpre lmsurpre changed the title search parameter extraction leads to lookup for implicit systems search parameter extraction leads to registry lookup for implicit systems Oct 18, 2021
@punktilious punktilious self-assigned this Oct 22, 2021
@punktilious punktilious added the reindex Resolution of issue will require a $reindex during upgrade label Oct 22, 2021
@punktilious
Copy link
Collaborator

Although this issue is tagged as requiring reindex, this is a very narrow edge case and it is unlikely that customers in general would require this. The release notes should reflect that the fact that a reindex is only required if customers need to rely on the new fallback implicit system functionality.

@prb112
Copy link
Contributor

prb112 commented Oct 25, 2021

QA complete

@prb112 prb112 closed this as completed Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P2 Priority 2 - Should Have performance performance reindex Resolution of issue will require a $reindex during upgrade
Projects
None yet
Development

No branches or pull requests

3 participants