automatic breadcrumbs for app, activity and sessions lifecycles and system events #348

marandaneto · 2020-04-08T15:44:17Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

automatic breadcrumbs for app, activity and sessions lifecycles and system events

💡 Motivation and Context

we'd like to collect automatic breadcrumbs
fixes #210

💚 How did you test it?

📝 Checklist

I reviewed submitted code
I added tests to verify changes
All tests passing

🔮 Next steps

mrmitew · 2020-04-16T07:45:16Z

@marandaneto Did you take into account that if android build has been obfuscated (e.g. with ProGuard or DexGuard), activity names in the breadcrumb messages will look like gibberish and won't be retraced?

Also, is there a way to disable the integration once this is merged?

marandaneto · 2020-04-16T08:14:17Z

@marandaneto Did you take into account that if android build has been obfuscated (e.g. with ProGuard or DexGuard), activity names in the breadcrumb messages will look like gibberish and won't be retraced?

Also, is there a way to disable the integration once this is merged?

we are gonna probably extract this module as a new maven package (android aar) and ship it with a proguard rule for that, keeping Activity names.
the other way would be just to add breadcrumbs yourselves with your own activity identifies.

not sure if we are gonna demangle the breadcrumbs as well, I'll check this internally.

mrmitew · 2020-04-16T09:26:24Z

It would be nice if you could demangle the class names in the breadcrumbs as a general feature.

Putting a rule for keeping activity names could be a nasty surprise for some people though. In my opinion it will be better as an opt-in feature, so having it as a separate artifact makes sense indeed.

marandaneto · 2020-04-16T09:32:51Z

It would be nice if you could demangle the class names in the breadcrumbs as a general feature.

Putting a rule for keeping activity names could be a nasty surprise for some people though. In my opinion it will be better as an opt-in feature, so having it as a separate artifact makes sense indeed.

yes, I agree, as I said, it's gonna be a separated maven package, so generally opt-in, I'll update you on that, thanks for the tip.

sentry-android-core/src/main/java/io/sentry/android/core/ActivityBreadcrumbsIntegration.java

codecov-io · 2020-04-20T16:38:25Z

Codecov Report

Merging #348 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #348      +/-   ##
============================================
- Coverage     58.89%   58.84%   -0.05%     
  Complexity      758      758              
============================================
  Files            87       87              
  Lines          3540     3543       +3     
  Branches        341      341              
============================================
  Hits           2085     2085              
- Misses         1302     1305       +3     
  Partials        153      153

Impacted Files	Coverage Δ	Complexity Δ
.../src/main/java/io/sentry/core/protocol/Device.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcec4d1...7811042. Read the comment docs.

sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java

sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java

marandaneto · 2020-04-21T12:01:14Z

...ry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java

+      // TODO: should we log the params? sometimes its necessary eg
+      // ACTION_AIRPLANE_MODE_CHANGED
+      // its a single action with 2 states that differs on extras
+      final Bundle extras = intent.getExtras();
+      final Map<String, String> newExtras = new HashMap<>();
+      if (extras != null && !extras.isEmpty()) {
+        for (String item : extras.keySet()) {
+          try {
+            newExtras.put(item, extras.get(item).toString());
+          } catch (Exception ignored) {
+          }
+        }
+      }
+      breadcrumb.setData("extras", newExtras);


this is trick, each broadcast has its own extras, we could send them all as I do now, but there are a lot of garbage too.
the other way would be to only send the useful extras for each broadcast.
the thing is, docs for extras are not great and it'd take some time to find the right keys.

Let's leave it as you did and see the feedback. If we only add stuff manually we could be missing things and there's the extra work/maintenance

...y-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java

marandaneto · 2020-04-22T09:44:32Z

It would be nice if you could demangle the class names in the breadcrumbs as a general feature.

Putting a rule for keeping activity names could be a nasty surprise for some people though. In my opinion it will be better as an opt-in feature, so having it as a separate artifact makes sense indeed.

so it seems that proguard-android-optimize.txt already does it by default, Activity names are never minified as far as I can see

bruno-garcia

I left suggestions and some nits -> !

...y-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java

sentry-android-core/src/main/java/io/sentry/android/core/AppLifecycleIntegration.java

sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java

bruno-garcia · 2020-04-22T17:15:12Z

sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java

