Replay time ordering #405

lmeier · 2024-10-21T19:20:24Z

We do 2 things here:

for all tracks we include the updatedAtMsDiff, which will be used to determine the location.updatedAt
we modify updatedAtMsDiff in RadarAPIHelper to update updatedAtMsDiff at request time, as updatedAtMsDiff is meant to represent the difference between the client request time and the timestamp of the location processed by the SDK. This is an important fix to locations being replayed as previously their updatedAtMsDiff was determined by their original (failed) request time — not actually the time at successful request.

RadarSDK/RadarAPIClient.m

RadarSDK/RadarAPIHelper.m

ShiCheng-Lu · 2024-10-25T17:25:35Z

I think on here we want to save the location's time (timeInMs) instead of the request time for the replay

ShiCheng-Lu · 2024-10-25T17:39:21Z

I think on here we want to save the location's time (timeInMs) instead of the request time for the replay

nvm, this is not going to be used. Maybe we should get rid of the timestamps that will no longer be used to avoid future confusion. Because this doesn't need to be back-compatible with server, only server need to be back compatible with sdks.

RadarSDK/RadarAPIHelper.m

ShiCheng-Lu · 2024-10-25T19:52:18Z

RadarSDK/RadarAPIClient.m

-        long timeInMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);
+    long timeInMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);
+    RadarTrackingOptions *options = [Radar getTrackingOptions];
+    if (!foreground || options.replay != RadarTrackingOptionsReplayNone) {
        params[@"updatedAtMsDiff"] = @(nowMs - timeInMs);


all track requests should have this updatedAtMsDiff, likely = 0 for normal tracks, but it'll keep our server code path consistent. We also don't need to set locationMs in the request as its not used, a bit of wasted bandwidth, we can send it at line:417 if the request fails.

we can basically think that all tracks are replays, but some are replayed immediately.

We toss in the locationMs for updating the updatedAtMsDiff in RadarAPIHelper.

I had initial hesitancy towards making all /tracks have updatedAtMsDiff, but yea, your model that they'd all have it matches mine. Might as well change that.

RadarSDK/RadarAPIClient.m

RadarSDK.podspec

RadarSDK/RadarAPIClient.m

tjulien · 2024-11-08T15:06:52Z

RadarSDK/RadarAPIClient.m

-        params[@"updatedAtMsDiff"] = @(nowMs - timeInMs);
+    long locationMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);
+    RadarTrackingOptions *options = [Radar getTrackingOptions];
+    if (!verified || !foreground) {


so verified background reqs will use updatedAtMsDiff, is that intended?
Are we intending to change how we treat foreground timestamps or holding off on that bc it's a bigger change not needed for the fix?

Would recommend updating the PR description with a high level description of the changes to make it easier to understand.

Yes that's correct. Per slack conversation with Nick, the intention is to not change any timestamp logic for verified requests.

All non verified /tracks will now use updatedAtMsDiff, both foreground and background. There's no reason to hold off on making (non-verified) foreground requests use this logic -- in fact it's required for making sure that foreground requests slot in correctly in between background requests.

Ooo yea, good call out re: pr description -- just updated it.

should the code be:

if (!verified) {

as is it doesn't match your comment. Unless I'm missing something

We do want background verified tracks to use the existing logic, i.e. if (!foreground) ...

So either if (!verified || !foreground) {

Does that make sense?

tjulien · 2024-11-08T15:32:12Z

@lmeier what's the linear for this?

RadarSDK/RadarNotificationHelper.m

lmeier · 2024-11-08T16:11:30Z

@lmeier what's the linear for this?

https://linear.app/radarlabs/issue/FENCE-2118/fix-trackreplay-time-ordering

This reverts commit 19dfb83.

tjulien · 2024-12-05T11:36:34Z

RadarSDK/RadarAPIHelper.m

@@ -67,9 +67,38 @@ - (void)requestWithMethod:(NSString *)method
            }

            if (params) {
-                [req setHTTPBody:[NSJSONSerialization dataWithJSONObject:params options:0 error:NULL]];
+                if (!params[@"updatedAtMsDiff"] && !params[@"replays"]) {


nit: it would be nice if iOS and Android had the same logic for this if case

Good callout, fixed: 1cd42d9

tjulien · 2024-12-05T11:47:11Z

RadarSDK/RadarAPIClient.m

-        long timeInMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);
-        params[@"updatedAtMsDiff"] = @(nowMs - timeInMs);
-    }
+    long locationMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);


ok so I guess the approach now is:
• change foreground = true to also use updatedAtMsDiff (for consistency? I might have been the one to bring this up?)
• Don't special case verified = true on the client side, handle it differently on the server side

If we decided not to change foreground = true to updatedAtMsDiff, would the other changes to updatedAtMsDiff in these PRs be enough to fix the bug? Or is the inconsistent behavior between foreground = true and foreground = false causing a problem?

change foreground = true to also use updatedAtMsDiff (for consistency? I might have been the one to bring this up?)

If we decided not to change foreground = true to updatedAtMsDiff, would the other changes to updatedAtMsDiff in these PRs be enough to fix the bug? Or is the inconsistent behavior between foreground = true and foreground = false causing a problem?

Yes, for three reasons:

not using updatedAtMsDiff causes a problem when a foreground location gets forced to server time while background locations are adjusted per updatedAtMsDiff, leading to inconsistent ordering (a subsequent background location can be brought erroneously before the foreground location by the updatedAtMsDiff adjustment). So no, the other updatedAtMsDiff changes would be not sufficient to fully fix the bug.

for consistency

because there's no empirical reason to specially trust foreground locations from the OS as time-correct. It's true that foreground locations aren't subject to delays related to backgrounding, but the OS can and does deliver locations when foregrounded that have a non now timestamp

• Don't special case verified = true on the client side, handle it differently on the server side

Correct, per slack conversation. I'll link in #mobile

tjulien · 2024-12-08T18:56:42Z

RadarSDK/RadarAPIHelper.m

@@ -67,9 +67,39 @@ - (void)requestWithMethod:(NSString *)method
            }

            if (params) {
-                [req setHTTPBody:[NSJSONSerialization dataWithJSONObject:params options:0 error:NULL]];
+                NSNumber *prevUpdatedAtMsDiff = params[@"updatedAtMsDiff"];


Now that we're collecting locationMS we could also pass up updatedAt (we probably already are) we could do this bookkeeping server side but I think let's do that in a v2 in the interest of getting this out.

lmeier added 5 commits October 20, 2024 20:03

Include locationMs, and replayRequestMs

83ace70

Ensure updatedAtMsDiff is correct

233ccc2

bump version

3b84c99

fix ordering

2e7bbbd

correct version

109283d

lmeier requested a review from KennyHuRadar October 21, 2024 19:20

Add logging of visits

9ee710f

KennyHuRadar reviewed Oct 24, 2024

View reviewed changes

RadarSDK/RadarAPIClient.m Outdated Show resolved Hide resolved

tjulien reviewed Oct 25, 2024

View reviewed changes

RadarSDK/RadarAPIHelper.m Outdated Show resolved Hide resolved

simplify to just updatedAtMsDiff

dcd18b8

ShiCheng-Lu reviewed Oct 25, 2024

View reviewed changes

RadarSDK/RadarAPIHelper.m Show resolved Hide resolved

ShiCheng-Lu reviewed Oct 25, 2024

View reviewed changes

lmeier added 4 commits October 25, 2024 16:02

fix lint error

531feb1

always include updatedAtMsDiff

ce6fb3a

log cleanup

19dfb83

Merge branch 'master' into replayTimestamps

b6a5ee8

lmeier marked this pull request as ready for review October 28, 2024 19:19

nickpatrick reviewed Nov 5, 2024

View reviewed changes

RadarSDK/RadarAPIClient.m Outdated Show resolved Hide resolved

lmeier added 2 commits November 7, 2024 10:17

timeInMs --> locationMs, don't send updatedAtMsDiff if verified

cc8b817

extra bracket

161cba4

KennyHuRadar reviewed Nov 7, 2024

View reviewed changes

RadarSDK.podspec Outdated Show resolved Hide resolved

KennyHuRadar reviewed Nov 7, 2024

View reviewed changes

RadarSDK/RadarAPIClient.m Outdated Show resolved Hide resolved

lmeier and others added 4 commits November 7, 2024 10:38

nit

8d7bdd6

Merge branch 'master' into replayTimestamps

a214c59

not verified or not foreground

2d22f43

version bump

9b1432c

tjulien reviewed Nov 8, 2024

View reviewed changes

RadarSDK/RadarNotificationHelper.m Outdated Show resolved Hide resolved

lmeier and others added 5 commits November 8, 2024 13:58

don't conditionalize for verified

a3818cf

Revert "log cleanup"

63d548d

This reverts commit 19dfb83.

don't conditionalize updatedAtMsDiff at all

04bae9e

make api helper logic more clear

910d489

Merge branch 'master' into replayTimestamps

ad35612

tjulien reviewed Dec 5, 2024

View reviewed changes

Reformat to more closely match android impl

1cd42d9

lmeier force-pushed the replayTimestamps branch from 8c60f12 to 1cd42d9 Compare December 5, 2024 16:36

lmeier added 3 commits December 5, 2024 11:49

Make nil check explict

b138b15

put behind a feature setting

1f5c6d0

Typo

1c7ca58

tjulien reviewed Dec 8, 2024

View reviewed changes

tjulien approved these changes Dec 8, 2024

View reviewed changes

KennyHuRadar and others added 2 commits December 9, 2024 15:33

Merge branch 'master' into replayTimestamps

d3c24f7

bump version

f06ebe5

KennyHuRadar merged commit 65844f2 into master Dec 9, 2024
2 checks passed

KennyHuRadar deleted the replayTimestamps branch December 9, 2024 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replay time ordering #405

Replay time ordering #405

lmeier commented Oct 21, 2024 •

edited

Loading

ShiCheng-Lu commented Oct 25, 2024

ShiCheng-Lu commented Oct 25, 2024 •

edited

Loading

ShiCheng-Lu Oct 25, 2024

ShiCheng-Lu Oct 25, 2024

lmeier Oct 25, 2024

lmeier Oct 25, 2024

tjulien Nov 8, 2024

lmeier Nov 8, 2024

tjulien Nov 8, 2024

lmeier Nov 8, 2024

tjulien commented Nov 8, 2024

lmeier commented Nov 8, 2024

tjulien Dec 5, 2024 •

edited

Loading

lmeier Dec 5, 2024

tjulien Dec 5, 2024

lmeier Dec 5, 2024 •

edited

Loading

lmeier Dec 5, 2024

tjulien Dec 8, 2024

Replay time ordering #405

Replay time ordering #405

Conversation

lmeier commented Oct 21, 2024 • edited Loading

ShiCheng-Lu commented Oct 25, 2024

ShiCheng-Lu commented Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjulien commented Nov 8, 2024

lmeier commented Nov 8, 2024

tjulien Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmeier Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmeier commented Oct 21, 2024 •

edited

Loading

ShiCheng-Lu commented Oct 25, 2024 •

edited

Loading

tjulien Dec 5, 2024 •

edited

Loading

lmeier Dec 5, 2024 •

edited

Loading