tidepool-org / hub

[DEPRECATED] Central storage for Tidepool planning and issue tracking.
2 stars 2 forks source link

Add attribute to new messages, aka notes, pointing to a specific time in patient's dataset #29

Closed nicolashery closed 10 years ago

nicolashery commented 10 years ago

After working to integrate Blip, Clamshell, and Tideline, some issues came up where messages would not display the expected time, and not render at the correct place in the timeline. This proposal attempts to solve that. (Created from discussions with @jebeck and @jh-bate.)

Overview

Objective: Always display a note related to a specific event in patient's data (ex: a BG low) at the correct place, regardless of what timezone the user was when she created the note.

Currently, a message data model looks something like:

{
  "id": "1",
  "creationTimestamp": "2014-04-21T21:00:00Z",
  "creationOffsetMinutes": 420,
  "messageText": "What did you have for lunch?"
}

Where creationTimestamp and creationOffsetMinutes are the UTC time and user's timezone offset when the message was created (in our example it was 2pm California time).

We'd like to add a 3rd time attribute, explicitly indicating the time of the event we're commenting on:

{
  "id": "1",
  "pointsToDeviceTime": "2014-04-21T14:00:00"
  "creationTimestamp": "2014-04-21T21:00:00Z",
  "creationOffsetMinutes": 420,
  "messageText": "What did you have for lunch?"
}

(Note: Attribute names are subject to change.)

That attribute, pointsToDeviceTime, will be used to display the message at the correct location in Blip's timeline (in our example 2pm).

This way, the note could be created in Boston, and it would still display correctly:

{
  "id": "1",
  "pointsToDeviceTime": "2014-04-21T14:00:00"
  "creationTimestamp": "2014-04-21T21:00:00Z",
  "creationOffsetMinutes": 240,
  "messageText": "What did you have for lunch?"
}

In this example, it was 7pm in boston (UTC of 2014-04-21T21:00:00Z and Boston offset of 240), but the note still displays at 2pm in Blip's timeline.

This only applies to new messages starting a thread (aka "notes"). Comments are already linked to the note via a parentMessage attribute:

[
  {
    "id": "1",
    "pointsToDeviceTime": "2014-04-21T14:00:00"
    "creationTimestamp": "2014-04-21T21:00:00Z",
    "creationOffsetMinutes": 240,
    "messageText": "What did you have for lunch?"
  },
  {
    "id": "2",
    "parentMessage": "1"
    "creationTimestamp": "2014-04-21T21:30:00Z",
    "creationOffsetMinutes": 420,
    "messageText": "I didn't finish my plate and forgot to adjust my bolus :("
  }
]

As we see in the thread above, the creation* attributes are still useful to sort messages in a single thread. The note was created at 7pm Boston time, and the reply came at 2:30pm California time, but they are kept in the correct order thanks to the UTC creation time.

Reasoning

When you think about it, notes are really "tied" to something else: a specific point in time in the dataset of the patient concerned. Unlike say, a new tweet or Facebook post, notes don't live on their own, only tied to the time of their creation. So it makes sense to have an attribute pointing to that specific time in the patient's dataset.

The solution proposed tries to be future-proof and at the same time stay as simple as possible. The core idea followed is "don't loose information you'll probably need in the future and that would be impossible to get or derive if you don't record it now".

Indeed, on note creation there are two pieces of information coming from the user:

We try to keep all that info with the proposed attributes.

As a side note (no pun intended), these are not entirely new ideas. It was expected to have to tackle this kind of problem when work started on Tideline and with the fact that we are dealing with both timezone-unaware devices and timezone-aware browsers.

Creating the new attribute

How do we generate this pointsToDeviceTime attribute from the apps? It's actually pretty straightforward.

In Blip:

In Clamshell:

Since we're separating when a note is created, and what (time in dataset) it's related to, we could imagine separating that in the UI (Blip and Clamshell).

The time in dataset the note is pointing to will always be the same, regardless of where I'm viewing it from in the world. However, the time of the note's creation and the replies in the thread would be in my browser's current timezone (like a Facebook or Twitter feed.)

Example as seen from a browser in California:

Note for **Mary Smith**'s data on **April 21 at 2pm**

Posted by **Charles West** on **April 21 at 2pm**
What did you have for lunch?

Reply from **Mary Smith** on **April 21 at 2:30pm**
I didn't finish my plate and forgot to adjust my bolus :(

Same example as seen from a browser in Boston:

Note for **Mary Smith**'s data on **April 21 at 2pm**

Posted by **Charles West** on **April 21 at 7pm**
What did you have for lunch?

Reply from **Mary Smith** on **April 21 at 7:30pm**
I didn't finish my plate and forgot to adjust my bolus :(

Impact

Changes will be required for (they should be relatively small):

