fix: improve push delivery for Android 14 and above by Shahroz16 · Pull Request #493 · customerio/customerio-android

Shahroz16 · 2025-02-10T16:06:37Z

closes: https://linear.app/customerio/issue/MBL-856/report-push-notification-metric-issues-instantly-dont-add-to-queue

changes:

Save configuration and share via core
Add HTTP layer in push utilizing saved config
PushDeliveryTracker, utilizing HTTP client
Move classes/dependencies from Datapipeline to Core for other modules to be used

github-actions · 2025-02-10T16:06:52Z

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 · 2025-02-10T16:09:13Z

Codecov Report

Attention: Patch coverage is 45.71429% with 57 lines in your changes missing coverage. Please review.

Project coverage is 52.01%. Comparing base (8b30804) to head (fdbe919).
Report is 46 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/io/customer/messagingpush/network/HTTPClient.kt	22.41%	43 Missing and 2 partials ⚠️
...o/customer/sdk/data/store/GlobalPreferenceStore.kt	0.00%	10 Missing ⚠️
...main/kotlin/io/customer/sdk/data/model/Settings.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main     #493       +/-   ##
=============================================
+ Coverage     41.98%   52.01%   +10.03%     
- Complexity      259      318       +59     
=============================================
  Files            99      105        +6     
  Lines          2320     2855      +535     
  Branches        344      380       +36     
=============================================
+ Hits            974     1485      +511     
- Misses         1247     1257       +10     
- Partials         99      113       +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-02-10T16:10:56Z

Build available to test
Version: mbl-856-push-delievery-SNAPSHOT
Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

Shahroz16 · 2025-02-10T16:11:03Z

gradlew

ahhh need to remove these changes

github-actions · 2025-02-10T16:11:25Z

java_layout: mbl-856-push-delievery (1739203656)

github-actions · 2025-02-10T16:12:13Z

kotlin_compose: mbl-856-push-delievery (1739203657)

This reverts commit 280de6b.

github-actions · 2025-02-10T18:11:53Z

java_layout: mbl-856-push-delievery (1739210965)

github-actions · 2025-02-10T18:12:57Z

kotlin_compose: mbl-856-push-delievery (1739210968)

github-actions · 2025-02-11T09:00:30Z

java_layout: mbl-856-push-delievery (1739264265)

github-actions · 2025-02-11T09:00:57Z

kotlin_compose: mbl-856-push-delievery (1739264259)

mahmoud-elmorabea · 2025-02-12T06:37:54Z

core/src/main/kotlin/io/customer/sdk/data/store/GlobalPreferenceStore.kt

        putString(KEY_DEVICE_TOKEN, token)
    }

