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

How to support pre-fetch tokens for objects in context? #377

Open
isaacvetter opened this issue Aug 1, 2018 · 12 comments
Open

How to support pre-fetch tokens for objects in context? #377

isaacvetter opened this issue Aug 1, 2018 · 12 comments

Comments

@isaacvetter
Copy link
Member

@isaacvetter isaacvetter commented Aug 1, 2018

As part of the specification of each hook, each field defined in context is explicitly labeled whether it can be used as a Prefetch Token.

Prefetch tokens are important because they are how a CDS service specifies any specific FHIR queries to be run as part of its pre-fetch during the request. A CDS service's discovery endpoint specifies it's pre-fetch as a pre-fetch template.

Currently, our pre-fetch token syntax is nice and simple - it only supports top-level elements. For example, a service responding to the [order-review](https://cds-hooks.org/hooks/order-review/) hook can request pre-fetch data about the patient, like so:

{
  "prefetch": {
    "p": "Patient/{{context.patientId}}"
  }
}

But the service has no way to request pre-fetch data that's based upon the orders object which is also sent in context. Unlike the simple, valued {{context.patientId}}, relating a prefetch template to the orders being sent would necessitate a prefetch key such as: {{context.orders.MedicationRequest.id[0]}} or something even more complicated using FHIRPath.

This inability to refer to objects in context is a gap.

Is there a simple, targeted method to enable this? What about something as straighforward as context...id returning a simple comma-delimited string of FHIR identifiers that could be subsequently used in a search?

@lmckenzi

This comment has been minimized.

Copy link
Contributor

@lmckenzi lmckenzi commented Aug 1, 2018

What Isaac actually proposed where it says context...id before Github intervened was context.<prefetch key>.<FHIR resource name>.id

I'd be in favor of this. The reason for putting resource names in is to allow you to search by each resource because the ids aren't unique across resource types. The comma-separated approach allows you to say something like DeviceRequest?_id={{context.orders.medicationRequest.id}}&_include=DeviceRequest.device

Though that does mean that your prefetch space is going to contain both device requests and devices, the former being redundant with what's already in the hook invocation.

@kpshek

This comment has been minimized.

Copy link
Member

@kpshek kpshek commented Aug 1, 2018

I'd be in favor of this. The reason for putting resource names in is to allow you to search by each resource because the ids aren't unique across resource types.

So is context.<prefetch key>.<FHIR resource name>.id only important in the case where the context value can multiple FHIR resource types?

In the hooks I've seen prototyped today, if the context field value relates to a FHIR resource, there is just a single resource in use so using context.<prefetch key>.id as the prefetch token is not ambiguous.

@lmckenzi

This comment has been minimized.

Copy link
Contributor

@lmckenzi lmckenzi commented Aug 1, 2018

Right. If the context element is limited to a single resource type, no need to include the resource name in the path (unless there was some reason we wanted consistency in context strings, perhaps to allow for future evolution of the hook?)

@kpshek

This comment has been minimized.

Copy link
Member

@kpshek kpshek commented Aug 1, 2018

Thanks for the clarification @lmckenzi.

There is no limitation of context fields to always be associated to a single FHIR resource type, but I would expect that to be the overwhelming majority of use cases. I think a context field relating to disparate FHIR resources is definitely an edge case.

As we wrap up 1.0, I don't see a big motivation in trying to further enhance prefetch semantics at this point. After 1.0, if we see a lot of implementer experience/use out of prefetch, then I think this is something worth looking out.

Personally, I don't think this is a problem space that warrants the additional complexity. I think most CDS Service implementers are looking at prefetch as a method to remove the need to make their own FHIR queries for convenience rather than on better merits. There are limited cases where prefetch is truly a benefit and any additional complexity we add to will further lessen the use cases in which prefetch was designed for (primarily performance considerations of in-memory or shared data access across services).

@lmckenzi

This comment has been minimized.

Copy link
Contributor

@lmckenzi lmckenzi commented Aug 1, 2018

I'm not sure "remove the need to make their own FHIR queries" is necessarily an illegitimate requirement. It may be that EHRs will choose not to satisfy the prefetch, but at least if the prefetch is computably expressible, an intermediary could do it for them. My concern is that if prefetch isn't possible, DaVinci is likely to push to drop CDS Hooks and proceed with a custom operation instead where they can demand inclusion of whatever data they like (and still have the flexibility to subsequently query through the inclusion of a hook-like token they can exercise). I don't think that would be a good outcome.

@kpshek

This comment has been minimized.

Copy link
Member

@kpshek kpshek commented Aug 1, 2018

Note the last part of my statement which was left out: "remove the need to make their own FHIR queries for convenience rather than on better merits". This critical part of my argument is what I'm pushing back against here (which is what I'm hearing is the reasoning behind this request). Please correct my understanding of the situation if I am not understanding this correctly.

