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

Replay time ordering #405

Merged
merged 28 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
83ace70
Include locationMs, and replayRequestMs
lmeier Oct 21, 2024
233ccc2
Ensure updatedAtMsDiff is correct
lmeier Oct 21, 2024
3b84c99
bump version
lmeier Oct 21, 2024
2e7bbbd
fix ordering
lmeier Oct 21, 2024
109283d
correct version
lmeier Oct 21, 2024
9ee710f
Add logging of visits
lmeier Oct 23, 2024
dcd18b8
simplify to just updatedAtMsDiff
lmeier Oct 25, 2024
531feb1
fix lint error
lmeier Oct 25, 2024
ce6fb3a
always include updatedAtMsDiff
lmeier Oct 25, 2024
19dfb83
log cleanup
lmeier Oct 28, 2024
b6a5ee8
Merge branch 'master' into replayTimestamps
lmeier Oct 28, 2024
cc8b817
timeInMs --> locationMs, don't send updatedAtMsDiff if verified
lmeier Nov 7, 2024
161cba4
extra bracket
lmeier Nov 7, 2024
8d7bdd6
nit
lmeier Nov 7, 2024
a214c59
Merge branch 'master' into replayTimestamps
KennyHuRadar Nov 7, 2024
2d22f43
not verified or not foreground
lmeier Nov 7, 2024
9b1432c
version bump
KennyHuRadar Nov 7, 2024
a3818cf
don't conditionalize for verified
lmeier Nov 8, 2024
63d548d
Revert "log cleanup"
lmeier Nov 8, 2024
04bae9e
don't conditionalize updatedAtMsDiff at all
lmeier Nov 8, 2024
910d489
make api helper logic more clear
lmeier Nov 8, 2024
ad35612
Merge branch 'master' into replayTimestamps
KennyHuRadar Nov 11, 2024
1cd42d9
Reformat to more closely match android impl
lmeier Dec 5, 2024
b138b15
Make nil check explict
lmeier Dec 5, 2024
1f5c6d0
put behind a feature setting
lmeier Dec 6, 2024
1c7ca58
Typo
lmeier Dec 6, 2024
d3c24f7
Merge branch 'master' into replayTimestamps
KennyHuRadar Dec 9, 2024
f06ebe5
bump version
KennyHuRadar Dec 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RadarSDK.podspec
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = 'RadarSDK'
s.version = '3.18.4'
s.version = '3.18.4-beta.2'
s.summary = 'iOS SDK for Radar, the leading geofencing and location tracking platform'
s.homepage = 'https://radar.com'
s.author = { 'Radar Labs, Inc.' => '[email protected]' }
Expand Down
13 changes: 6 additions & 7 deletions RadarSDK/RadarAPIClient.m
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,13 @@ - (void)trackWithLocation:(CLLocation *_Nonnull)location
params[@"floorLevel"] = @(location.floor.level);
}
long nowMs = (long)([NSDate date].timeIntervalSince1970 * 1000);
if (!foreground) {
long timeInMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);
params[@"updatedAtMsDiff"] = @(nowMs - timeInMs);
long locationMs = (long)(location.timestamp.timeIntervalSince1970 * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@lmeier lmeier Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

• 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

RadarTrackingOptions *options = [Radar getTrackingOptions];
// if not verified
if (!verified) {
params[@"updatedAtMsDiff"] = @(nowMs - locationMs);
}
params[@"locationMs"] = @(locationMs);
params[@"foreground"] = @(foreground);
params[@"stopped"] = @(stopped);
params[@"replayed"] = @(replayed);
Expand Down Expand Up @@ -299,7 +302,6 @@ - (void)trackWithLocation:(CLLocation *_Nonnull)location
params[@"tripOptions"] = tripParams;
}

RadarTrackingOptions *options = [Radar getTrackingOptions];
if (options.syncGeofences) {
params[@"nearbyGeofences"] = @(YES);
}
Expand Down Expand Up @@ -415,9 +417,6 @@ - (void)trackWithLocation:(CLLocation *_Nonnull)location
// create a copy of params that we can use to write to the buffer in case of request failure
NSMutableDictionary *bufferParams = [params mutableCopy];
bufferParams[@"replayed"] = @(YES);
bufferParams[@"updatedAtMs"] = @(nowMs);
// remove the updatedAtMsDiff key because for replays we want to rely on the updatedAtMs key for the time instead
[bufferParams removeObjectForKey:@"updatedAtMsDiff"];

