e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 34 forks source link

Labels sometimes taking minutes to "stick" #948

Open shankari opened 1 year ago

shankari commented 1 year ago

While testing on my personal phone, I sometimes find that it takes over a minute for the labels to "stick"

Concretely:

shankari commented 1 year ago

I wonder if the error about the pipeline range timeout is related. I vaguely remember getting the same error the previous time I had extreme delay. I have noticed that the trip load is really slow as well. But once we have finished the trip load, why are we making other calls to the pipeline range?

shankari commented 1 year ago

I'm going to create an internal issue for this so that I can link videos and downloaded logs without having to worry about leaking my information any more than it already has been.

shankari commented 1 year ago

After the internal investigation, it looks like the issue is that the way the react hooks are set up, we re-retrieve the pipeline range and the confirm objects from the phone every time the user sets a label, and the user label dont stick properly if this remote call is delayed.

Matching up server and client calls, we see

137692,1691900829.457,2023-08-12T21:27:09.457000-07:00,BuiltinUserCache : Added value for key manual/mode_confirm at time 1.691900829455E9
137693,1691900829.535,2023-08-12T21:27:09.535000-07:00,"ConnectionSettings : in getConnectURL, connectionSettings =

1691900831458,"2023-08-13 04:27:11,458:DEBUG:139761210832448:START POST /pipeline/get_range_ts"
-----

137708,1691900832.876,2023-08-12T21:27:12.876000-07:00,BuiltinUserCache : Added value for key manual/purpose_confirm at time 1.691900832865E9
137709,1691900832.9,2023-08-12T21:27:12.900000-07:00,"ConnectionSettings : in getConnectURL, connectionSettings =

1691900834916,"2023-08-13 04:27:14,916:DEBUG:139761200342592:START POST /pipeline/get_range_ts"

This has to be fixed before we go to production

JGreenlee commented 1 year ago

why are we making other calls to the pipeline range?

This is not the expected behavior, but I am investigating why it happens.


When a trip is labeled, we call repopulateTimelineEntry(): https://github.com/e-mission/e-mission-phone/blob/b2d0a21c66a48a7829e76ee3402e8dd9ceae5ebc/www/js/diary/LabelTab.tsx#L233

Which will re-match labels to that timeline entry, including any new ones.

To do this it calls getUnprocessedLabels() from services.js and passes the unprocessed labels to the label factory for it to be matched.

JGreenlee commented 1 year ago

What I didn't realize, and is causing this issue, is that getUnprocessedLabels() retrieves the pipeline range every time it is called. https://github.com/e-mission/e-mission-phone/blob/b2d0a21c66a48a7829e76ee3402e8dd9ceae5ebc/www/js/diary/services.js#L137

JGreenlee commented 1 year ago

So this fix should not be too difficult.

We can split getUnprocessedLabels() into 2 functions - one that retrieves the pipelineRange and one that uses that pipelineRange to get unprocessed labels.

And name them accordingly

JGreenlee commented 1 year ago

I see there's already a dedicated function for retrieving the pipelineRange in CommHelper (getPipelineRangeTs()), so we can just call that directly from LabelTab on init / on refresh, and pass the pipelineRange as an argument into getUnprocessedLabels().

There is no Diary tab to worry about anymore, getPipelineRangeTs() will only be called from one place.

This way, in repopulateTimelineEntry() we can use the existing pipelineRange.


I'm going to put the new getUnprocessedLabels() into the new Typescript file timelineHelper.ts because there's no point in writing Angular functions right now.

JGreenlee commented 1 year ago

Fix in https://github.com/e-mission/e-mission-phone/pull/1013

shankari commented 1 year ago

https://github.com/e-mission/e-mission-phone/pull/1013 merged. I think we still want to investigate the "react way" of in-memory caching, but we can track that in a separate issue. Once we file that issue, we can close this one.