+            addSessionBreadcrumb("end");
            hub.endSession();


We could pass here the session duration:

Suggested change

addSessionBreadcrumb("end");

hub.endSession();

hub.endSession();

addSessionBreadcrumb("end", hub.getDuration());

there's no API for that right now, but yeah we could enrich these breadcrumbs later on

...ry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java

bruno-garcia · 2020-04-22T17:48:52Z

...ry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java

+      // TODO: should we log the params? sometimes its necessary eg
+      // ACTION_AIRPLANE_MODE_CHANGED
+      // its a single action with 2 states that differs on extras
+      final Bundle extras = intent.getExtras();
+      final Map<String, String> newExtras = new HashMap<>();
+      if (extras != null && !extras.isEmpty()) {
+        for (String item : extras.keySet()) {
+          try {
+            newExtras.put(item, extras.get(item).toString());
+          } catch (Exception ignored) {
+          }
+        }
+      }
+      breadcrumb.setData("extras", newExtras);


Let's leave it as you did and see the feedback. If we only add stuff manually we could be missing things and there's the extra work/maintenance

sentry-android-core/src/main/java/io/sentry/android/core/TempSensorBreadcrumbsIntegration.java

ninniuz · 2020-04-23T09:30:14Z

@marandaneto Did you take into account that if android build has been obfuscated (e.g. with ProGuard or DexGuard), activity names in the breadcrumb messages will look like gibberish and won't be retraced?
Also, is there a way to disable the integration once this is merged?

we are gonna probably extract this module as a new maven package (android aar) and ship it with a proguard rule for that, keeping Activity names.
the other way would be just to add breadcrumbs yourselves with your own activity identifies.

not sure if we are gonna demangle the breadcrumbs as well, I'll check this internally.

AFAIK Activity names are kept if they are found in the AndroidManifest.xml file (AAPT generates rules for this in intermediates/aapt_proguard_file), together with Services and BroadcastReceivers, as otherwise the OS would not be able to even launch any of those components. WDYT?

marandaneto · 2020-04-23T10:01:16Z

@marandaneto Did you take into account that if android build has been obfuscated (e.g. with ProGuard or DexGuard), activity names in the breadcrumb messages will look like gibberish and won't be retraced?
Also, is there a way to disable the integration once this is merged?

we are gonna probably extract this module as a new maven package (android aar) and ship it with a proguard rule for that, keeping Activity names.
the other way would be just to add breadcrumbs yourselves with your own activity identifies.
not sure if we are gonna demangle the breadcrumbs as well, I'll check this internally.

AFAIK Activity names are kept if they are found in the AndroidManifest.xml file (AAPT generates rules for this in intermediates/aapt_proguard_file), together with Services and BroadcastReceivers, as otherwise the OS would not be able to even launch any of those components. WDYT?

yes,

@marandaneto Did you take into account that if android build has been obfuscated (e.g. with ProGuard or DexGuard), activity names in the breadcrumb messages will look like gibberish and won't be retraced?
Also, is there a way to disable the integration once this is merged?

we are gonna probably extract this module as a new maven package (android aar) and ship it with a proguard rule for that, keeping Activity names.
the other way would be just to add breadcrumbs yourselves with your own activity identifies.
not sure if we are gonna demangle the breadcrumbs as well, I'll check this internally.

AFAIK Activity names are kept if they are found in the AndroidManifest.xml file (AAPT generates rules for this in intermediates/aapt_proguard_file), together with Services and BroadcastReceivers, as otherwise the OS would not be able to even launch any of those components. WDYT?

can confirm, I was suspecting because of either proguard-android-optimize.txt or just Proguard/R8 by default, but it's aapt, TIL :D thanks.

…a/automatic_breadcrumbs

marandaneto added 3 commits April 8, 2020 17:42

draft for automatic breadcrumbs

0213453

Merge branch 'master' into enha/automatic_breadcrumbs

1f9a196

draft with more breadcrumbs

58f12cd

marandaneto added 3 commits April 17, 2020 10:37

fix merge conflict

9d684ef

Merge branch 'master' into enha/automatic_breadcrumbs

68f8137

Merge branch 'master' into enha/automatic_breadcrumbs

d0f6e47

marandaneto commented Apr 20, 2020

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ActivityBreadcrumbsIntegration.java Show resolved Hide resolved