[[RadarReplayBuffer sharedInstance] writeNewReplayToBuffer:bufferParams];
} else if (options.replay == RadarTrackingOptionsReplayStops && stopped &&
Expand Down
31 changes: 30 additions & 1 deletion RadarSDK/RadarAPIHelper.m
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,38 @@ - (void)requestWithMethod:(NSString *)method
}

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

@tjulien tjulien Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout, fixed: 1cd42d9

[req setHTTPBody:[NSJSONSerialization dataWithJSONObject:params options:0 error:NULL]];
} else {
NSMutableDictionary *requestParams = [params mutableCopy];
NSNumber *locationMs = params[@"locationMs"];
long nowMs = (long)([NSDate date].timeIntervalSince1970 * 1000);

if (locationMs) {
long updatedAtMsDiff = nowMs - [locationMs longValue];
requestParams[@"updatedAtMsDiff"] = @(updatedAtMsDiff);
}

NSArray *replays = params[@"replays"];
if (replays) {
NSMutableArray *updatedReplays = [NSMutableArray arrayWithCapacity:replays.count];
for (NSDictionary *replay in replays) {
NSMutableDictionary *updatedReplay = [replay mutableCopy];
NSNumber *replayLocationMs = replay[@"locationMs"];
if (replayLocationMs) {
long replayUpdatedAtMsDiff = nowMs - [replayLocationMs longValue];
updatedReplay[@"updatedAtMsDiff"] = @(replayUpdatedAtMsDiff);
}
[updatedReplays addObject:updatedReplay];
}
requestParams[@"replays"] = updatedReplays;
}

[req setHTTPBody:[NSJSONSerialization dataWithJSONObject:requestParams options:0 error:NULL]];
}
}


NSURLSessionConfiguration *configuration;
if (extendedTimeout) {
configuration = [NSURLSessionConfiguration defaultSessionConfiguration];
Expand Down
4 changes: 4 additions & 0 deletions RadarSDK/RadarLocationManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,10 @@ - (void)locationManager:(CLLocationManager *)manager didVisit:(CLVisit *)visit {
return;
}

[[RadarLogger sharedInstance] logWithLevel:RadarLogLevelDebug
message:[NSString stringWithFormat:@"Visit detected | arrival = %@; departure = %@; horizontalAccuracy = %f; visit.coordinate = (%f, %f); manager.location = %@",
visit.arrivalDate, visit.departureDate, visit.horizontalAccuracy, visit.coordinate.latitude, visit.coordinate.longitude, manager.location]];

BOOL tracking = [RadarSettings tracking];
if (!tracking) {
[[RadarLogger sharedInstance] logWithLevel:RadarLogLevelDebug message:@"Ignoring visit: not tracking"];
Expand Down
1 change: 0 additions & 1 deletion RadarSDK/RadarNotificationHelper.m
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ + (void)removePendingNotificationsWithCompletionHandler:(void (^)(void))completi
NSMutableArray *identifiers = [NSMutableArray new];
for (UNNotificationRequest *request in requests) {
if ([request.identifier hasPrefix:kSyncGeofenceIdentifierPrefix]) {
[[RadarLogger sharedInstance] logWithLevel:RadarLogLevelDebug message:[NSString stringWithFormat:@"Found pending notification to remove | identifier = %@", request.identifier]];
[identifiers addObject:request.identifier];
}
}
Expand Down
2 changes: 1 addition & 1 deletion RadarSDK/RadarUtils.m
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ + (NSNumber *)timeZoneOffset {
}

+ (NSString *)sdkVersion {
return @"3.18.4";
return @"3.18.4-beta.2";
}

+ (NSString *)deviceId {
Expand Down
2 changes: 1 addition & 1 deletion RadarSDKMotion.podspec
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = 'RadarSDKMotion'
s.version = '3.18.3'
s.version = '3.18.4-beta.2'
s.summary = 'Motion detection plugin for RadarSDK, the leading geofencing and location tracking platform'
s.homepage = 'https://radar.com'
s.author = { 'Radar Labs, Inc.' => '[email protected]' }
Expand Down
Loading