jh-bate commented 10 years ago

currently a message looks like this

{
    id: '1'
    parentmessage : null,
    userid: '99',
    groupid: '99',
    timestamp: '2014-04-24T11:18:41.183Z',
    messagetext: 'In three words I can sum up everything I have learned about life: it goes on.'
}
nicolashery commented 10 years ago

Yes thanks for the precision @jh-bate. I should of said more explicitly, but I did deviate a bit from the actual model for the purpose of the examples.

cheddar commented 10 years ago

Ok, if I understand this correctly, you are proposing that we add a new timestamp field that will act as a reference into the data stream. Your reasoning for this is because most notes done via the blip app are actually going to be in response to some datum that is being visualized instead of needing to be associated with the most-recent position in the stream.

This proposal is actually two things.

  1. It is an attempted solution to the fact that it's kinda hard to visualize multiple events correctly on the same timeline if one of them has a timezone and the other one doesn't.
  2. It is a proposal for how to create messages and message threads about data points in the past.

I'm going to talk about these separately, and actually start with the second.

Taking it back to computer science principles, you are proposing that we add what is called a "foreign key". I.e. a "tag" that associates pieces of data from X with pieces of data from Y. You are proposing that this "foreign key" be a timestamp without a timezone, such that we can generate it on apps without actually having the datum to point back to.

In general, it is poor practice to use an imprecise foreign key. Foreign keys should exactly point to the datum that you care about. Using a "guesstimate" timestamp is rather imprecise because there are many ways that the timestamp can be different from the event. And if the event is ever decided to actually occur at a different time, it is not possible to also adjust the comment thread along with it. So this is actually a poor solution for the second problem. If we are introducing a foreign key in order to associate a message thread with specific events, then we should be using the specific event's id as the foreign key. It also turns out that this feature is something that can be done after-the-fact and is orthogonal to the question of aligning events on a timeline.

That does it for the response to (2). Onto (1).

As a solution to (1), it does resolve the immediate effects of our decision to store device data without a timezone. What it also does is propogates the choice to not store a timezone into yet another service. When we made the decision to not store the timezone on device data, it was done with the understanding that it is incorrect in every conceivable way and something we would have to back out at some point in the future. Continuing to propogate that incorrect decision will simply serve to increase the surface area that has to be affected when backing it out.

Secondarily, it adds a rather unintuitive interaction to our messaging app that requires the messaging app ask the user what timezone the message is in. This means that users will constantly need to select the right timezone when creating messages. They will inevitably get it wrong at some point and be rather frustrated.

Thirdly, I'd like to reiterate that the reason we have this problem right now is because we are not storing device data in the UTC timezone. As you mention, @nicolashery, we knew about the potential mis-match between the timestamps of the device data and messages well in advance. Which is exactly why there should have been plenty of time to make it so that it works correctly given the broken timestamps on the device data. It appears that given the 3 months we have had to make this work right, we have been unable to actually do it.

I am vehemently opposed to this proposal. I believe it is a move towards further ingraining the poor choice of not storing a timezone rather than a step towards helping us get to a world where timezones are attached to all timestamps.

As a counter proposal, I believe that we should implement a solution that is purely client-side. It goes

  1. Blip gets messages from message-api in UTC
  2. Blip applies the local timezone from the browser to the timestamp
  3. Blip renders the message using the "local" timestamp

This will produce a correct view for people as long as they are viewing the data from a location that is in the same timezone as their actual devices. If they are viewing the data from a different timezone, the messages will not line up correctly, but I believe that is the best we can hope for given that we are not storing the device data in UTC.

We should also follow-up quickly with some adjustments to the product such that we are converting all uploaded data into UTC timestamps and visualizing from those instead.

jebeck commented 10 years ago

So after much thought today, I would like to add a point of clarification: the idea @nicolashery (and I) were trying to get at in a preliminary fashion was the need to store an additional piece of information in order to express the relationship of a message to its subject (data) more precisely, since we can't rely on a time correspondence (until we are able to store device data as UTC, which I believe is a day that will be long in coming; I will be happy to be proven wrong, but until that day everything I say in these debates will assume that we are not yet able to do that, because we aren't).

In particular, @nicolashery suggested a pointsToDeviceTime attribute as the link between a message and the data under discussion, and @cheddar pointed out a potential flaw in that, but the proposal to store a particular event's id is not, IMO, fundamentally different, just a better implementation. (Although @brandonarbiter pointed out that requiring a correspondence between a message and a datum will remove the possibility of commenting on a time point where there is no data, and he thought there might be use cases for that. Also I realize @cheddar doesn't want us to go with that type of strategy, but again, I don't think UTC device data storage is going to be possible at all soon.) Storing the id of the datum under discussion is still an additional piece of information that seeks to anchor a message not just within the ongoing stream of time, but also within the collection of data that is the subject of visualization/discussion/therapy/etc. I believe it would be a good medium-term solution (post-pilot, I suppose, but before we can store UTC device data), and I believe we will need a medium-term solution.