+    override fun saveSettings(value: String) = prefs.edit {


Does it make sense for this method to accept Settings so that the encoding/decoding is encapsulated within this store?

Think it does make sense, i wanted to avoid strong coupling of serialization with storage, but since string format could it make it generic and prone to break, it probably wiser to make this method stricter.

mahmoud-elmorabea · 2025-02-12T06:38:54Z

core/src/main/kotlin/io/customer/sdk/util/EventNames.kt

 * reflected on Journeys.
 */
-internal object EventNames {
+object EventNames {


Curious as to why we moved this to core? It seems like those events are very related to Data Pipelines

Because multiple modules are using it

mahmoud-elmorabea · 2025-02-12T06:40:00Z

datapipelines/src/main/kotlin/io/customer/sdk/CustomerIO.kt

        migrateTrackingEvents()
+
+        // save settings to storage
+        analytics.configuration.let { config ->


If this configuration object is available through analytics, what's the main reason for storing it locally? Is it for cases when the SDK hasn't been initialized yet?

analytics is a dependency/part of data pipeline module, while push module doesn't rely on it. It just needs writeKey and endpoint that it can use to make the HTTP request.

mahmoud-elmorabea · 2025-02-12T06:40:35Z

messagingpush/src/main/java/io/customer/messagingpush/PushDeliveryTracker.kt

+    fun trackMetric(token: String, event: String, deliveryId: String, onComplete: ((Result<Unit>) -> Unit?)? = null)
+}
+
+class PushDeliveryTrackerImpl : PushDeliveryTracker {


Do any of these types need to be public?

I thought about it, made them public when I was adding test for it. But I am going to revisit it.

mahmoud-elmorabea · 2025-02-12T06:43:05Z

messagingpush/src/main/java/io/customer/messagingpush/PushDeliveryTracker.kt

+        token: String,
+        event: String,
+        deliveryId: String,
+        onComplete: ((Result<Unit>) -> Unit?)?


Do we need a callback here? I'm not sure if the caller can do much in case of failure?

Added it, in case we wanted to do some logging based on it or perform any operation based on its result.

mahmoud-elmorabea · 2025-02-12T06:48:17Z

messagingpush/src/main/java/io/customer/messagingpush/processor/PushMessageProcessorImpl.kt

+            )
+
+            // Track delivered metrics via event bus
            eventBus.publish(


Does this mean that this event will be tracked twice if the app is open or not restricted by OS to run in the background?

Yes, thats true. But i think its safer this way. Duplicate delivery is doesn't reflect on journeys etc but it safe guard from the fact that HTTP request can fail and we shouldn't just rely on it. The other way it becomes of offline queue so even if it fails, we have it in queue to be retried.

So Journey's will handle this as a single delivery if we report it twice? My understanding is that this will count twice against the same delivery ID

My understanding is that this will count twice against the same delivery ID

I am not 100% sure I get this? there were will be 2 requests in cdp and two might to journeys, but where is the count ?

The points make sense. However, I think we can still skip sending metrics to EventBus since the implementation aligns with iOS and has lower chance of failure due to network issues as push can only be delivered when the network is available. Or maybe we could push to EventBus only in case of failure? 🤔

That said, I do agree this ensures delivered metrics are always tracked even if the request fails. Since CDP already skips duplicate requests, that shouldn't be a concern. So I'm okay with either approach we decide to continue with.

mahmoud-elmorabea · 2025-02-12T06:50:51Z

datapipelines/src/main/kotlin/io/customer/sdk/CustomerIO.kt

        migrateTrackingEvents()
+
+        // save settings to storage
+        analytics.configuration.let { config ->


Should we add a test for this?

Let me try adding test for these

Would be nice to add these tests for the EU region as well

mahmoud-elmorabea · 2025-02-12T06:51:33Z

messagingpush/api/messagingpush.api

+	public fun <init> ()V
+	public fun trackMetric (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)V
+}
+


Do we need those to be exposed publicly?

I thought about it, made them public when I was adding test for it. But I am going to revisit it.

mahmoud-elmorabea · 2025-02-12T06:53:38Z

datapipelines/api/datapipelines.api

 	public fun getProfileAttributes ()Ljava/util/Map;
 	public fun getRegisteredDeviceToken ()Ljava/lang/String;
 	public fun getUserId ()Ljava/lang/String;
+	public final fun getWriteKey ()Ljava/lang/String;


I don't see this method actually in DataPipelinesModuleConfig, I might be missing something though

Good catch, it was an unused variable added to CustomerIO class.

github-actions · 2025-02-12T07:35:06Z

java_layout: mbl-856-push-delievery (1739345552)

github-actions · 2025-02-12T07:35:44Z

kotlin_compose: mbl-856-push-delievery (1739345547)

github-actions · 2025-02-12T07:40:14Z

java_layout: mbl-856-push-delievery (1739345854)

github-actions · 2025-02-12T07:40:57Z

kotlin_compose: mbl-856-push-delievery (1739345859)

github-actions · 2025-02-12T08:44:43Z

java_layout: mbl-856-push-delievery (1739349732)

github-actions · 2025-02-12T08:45:28Z

kotlin_compose: mbl-856-push-delievery (1739349735)

github-actions · 2025-02-12T09:55:52Z

java_layout: mbl-856-push-delievery (1739354002)

github-actions · 2025-02-12T09:56:47Z

kotlin_compose: mbl-856-push-delievery (1739354004)

mrehan27 · 2025-02-12T10:34:16Z

messagingpush/src/main/java/io/customer/messagingpush/network/HTTPClient.kt

+internal class HttpClientImpl : HttpClient {
+
+    private val connectTimeoutMs = 15_000
+    private val readTimeoutMs = 20_000


Do we really need such high timeout values here? 🤔

nope not really, got these from iOS HTTP client and then kept it for consistency. We can lower it if needed.

mahmoud-elmorabea

Looks good! 💯 Thanks for addressing the comments 😇

github-actions · 2025-02-18T10:57:49Z

java_layout: mbl-856-push-delievery (1739876117)

github-actions · 2025-02-18T10:58:48Z

kotlin_compose: mbl-856-push-delievery (1739876120)

## [4.5.3](4.5.2...4.5.3) (2025-02-18) ### Bug Fixes * improve push delivery tracking for Android 14 and above ([#493](#493)) ([ea5c7a7](ea5c7a7))

Shahroz16 requested a review from a team as a code owner February 10, 2025 16:06

Shahroz16 self-assigned this Feb 10, 2025

Shahroz16 commented Feb 10, 2025

View reviewed changes

gradlew

Copy link

Contributor Author

Shahroz16 Feb 10, 2025

Choose a reason for hiding this comment

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

ahhh need to remove these changes

Revert "implementation added"

8623a75

This reverts commit 280de6b.

Shahroz16 force-pushed the mbl-856-push-delievery branch from 280de6b to 8623a75 Compare February 10, 2025 18:08

access modifiers update

44dafd2

mahmoud-elmorabea requested changes Feb 12, 2025

View reviewed changes

mahmoud-elmorabea requested a review from a team February 12, 2025 06:54

removed unused variable

bc258b1

encapsulate encoding

21364bc

added test

7b4ecc7

added more tests

c54d6bf

Shahroz16 requested a review from mahmoud-elmorabea February 12, 2025 09:56

mrehan27 reviewed Feb 12, 2025

View reviewed changes

Shahroz16 requested a review from mrehan27 February 12, 2025 11:14

mahmoud-elmorabea approved these changes Feb 13, 2025

View reviewed changes

mrehan27 approved these changes Feb 14, 2025

View reviewed changes

lowered the timeouts

fdbe919

Shahroz16 merged commit ea5c7a7 into main Feb 18, 2025
34 of 35 checks passed

Shahroz16 deleted the mbl-856-push-delievery branch February 18, 2025 11:07

github-actions bot pushed a commit that referenced this pull request Feb 18, 2025

chore: prepare for 4.5.3

ce526c5

## [4.5.3](4.5.2...4.5.3) (2025-02-18) ### Bug Fixes * improve push delivery tracking for Android 14 and above ([#493](#493)) ([ea5c7a7](ea5c7a7))

Conversation

Shahroz16 commented Feb 10, 2025

Uh oh!

github-actions bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sample app builds 📱

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 10, 2025

Uh oh!

github-actions bot commented Feb 10, 2025

Uh oh!

github-actions bot commented Feb 10, 2025

Uh oh!

github-actions bot commented Feb 10, 2025

Uh oh!

github-actions bot commented Feb 11, 2025

Uh oh!

github-actions bot commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 12, 2025

Uh oh!

github-actions bot commented Feb 12, 2025

Uh oh!

github-actions bot commented Feb 12, 2025

Uh oh!

github-actions bot commented Feb 12, 2025

Uh oh!

github-actions bot commented Feb 12, 2025

github-actions bot commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading