New PushNotificationDataSource #688

nelsitoPuglisi · 2024-04-10T19:57:27Z

Goal

Log push notifications as span events in the session span instead of breadcrumbs

Testing

Integration and manual tests

nelsitoPuglisi · 2024-04-10T19:57:41Z

New ViewDataSource #717
New PushNotificationDataSource #688 👈
Use new webView data source in prod #680
DataSourceModule providers #712
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @nelsitoPuglisi and the rest of your teammates on Graphite

fractalwrench

I had one query about some assertions in the test cases, otherwise this looks good

fractalwrench · 2024-04-15T13:40:32Z

...roid-sdk/src/integrationTest/kotlin/io/embrace/android/embracesdk/testcases/PublicApiTest.kt

+import io.embrace.android.embracesdk.arch.schema.EmbType
+import io.embrace.android.embracesdk.findEventOfType
+import io.embrace.android.embracesdk.findSessionSpan
+import io.embrace.android.embracesdk.getSentSessionMessages
+import io.embrace.android.embracesdk.hasEventOfType
 import io.embrace.android.embracesdk.internal.ApkToolsConfig
 import io.embrace.android.embracesdk.internal.IdGeneratorTest.Companion.validPattern
+import io.embrace.android.embracesdk.internal.clock.millisToNanos
 import io.embrace.android.embracesdk.recordSession
+import org.junit.Assert


nit: I'd assume these are all unused imports?

fractalwrench · 2024-04-15T13:41:41Z

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/payload/Breadcrumbs.kt

- * List of captured push notifications
- */
- @Json(name = "pn")
- val pushNotifications: List<PushNotificationBreadcrumb>? = null,


Can the LegacyPushNotificationDataSource be deleted if we're getting rid of payload fields in the same changeset?

fractalwrench · 2024-04-15T13:44:03Z

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/injection/DataSourceModule.kt

@@ -69,6 +71,19 @@ internal class DataSourceModuleImpl(
 )
 }