brandonarbiter commented 10 years ago

@jebeck is the idea of linking a comment directly to a datum only relevant for comments added within Tideline (as opposed to Clam Shell)? I ask because:

  1. Clam Shell does not have real time data visibility, so there's no way to link a comment in Clam Shell to a datum
  2. In Tideline, the comment will be associated with a timestamp because you add it while looking at a graph of diabetes data. These are retrospective adds. I can't imagine the benefit of tying the comment to a diabetes datum rather than a timestamp associated with the x-axis you're already looking at.

In related news, I began playing around with Clam Shell to day and encountered a problem that I anticipate will be a big one. :) All comments are expected to be real time. There's no way to add a comment at day's end and change the timestamp. I found this to be the biggest problem with Nico and my first joint implementation of Nutshell, so i wanted to point it out as early as possible here.

Brandon Arbiter VP, Product + Biz Dev Tidepool http://www.tidepool.org | brandon@tidepool.org 917.536.0505 (m)

On Thu, Apr 24, 2014 at 8:44 PM, Jana Beck notifications@github.com wrote:

So after much thought today, I would like to add a point of clarification: the idea @nicolashery https://github.com/nicolashery (and I) were trying to get at in a preliminary fashion was the need to store an additional piece of information in order to express the relationship of a message to its subject (data) more precisely, since we can't rely on a time correspondence (until we are able to store device data as UTC, which I believe is a day that will be long in coming; I will be happy to be proven wrong, but until that day everything I say in these debates will assume that we are not yet able to do that, because we aren't).

In particular, @nicolashery https://github.com/nicolashery suggested a pointsToDeviceTime attribute as the link between a message and the data under discussion, and @cheddar https://github.com/cheddar pointed out a potential flaw in that, but the proposal to store a particular event's idis not, IMO, fundamentally different, just a better implementation. (Although @brandonarbiter https://github.com/brandonarbiter pointed out that requiring a correspondence between a message and a datum will remove the possibility of commenting on a time point where there is no data, and he thought there might be use cases for that. Also I realize @cheddarhttps://github.com/cheddardoesn't want us to go with that type of strategy, but again, I don't think UTC device data storage is going to be possible at all soon.) Storing the id of the datum under discussion is still an additional piece of information that seeks to anchor a message not just within the ongoing stream of time, but also within the collection of data that is the subject of visualization/discussion/therapy/etc. I believe it would be a good medium-term solution (post-pilot, I suppose, but before we can store UTC device data), and I believe we will need a medium-term solution.

Reply to this email directly or view it on GitHubhttps://github.com/tidepool-org/hub/issues/29#issuecomment-41356515 .

jebeck commented 10 years ago

Yes, linking to a datum is only relevant when adding (historical) comments in blip.