a bit of refactoring

f0a22f7

marandaneto added 3 commits April 21, 2020 10:13

Merge branch 'master' into enha/automatic_breadcrumbs

0bf9f91

implement battery temperature and ringing phone breadcrumb

460c8a1

refactoring

1717733

marandaneto commented Apr 21, 2020

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java Show resolved Hide resolved

marandaneto commented Apr 21, 2020

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java Outdated Show resolved Hide resolved

marandaneto commented Apr 21, 2020

View reviewed changes

HazAT reviewed Apr 22, 2020

View reviewed changes

...y-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java Outdated Show resolved Hide resolved

marandaneto added 2 commits April 22, 2020 11:09

Merge branch 'master' into enha/automatic_breadcrumbs

02c2e2b

code review

282b658

add tests

75cbe9b

marandaneto requested a review from bruno-garcia April 22, 2020 14:11

marandaneto marked this pull request as ready for review April 22, 2020 14:11

marandaneto changed the title ~~automatic breadcrumbs~~ automatic breadcrumbs for app, activity and sessions lifecycles and system events Apr 22, 2020

marandaneto added 2 commits April 22, 2020 18:22

Merge branch 'master' into enha/automatic_breadcrumbs

f2eeb5b

added 150ms sleep to get enough time for triggering task

e68383c

bruno-garcia approved these changes Apr 22, 2020

View reviewed changes

marandaneto added 2 commits April 22, 2020 20:41

code review

7811042

code review

4358fb1

marandaneto added 4 commits April 24, 2020 09:16

added a test for shortening the action string

5e583ad

added tests for automatic breadcrumbs configurations on manifest

c7c8fdd

Merge branch 'master' of github.com:getsentry/sentry-android into enh…

064b7c0

…a/automatic_breadcrumbs

matching up keys and values

452ee82

marandaneto merged commit 4665894 into master Apr 30, 2020

marandaneto deleted the enha/automatic_breadcrumbs branch April 30, 2020 11:50

wzieba mentioned this pull request Apr 21, 2021

Automatic breadcrumbs for Android's fragments getsentry/sentry-java#1432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

automatic breadcrumbs for app, activity and sessions lifecycles and system events #348

automatic breadcrumbs for app, activity and sessions lifecycles and system events #348

marandaneto commented Apr 8, 2020 •

edited

Loading

mrmitew commented Apr 16, 2020

marandaneto commented Apr 16, 2020 •

edited

Loading

mrmitew commented Apr 16, 2020 •

edited

Loading

marandaneto commented Apr 16, 2020

codecov-io commented Apr 20, 2020 •

edited

Loading

marandaneto Apr 21, 2020 •

edited

Loading

bruno-garcia Apr 22, 2020

marandaneto commented Apr 22, 2020

bruno-garcia left a comment

bruno-garcia Apr 22, 2020

marandaneto Apr 22, 2020

bruno-garcia Apr 22, 2020

ninniuz commented Apr 23, 2020

marandaneto commented Apr 23, 2020

automatic breadcrumbs for app, activity and sessions lifecycles and system events #348

automatic breadcrumbs for app, activity and sessions lifecycles and system events #348

Conversation

marandaneto commented Apr 8, 2020 • edited Loading

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

mrmitew commented Apr 16, 2020

marandaneto commented Apr 16, 2020 • edited Loading

mrmitew commented Apr 16, 2020 • edited Loading

marandaneto commented Apr 16, 2020

codecov-io commented Apr 20, 2020 • edited Loading

Codecov Report

marandaneto Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

bruno-garcia Apr 22, 2020

Choose a reason for hiding this comment

marandaneto commented Apr 22, 2020

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia Apr 22, 2020

Choose a reason for hiding this comment

marandaneto Apr 22, 2020

Choose a reason for hiding this comment

bruno-garcia Apr 22, 2020

Choose a reason for hiding this comment

ninniuz commented Apr 23, 2020

marandaneto commented Apr 23, 2020

marandaneto commented Apr 8, 2020 •

edited

Loading

marandaneto commented Apr 16, 2020 •

edited

Loading

mrmitew commented Apr 16, 2020 •

edited

Loading

codecov-io commented Apr 20, 2020 •

edited

Loading

marandaneto Apr 21, 2020 •

edited

Loading