+ override val pushNotificationDataSource: DataSourceState<PushNotificationDataSource> by dataSourceState {
+ DataSourceState(


How do users enable this feature in their apps? I'm a little surprised that there isn't any parameter supplied for configGate although if that's existing behavior that's ok

This is used by manually calling our public api, and automacally if the feature is enabled with this guide: https://embrace.io/docs/android/features/push-notifications/

fractalwrench · 2024-04-15T13:44:55Z

...d-sdk/src/main/java/io/embrace/android/embracesdk/capture/crumbs/EmbraceBreadcrumbService.kt

- type
- )
+ ) {
+ dataSourceModuleProvider()?.pushNotificationDataSource?.dataSource?.apply {


nit: you could remove the apply here as there's only one statement inside

fractalwrench · 2024-04-15T13:46:52Z

...k/src/integrationTest/kotlin/io/embrace/android/embracesdk/testcases/PushNotificationTest.kt

+ "notification.title" to "",
+ "notification.type" to type,
+ "notification.body" to "",
+ "notification.id" to "id",
+ "notification.from" to "",


These empty string assertions don't look right - if "title" and "body" are being supplied as parameters in the test invocation then I'd expect them to show up in the attrs map.

They shouldn't be sent because the default setting for PII capture is FALSE. A specific test captures that data when the setting is overridden to true.

Got it. This is fine then

...sdk/src/main/java/io/embrace/android/embracesdk/capture/crumbs/PushNotificationDataSource.kt

codecov · 2024-04-15T21:24:52Z

Codecov Report

Attention: Patch coverage is 89.23077% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 80.21%. Comparing base (5438954) to head (3c731ff).
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #688      +/-   ##
==========================================
+ Coverage   79.83%   80.21%   +0.38%     
==========================================
  Files         429      429              
  Lines       11330    11374      +44     
  Branches     1720     1725       +5     
==========================================
+ Hits         9045     9124      +79     
+ Misses       1558     1532      -26     
+ Partials      727      718       -9

Files	Coverage Δ
.../embrace/android/embracesdk/arch/schema/EmbType.kt	`56.25% <100.00%> (+1.41%)`	⬆️
...brace/android/embracesdk/arch/schema/SchemaType.kt	`70.23% <100.00%> (+4.96%)`	⬆️
...e/android/embracesdk/injection/DataSourceModule.kt	`100.00% <100.00%> (ø)`
.../embrace/android/embracesdk/payload/Breadcrumbs.kt	`100.00% <100.00%> (ø)`
...racesdk/capture/crumbs/EmbraceBreadcrumbService.kt	`80.00% <87.50%> (-2.98%)`	⬇️
...d/embracesdk/config/behavior/BreadcrumbBehavior.kt	`45.00% <50.00%> (-2.06%)`	⬇️
...cesdk/capture/crumbs/PushNotificationDataSource.kt	`87.09% <87.09%> (ø)`

... and 35 files with indirect coverage changes

fractalwrench

LGTM

fractalwrench · 2024-04-16T09:10:54Z

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/injection/DataSourceModule.kt

+ override val pushNotificationDataSource: DataSourceState<PushNotificationDataSource> by dataSourceState {
+ DataSourceState(
+ factory = {
+ System.out.println("DataSourceModuleImpl ${essentialServiceModule.configService}")


Would suggest removing this println statement

nelsitoPuglisi changed the title ~~PushNotificationDataSource~~ New PushNotificationDataSource Apr 10, 2024

nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch 4 times, most recently from 2a9464e to eace200 Compare April 13, 2024 19:10

nelsitoPuglisi changed the base branch from master to nelson/web-view-data-source-prod April 13, 2024 19:10

This was referenced Apr 13, 2024

Use new webView data source in prod #680

Merged

DataSourceModule providers #712

Merged

New ViewDataSource #717

Merged

nelsitoPuglisi force-pushed the nelson/web-view-data-source-prod branch from be4eb50 to 5edc7f6 Compare April 15, 2024 12:38

nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from 4117a92 to 504be70 Compare April 15, 2024 12:38

nelsitoPuglisi marked this pull request as ready for review April 15, 2024 12:41

nelsitoPuglisi requested a review from a team as a code owner April 15, 2024 12:41

nelsitoPuglisi force-pushed the nelson/web-view-data-source-prod branch from 5edc7f6 to f4b7b57 Compare April 15, 2024 13:04

nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from 504be70 to db6a928 Compare April 15, 2024 13:04

$fractalwrench$

fractalwrench reviewed Apr 15, 2024

View reviewed changes

nelsitoPuglisi force-pushed the nelson/web-view-data-source-prod branch from f4b7b57 to e384707 Compare April 15, 2024 17:23

nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from db6a928 to 251548a Compare April 15, 2024 17:23

Base automatically changed from nelson/web-view-data-source-prod to master April 15, 2024 21:20

nelsitoPuglisi added 8 commits April 15, 2024 18:24

PushNotificationDataSource

e7d089e

add missing files

edc8074

fix tests

6d30413

fix tests

4aab37c

fix funciontal test

40e7550

fix tests

5c46996

fix tests

f3be43f

fix tests

83a4109

nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from aee3ba3 to 83a4109 Compare April 15, 2024 21:24

remove legacy data source

6768031

$fractalwrench$

fractalwrench approved these changes Apr 16, 2024

View reviewed changes

remove println

3c731ff

nelsitoPuglisi merged commit b09b5ea into master Apr 16, 2024
6 checks passed

nelsitoPuglisi deleted the nelson/push-notification-data-source branch April 16, 2024 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New PushNotificationDataSource #688

New PushNotificationDataSource #688

nelsitoPuglisi commented Apr 10, 2024 •

edited

Loading

nelsitoPuglisi commented Apr 10, 2024 •

edited

Loading

$@fractalwrench$ fractalwrench left a comment

$@fractalwrench$ fractalwrench Apr 15, 2024

nelsitoPuglisi Apr 15, 2024

$@fractalwrench$ fractalwrench Apr 15, 2024

$@fractalwrench$ fractalwrench Apr 15, 2024

nelsitoPuglisi Apr 15, 2024

$@fractalwrench$ fractalwrench Apr 15, 2024

$@fractalwrench$ fractalwrench Apr 15, 2024

nelsitoPuglisi Apr 15, 2024

$@fractalwrench$ fractalwrench Apr 16, 2024

codecov bot commented Apr 15, 2024 •

edited

Loading

$@fractalwrench$ fractalwrench left a comment

$@fractalwrench$ fractalwrench Apr 16, 2024

New PushNotificationDataSource #688

New PushNotificationDataSource #688

Conversation

nelsitoPuglisi commented Apr 10, 2024 • edited Loading

Goal

Testing

nelsitoPuglisi commented Apr 10, 2024 • edited Loading

fractalwrench left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 15, 2024 • edited Loading

Codecov Report

fractalwrench left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsitoPuglisi commented Apr 10, 2024 •

edited

Loading

nelsitoPuglisi commented Apr 10, 2024 •

edited

Loading

$@fractalwrench$ fractalwrench left a comment

codecov bot commented Apr 15, 2024 •

edited

Loading

$@fractalwrench$ fractalwrench left a comment