However, you are incorrect that in tideline "the comment will be associated with a timestamp" - this is the whole crux of the issue. Tideline timestamps are timezone-naive, they exist in a world of their own, while messages have pointers (UTC timestamps) anchoring them in the real world, and marrying the two is a very difficult task. The solution I proposed involving storing timezone offsets allows us to precisely recreate the tideline timestamp (which is 100% accurate within the tideline "world' because that's where it was born - when you clicked on the relevant pool) by "remembering" how we translated it into our best guess UTC time for storage (by applying the browser's timezone offset at the time of message creation). If we store this transformation, we can reverse it and guarantee that the message will always be placed at the same location within tideline.

jebeck commented 10 years ago

@brandonarbiter I responded above ^

cheddar commented 10 years ago

So, on the points of use cases. After conversation with @HowardLook some more yesterday I realized there are two use cases for retrospective annotations of things.

  1. I want to annotate a point in time saying that I started playing soccer, for example. This is a piece of information that it is important to have and is not tied specifically to a datum. It's "foreign key" needs to be the timestamp and that timestamp needs to be accurate with the actual world-time that the event happened in (i.e. UTC)
  2. I had a low and I want to annotate why I believe I went low. I want to annotate the datum, not the timestamp, here because we are envisioning a world where people might correct the timestamps on their data. If I make an annotation at the incorrect timestamp and then change the datum to the correct timestamp, now my annotation and my data do not line up. I really want the annotation to move along with the datum. If we only support the first use case, then it will be impossible to actually move annotations as well.

The requirement for (1) screams to me that we need to be applying hack upon hack to convert the timestamps that we get from the device makers into UTC time rather than applying hack upon hack to work within a made up world of fake timestamps.

@brandonarbiter On the clamshell "only realtime" issue

  1. Please don't derail the thread. This is a different issue and it should be discussed elsewhere
  2. While you are creating the other issue, keep in mind that it was impossible to create message threads in the past with the blip-mvp release that used FB groups. When we set out on this pilot thing, we were targeting feature parity and that is what we have delivered. New features should be prioritized and discussed as new features, not as bugs.
brandonarbiter commented 10 years ago

Thanks @cheddar - I wasn't sure whether "only realtime" should be it's own Issue, so I appreciate your clarification to make it so. I just created Clamshell Issue 10 https://github.com/tidepool-org/clamshell/issues/10: "In Clamshell, as a patient I should be able to add a note at my convenience and adjust the timestamp to reflect when the event actually happened." This has been assigned to @jh-bate

With regard to the two use cases you and @howardlook derived for retrospective analysis, "foreign key to timestamp" and "foreign key to event", in theory I agree with them. In a world of perfect timestamps, I would say there is little-to-no value to explicitly relating a comment to a diabetes datum, like a low BG. This relationship would be created through visual proximity. But to your point, when we aspire to timestamp corrections for data coming from some devices and not others, maintaining relativity is important.

Brandon Arbiter VP, Product + Biz Dev Tidepool http://www.tidepool.org | brandon@tidepool.org 917.536.0505 (m)

On Fri, Apr 25, 2014 at 8:31 AM, cheddar notifications@github.com wrote:

So, on the points of use cases. After conversation with @HowardLookhttps://github.com/HowardLooksome more yesterday I realized there are two use cases for retrospective annotations of things.

  1. I want to annotate a point in time saying that I started playing soccer, for example. This is a piece of information that it is important to have and is not tied specifically to a datum. It's "foreign key" needs to be the timestamp and that timestamp needs to be accurate with the actual world-time that the event happened in (i.e. UTC)
  2. I had a low and I want to annotate why I believe I went low. I want to annotate the datum, not the timestamp, here because we are envisioning a world where people might correct the timestamps on their data. If I make an annotation at the incorrect timestamp and then change the datum to the correct timestamp, now my annotation and my data do not line up. I really want the annotation to move along with the datum. If we only support the first use case, then it will be impossible to actually move annotations as well.

The requirement for (1) screams to me that we need to be applying hack upon hack to convert the timestamps that we get from the device makers into UTC time rather than applying hack upon hack to work within a made up world of fake timestamps.

@brandonarbiter https://github.com/brandonarbiter On the clamshell "only realtime" issue

  1. Please don't derail the thread. This is a different issue and it should be discussed elsewhere
  2. While you are creating the other issue, keep in mind that it was impossible to create message threads in the past with the blip-mvp release that used FB groups. When we set out on this pilot thing, we were targeting feature parity and that is what we have delivered. New features should be prioritized and discussed as new features, not as bugs.

Reply to this email directly or view it on GitHubhttps://github.com/tidepool-org/hub/issues/29#issuecomment-41405812 .

cheddar commented 10 years ago

@brandonarbiter thanks for the other ticket. Sorry for the strong wording, I know you weren't trying to derail the thread. I got really defensive because I was scared of feature/scope creep and let that override my judgement.

kentquirk commented 10 years ago

So it's 1AM after an 18 hour day and I'm not at my best, but I want to jump in and say that I largely agree with Cheddar, and I'm going to make a couple of points just to get people thinking about it.

1) Timezoneless time has no real meaning and we have been chasing bug after bug (and applying "hack after hack") to deal with it. We should do everything in our power to avoid propagating timezoneless time to other systems.

2) We don't have to wait until there are devices that have a time zone. What we need to do is make our best guess at a time correction -- for every datapoint -- when we import the data, AND have a tool that allows users to adjust the correction. We can't have one without the other. So we need to figure out what our UI will be for letting people make those adjustments.

Please don't have discussions about the long term strategy for managing time without involving all three of Cheddar, Jana, and me. Perhaps the three of us should figure out a strategy, and then we can work out our plan to get there.

Thanks.

Kent

On Fri, Apr 25, 2014 at 9:30 AM, cheddar notifications@github.com wrote:

@brandonarbiter https://github.com/brandonarbiter thanks for the other ticket. Sorry for the strong wording, I know you weren't trying to derail the thread. I got really defensive because I was scared of feature/scope creep and let that override my judgement.

Reply to this email directly or view it on GitHubhttps://github.com/tidepool-org/hub/issues/29#issuecomment-41412412 .

Kent Quirk VP of Engineering, Tidepool

Tidepool is an open source, not-for-profit effort to build an open data platform and better applications to reduce the burden of Type 1 Diabetes.

brandonarbiter commented 10 years ago

Closing on Jana's recommendation. This thread has become more of a discussion than a task. We are going to follow up with an internal Tidepool meeting on May 14 to regroup.