chore: make location module self-contained with generic core interfaces#664
chore: make location module self-contained with generic core interfaces#664Shahroz16 merged 5 commits intofeat/real-time-locationfrom
Conversation
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. |
|
|
|
Build available to test |
📏 SDK Binary Size Comparison Report
|
| */ | ||
| @InternalCustomerIOApi | ||
| interface DataPipeline { | ||
| val userId: String? |
There was a problem hiding this comment.
Are we exposing userId to check if the user is identified? Should we expose isIdentified instead?
There was a problem hiding this comment.
userId is exposed if we wanted to also track user being changed, but i can remove it right now and rely on UserChangedEvent
There was a problem hiding this comment.
Good point, that covers two identify call different ids with no clearIdentify, I will port this to iOS
| */ | ||
| @InternalCustomerIOApi | ||
| interface ProfileEnrichmentProvider { | ||
| fun getProfileEnrichmentAttributes(): Map<String, Any>? |
There was a problem hiding this comment.
Let's make this non-null. If there is nothing to enrich, we can return empty map.
| class ProfileEnrichmentRegistry { | ||
| private val providers = mutableListOf<ProfileEnrichmentProvider>() | ||
|
|
||
| @Synchronized |
There was a problem hiding this comment.
Are we sure that @Synchronized is the right tool here? Do plugins use threads or coroutines?
There was a problem hiding this comment.
Plugins run synchronously on the caller's thread, EventPlugin.identify() is blocking and executes on whatever thread called analytics.identify(). No coroutines involved at the plugin level.
@Synchronized is the right tool here because register(), getAll(), and clear() are fast, non-suspending operations (list add/copy/clear). A coroutine Mutex would require these to be suspend functions, which doesn't fit the EventPlugin interface.
There was a problem hiding this comment.
The main idea is that the registry is called from other modules to register themselves and this could expand to more modules in the future. So we can't be sure that this invariant remains true
| // Register LocationPlugin as LocationCache so the location module can update it | ||
| SDKComponent.registerDependency<LocationCache> { locationPlugin } | ||
| // Register this instance as DataPipeline so modules can send track events directly | ||
| SDKComponent.registerDependency<DataPipeline> { this } |
There was a problem hiding this comment.
Is this the best way to make this registration? This has to be done before the location module is inialized, correct?
There was a problem hiding this comment.
Yes, it must happen before. The init order is guaranteed by CustomerIO.initialize() : createInstance() runs the constructor (which triggers init → registers DataPipeline), then module.initialize() is called in the loop afterwards. So SDKComponent.getOrNull<DataPipeline>() in ModuleLocation.initialize() always finds the registered instance.
| * This guards against race conditions during app startup or incorrect call order. | ||
| */ | ||
| val locationServices: LocationServices | ||
| get() = _locationServices ?: UninitializedLocationServices(SDKComponent.logger) |
There was a problem hiding this comment.
Is this access thread safe?
There was a problem hiding this comment.
It's safe in practice, _locationServices is written once during initialize() (main thread, during SDK startup) before any host app code can access the getter. But it's not formally guaranteed across threads. Will add @Volatile for extra hardening.
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
location/src/main/kotlin/io/customer/location/LocationTracker.kt
Outdated
Show resolved
Hide resolved
datapipelines/src/main/kotlin/io/customer/datapipelines/plugins/IdentifyContextPlugin.kt
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/real-time-location #664 +/- ##
==========================================================
Coverage ? 67.41%
Complexity ? 764
==========================================================
Files ? 146
Lines ? 4382
Branches ? 591
==========================================================
Hits ? 2954
Misses ? 1201
Partials ? 227 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
|
|
|
|
||
| if (isChangingIdentifiedProfile) { | ||
| logger.info("changing profile from id $currentlyIdentifiedProfile to $userId") | ||
| locationSyncFilter.clearSyncedData() |
There was a problem hiding this comment.
Sync filter not cleared on direct profile switch
High Severity
When switching directly from user A to user B (without clearIdentify), the old code called locationSyncFilter.clearSyncedData() inside the isChangingIdentifiedProfile block. That line was removed, and LocationTracker.resetContext() only runs during analytics.reset() (called in clearIdentifyImpl), not during direct profile switches. This means user B's location track events can be suppressed by user A's 24h/1km sync filter window. The PR description mentions "Profile switch detection via lastKnownUserId with sync filter reset" but lastKnownUserId does not exist in the implementation.
Additional Locations (1)
There was a problem hiding this comment.
The sync filter not clearing on direct profile switch is intentional.
When user B is identified, the IdentifyContextPlugin calls LocationTracker.getIdentifyContext() and attaches the current coordinates to request. This path is unfiltered, user B's profile receives the device's location immediately as part of the identify call itself.
The sync filter only gates the supplementary "Location Update" track event. If it suppresses that event because the same coordinates were recently sent, that's correct behavior — the backend already has the location from the identify event. No data is lost.
mahmoud-elmorabea
left a comment
mahmoud-elmorabea
left a comment
There was a problem hiding this comment.
Looks good! 💯
I do think we should go back to the approach where we exposed userId to detect a change. But do feel free to address this in this or later PR if we decide to go that way.
2 participants
Summary
interfaces (DataPipeline, ProfileEnrichmentProvider) that any module can use
What changed
Core module — new generic interfaces:
Datapipelines module — zero location awareness:
Location module — fully self-contained:
Note
Medium Risk
Changes the location-identify/track flow and moves filtering/state-reset responsibilities across modules; regressions could drop or duplicate location enrichment/track events despite being covered by updated unit tests.
Overview
Decouples location from
datapipelinesby removing location-specificEventtypes (Event.LocationData,Event.TrackLocationEvent) and theLocationCachecontract, and introducing core internal interfaces for cross-module interaction:DataPipeline(directtrack+isUserIdentified) andIdentifyHook/IdentifyHookRegistry(identify context enrichment + synchronous reset hook).CustomerIOnow implements and registersDataPipeline, replacesLocationPluginwith a genericIdentifyContextPluginthat enriches identify events from all registered hooks, and removes the EventBus-driven location track subscriber and sync-filter reset calls. The location module takes ownership of location business logic:LocationTrackerimplementsIdentifyHook, persists/caches coordinates, applies the 24h/1kmLocationSyncFilter(moved into the location module), and sendsLOCATION_UPDATEviaDataPipeline.trackinstead of publishing through EventBus; tests are updated accordingly and location/datapipelines builds opt into@InternalCustomerIOApi.Written by Cursor Bugbot for commit 5c2a122. This will update automatically on new commits. Configure here.