My concern is that if prefetch isn't possible, DaVinci is likely to push to drop CDS Hooks and proceed with a custom operation instead where they can demand inclusion of whatever data they like (and still have the flexibility to subsequently query through the inclusion of a hook-like token they can exercise).

I may be further misunderstanding you, but these statements seem like an argument for introduce complexity and promoting a design/approach that I think does not stand on its own merits due to implementers that are prioritizing their convenience above good API/standards design. I know you don't agree with that as well so there must be a disconnect here.

@lmckenzi

This comment has been minimized.

Copy link
Contributor

@lmckenzi lmckenzi commented Aug 1, 2018

It's not totally clear to me that convenience doesn't itself have merit or that doing so necessarily constitutes bad API design, though I'm open to that perspective. One challenge that I have is that there isn't anyone who can convincingly make the case that prefetch for convenience/simplification for payers is a bad idea. (Isaac doesn't speak with as strong an opinion on this as you do :>) If I have to weigh the cost of enabling (but not mandating support for) more complex prefetch vs. DaVinci mandating that EHRs implement a custom operation instead, I know where I'd land. If we can convince payers they don't need prefetch, that'd be even better. However, I'm not confident of my ability to make that pitch given my own ambivilence on the question and the mindset they bring to the table from prior interfaces.

@eedrummer

This comment has been minimized.

Copy link

@eedrummer eedrummer commented Aug 7, 2018

I'll try to add some more context from Da Vinci to see if that helps justify the request or see if we are on the right path.

In our group, @lmckenzi has advocated that we stick with hooks that have been included in the specification, such as order-review. Given that, with the currently defined context and current capabilities of prefetch, we will be unable to send all of the information we know a payer will need to properly respond to a request.

That appears to leave us with two options if we want to be able to send all of the information necessary in a single request:

  1. Define a new hook that has a context with all of the information we need
  2. Enhance prefetch capabilities so that all of the desired information can be sent in the request

As I mentioned before, we have taken Option 1 off of the table to avoid hook proliferation and try to leverage current EHR implementation. We are pursuing Option 2 because payers in our group did not respond well to the suggestion of querying the EHR system.

I understand the benefits that querying the EHR can convey. It gives the payers future opportunities to make decisions on even more data than they may use today. However, our base use case with order-review forces us to query for information that is necessary to all CDS Services.

@lmckenzi

This comment has been minimized.

Copy link
Contributor

@lmckenzi lmckenzi commented Aug 7, 2018

The base usecase for prefetch - wanting to provide optimal access to data that may already be in memory, minimizing queries when providing data to multiple hook services, etc. all seems to elements associated to resources provided in context, not just those passed as an identifier in the context. The payer's current preference for prefetch/context data rather than querying their own is not a driver for this architectural change. The key is that the decision to include a full resource rather than just an identifier in context should not drive whether prefetch is possible. The decision of identifier vs. full resource should be driven only by whether we believe almost all consumers of the hook will need the full resource rather than just the id. Ensuring that prefetch is possible with both approaches is the best way of separating these concerns.

Whether some systems rely on prefetch or not (and whether they should) is a separate architectural/cultural question that will need to be addressed separately from the "Is it reasonable to enable prefetch at all here?" question.

@brynrhodes

This comment has been minimized.

Copy link
Contributor

@brynrhodes brynrhodes commented Oct 2, 2018

Proposed Disposition: Persuasive, the limitation on prefetch token references within prefetch templates will be relaxed to allow for context..[].id
Moved: Isaac Vetter, Second: Ken Kawamoto
Motion tabled to allow for more discussion

MedicationRequest?id={{context.orders.MedicationRequest.id}}
&_include=MedicationRequest:patient
&_include=MedicationRequest:intended-dispenser
&_include=MedicationRequest:requester:Practitioner
&_include=MedicationRequest:medication
&_include=MedicationRequest:on-behalf
&_include=MedicationRequest:insurance:Coverage

@lmckenzi

This comment has been minimized.

Copy link
Contributor

@lmckenzi lmckenzi commented Mar 9, 2019

I haven't seen more discussion - how do we move this forward?

@bvdh

This comment has been minimized.

Copy link
Contributor

@bvdh bvdh commented Mar 11, 2019

I think we should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.