chore: LocationTracker coordinator with rate-limited location sends#661
Conversation
- Enrich identify events with last known lat/lng in context so Customer.io knows user location at identification time. - Block consumer-sent "Location Update" track events to prevent backend flooding. Only SDK-internal location events pass through using an internal marker that is stripped before delivery. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/real-time-location #661 +/- ##
==========================================================
Coverage ? 67.02%
Complexity ? 773
==========================================================
Files ? 146
Lines ? 4443
Branches ? 598
==========================================================
Hits ? 2978
Misses ? 1231
Partials ? 234 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "lat" to location.latitude, | ||
| "lng" to location.longitude | ||
| "latitude" to location.latitude, | ||
| "longitude" to location.longitude |
There was a problem hiding this comment.
Track event property keys renamed, breaking server contract
High Severity
The "Location Update" track event property keys changed from "lat" and "lng" to "latitude" and "longitude". This silently changes the payload schema sent to Customer.io servers. If the backend or any existing campaigns/journeys depend on the original "lat"/"lng" property names, location data processing would break for all customers upgrading to this SDK version. The PR description does not mention this as an intentional change.
|
|
Build available to test |
📏 SDK Binary Size Comparison Report
|
mahmoud-elmorabea
left a comment
mahmoud-elmorabea
left a comment
There was a problem hiding this comment.
I would have preferred that that logic lived in the location module, WDYT?
| * (e.g. send a "Location Update" track for the newly-identified user). | ||
| */ | ||
| @Volatile | ||
| internal var identifySentWithoutLocation: Boolean = false |
There was a problem hiding this comment.
Is this an in-memory only value? What happens when there an identify in a session then in new launch, the location is acquired
There was a problem hiding this comment.
Good point, the debt flag is now persisted to LocationPreferenceStore via savePendingLocationDebt()/getPendingLocationDebt(). The in-memory field has been removed entirely the store is the single source of truth. The flag is restored on cold start in restorePersistedLocation() and cleared when location arrives or when clearIdentify() is called.
| fun restorePersistedLocation() { | ||
| val lat = locationPreferenceStore.getLatitude() ?: return | ||
| val lng = locationPreferenceStore.getLongitude() ?: return | ||
| locationPlugin.lastLocation = Event.LocationData(latitude = lat, longitude = lng) |
There was a problem hiding this comment.
Is locationPlugin.lastLocation access thread safe?
There was a problem hiding this comment.
Yes. lastLocation is declared @Volatile. The operations are simple reference reads (lastLocation ?: return payload) and writes (lastLocation = location) both atomic on the JVM. There are no compound read-modify-write operations. @Volatile guarantees cross-thread visibility, which is all that's needed here.
| companion object { | ||
| private const val KEY_LATITUDE = "latitude" | ||
| private const val KEY_LONGITUDE = "longitude" | ||
| private const val KEY_LAST_SENT_TIMESTAMP = "last_sent_timestamp" |
There was a problem hiding this comment.
Should we prefix these to avoid any future collisions
There was a problem hiding this comment.
they are under separate directory, io.customer.sdk.location.${context.packageName} so doubt collision, but sure can do add prefix
| analytics.add(ApplicationLifecyclePlugin()) | ||
|
|
||
| // Restore persisted location so identify events have context immediately | ||
| locationTracker.restorePersistedLocation() |
There was a problem hiding this comment.
Does this happen on main thread?
There was a problem hiding this comment.
yes on called thread, might change in next PR where i am updating logic to cater guardrails
|
|
||
| if (!locationTracker.hasLocationContext()) { | ||
| locationTracker.onIdentifySentWithoutLocation() | ||
| } |
There was a problem hiding this comment.
I think we should ignore this with an error, there are APIs that can be used to do that
There was a problem hiding this comment.
Could you clarify which APIs you're referring to? Currently the identify proceeds normally and the debt is recorded silently. when location arrives, the SDK sends a compensating Location Update track event. The identify call itself is never blocked or errored, since the location module is optional and apps that don't use location would have every identify() fail.?
|
|
|
|
| * (e.g. send a "Location Update" track for the newly-identified user). | ||
| */ | ||
| @Volatile | ||
| internal var identifySentWithoutLocation: Boolean = false |
There was a problem hiding this comment.
Follow-up debt flag lost on app restart
Medium Severity
identifySentWithoutLocation is an in-memory @Volatile boolean that is never persisted. If identify() is called without location context and the app is killed before any location arrives, this flag is lost on cold start. When the location eventually arrives in the new session, no compensating "Location Update" track is sent for the previous identify, silently dropping the follow-up obligation.
There was a problem hiding this comment.
its removed in the other PR
datapipelines/src/main/kotlin/io/customer/datapipelines/store/PreferenceCrypto.kt
Show resolved
Hide resolved
|
|
|
|
||
| if (!locationTracker.hasLocationContext()) { | ||
| locationTracker.onIdentifySentWithoutLocation() | ||
| } |
There was a problem hiding this comment.
Race between identify and location event handling
Low Severity
identifyImpl (running inside synchronized(this)) performs a compound check-then-act on hasLocationContext() and identifySentWithoutLocation, while onLocationReceived (running on the event bus coroutine dispatcher, outside any lock) concurrently writes lastLocation and reads the same flag. The @Volatile annotation ensures visibility of individual reads/writes but does not make the compound operations atomic across both methods. This can leave identifySentWithoutLocation set to true even when the identify was enriched with location, causing a spurious compensating track on the next location event.
Additional Locations (2)
| } | ||
|
|
||
| logger.info("identify profile with identifier $userId and traits $traits") | ||
|
|
There was a problem hiding this comment.
Location debt not cleared when switching identified profiles
Low Severity
When isChangingIdentifiedProfile is true, identifyImpl does not call locationTracker.onUserReset(). The onUserReset KDoc itself states "the debt belongs to the identified user, and once they're gone the follow-up location track is no longer owed." If user A identified without location (setting the debt flag), and then user B identifies directly (without clearIdentify) with location available, the stale debt from user A persists. The next location event then sends a spurious compensating "Location Update" track attributed to user B's profile.
Additional Locations (1)
| val lat = locationPreferenceStore.getLatitude() ?: return null | ||
| val lng = locationPreferenceStore.getLongitude() ?: return null | ||
|
|
||
| if (!isStale()) return null |
There was a problem hiding this comment.
Shouldn't isStale be checked before instead?
| val lat = locationPreferenceStore.getLatitude() ?: return null | |
| val lng = locationPreferenceStore.getLongitude() ?: return null | |
| if (!isStale()) return null | |
| if (!isStale()) return null | |
| val lat = locationPreferenceStore.getLatitude() ?: return null | |
| val lng = locationPreferenceStore.getLongitude() ?: return null |
3 participants
Summary
CustomerIO.ktinto a dedicatedLocationTrackercoordinatoridentify that lacked location context
Behavior
contextclearIdentify()after identify-without-locationsetLastKnownLocation()sends track on first call, suppresses subsequent calls within 24hidentify()without location sets follow-up debt; first location arrival sends track and clears debtclearIdentify()voids follow-up debtcontext.location_latitudeandcontext.location_longitudewhen location is availableNote
Medium Risk
Changes event emission/enrichment behavior and introduces persisted (encrypted) storage plus new rate-limiting logic, which could affect analytics payload compatibility and when location tracks are sent.
Overview
Adds a new
LocationTracker+LocationPluginflow that persists last-known location across restarts and enriches Segmentidentifyevents withcontext.location_latitude/context.location_longitude.Location updates are now rate-limited: incoming
TrackLocationEvents always update cached/persisted coordinates, but only emit the "Location Update" track when an identify previously occurred without location context or when the last send is stale (>=24h); on SDK startup, a stale persisted location is re-sent.Introduces a
LocationPreferenceStore(wired viaSDKComponentExt) with Android Keystore-backed AES-GCM encryption fallback, and adjusts the location track payload keys fromlat/lngtolatitude/longitude.Written by Cursor Bugbot for commit 56d6033. This will update automatically on new commits. Configure here.